From 0dcf68e5a1f1c600bdf29b47cab30a62301a7af1 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sat, 16 Jun 2018 14:10:17 -0400 Subject: [PATCH] Fix some minor error-checking oversights in ParseFuncOrColumn(). Recent additions to ParseFuncOrColumn to make it reject non-procedure functions in CALL were neither adequate nor documented. Reorganize the code to ensure uniform results for all the cases that should be rejected. Also, use ERRCODE_WRONG_OBJECT_TYPE for this case as well as the converse case of a procedure in a non-CALL context. The original coding used ERRCODE_UNDEFINED_FUNCTION which seems wrong, and is certainly inconsistent with the adjacent wrong-kind-of-routine errors. This reorganization also causes the checks for aggregate decoration with a non-aggregate function to be made in the FUNCDETAIL_COERCION case; that they were not is a long-standing oversight. Discussion: https://postgr.es/m/14497.1529089235@sss.pgh.pa.us --- src/backend/parser/parse_func.c | 98 +++++++++++-------- .../regress/expected/create_procedure.out | 1 + 2 files changed, 57 insertions(+), 42 deletions(-) diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index ea5d5212b4..21ddd5b7e0 100644 --- a/src/backend/parser/parse_func.c +++ b/src/backend/parser/parse_func.c @@ -68,6 +68,9 @@ static Node *ParseComplexProjection(ParseState *pstate, const char *funcname, * last_srf should be a copy of pstate->p_last_srf from just before we * started transforming fargs. If the caller knows that fargs couldn't * contain any SRF calls, last_srf can just be pstate->p_last_srf. + * + * proc_call is true if we are considering a CALL statement, so that the + * name must resolve to a procedure name, not anything else. */ Node * ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, @@ -204,7 +207,8 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, * the "function call" could be a projection. We also check that there * wasn't any aggregate or variadic decoration, nor an argument name. */ - if (nargs == 1 && agg_order == NIL && agg_filter == NULL && !agg_star && + if (nargs == 1 && !proc_call && + agg_order == NIL && agg_filter == NULL && !agg_star && !agg_distinct && over == NULL && !func_variadic && argnames == NIL && list_length(funcname) == 1) { @@ -253,21 +257,42 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, cancel_parser_errposition_callback(&pcbstate); - if (fdresult == FUNCDETAIL_COERCION) + /* + * Check for various wrong-kind-of-routine cases. + */ + + /* If this is a CALL, reject things that aren't procedures */ + if (proc_call && + (fdresult == FUNCDETAIL_NORMAL || + fdresult == FUNCDETAIL_AGGREGATE || + fdresult == FUNCDETAIL_WINDOWFUNC || + fdresult == FUNCDETAIL_COERCION)) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("%s is not a procedure", + func_signature_string(funcname, nargs, + argnames, + actual_arg_types)), + errhint("To call a function, use SELECT."), + parser_errposition(pstate, location))); + /* Conversely, if not a CALL, reject procedures */ + if (fdresult == FUNCDETAIL_PROCEDURE && !proc_call) + ereport(ERROR, + (errcode(ERRCODE_WRONG_OBJECT_TYPE), + errmsg("%s is a procedure", + func_signature_string(funcname, nargs, + argnames, + actual_arg_types)), + errhint("To call a procedure, use CALL."), + parser_errposition(pstate, location))); + + if (fdresult == FUNCDETAIL_NORMAL || + fdresult == FUNCDETAIL_PROCEDURE || + fdresult == FUNCDETAIL_COERCION) { /* - * We interpreted it as a type coercion. coerce_type can handle these - * cases, so why duplicate code... - */ - return coerce_type(pstate, linitial(fargs), - actual_arg_types[0], rettype, -1, - COERCION_EXPLICIT, COERCE_EXPLICIT_CALL, location); - } - else if (fdresult == FUNCDETAIL_NORMAL || fdresult == FUNCDETAIL_PROCEDURE) - { - /* - * Normal function found; was there anything indicating it must be an - * aggregate? + * In these cases, complain if there was anything indicating it must + * be an aggregate or window function. */ if (agg_star) ereport(ERROR, @@ -306,26 +331,14 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, errmsg("OVER specified, but %s is not a window function nor an aggregate function", NameListToString(funcname)), parser_errposition(pstate, location))); + } - if (fdresult == FUNCDETAIL_NORMAL && proc_call) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_FUNCTION), - errmsg("%s is not a procedure", - func_signature_string(funcname, nargs, - argnames, - actual_arg_types)), - errhint("To call a function, use SELECT."), - parser_errposition(pstate, location))); - - if (fdresult == FUNCDETAIL_PROCEDURE && !proc_call) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_FUNCTION), - errmsg("%s is a procedure", - func_signature_string(funcname, nargs, - argnames, - actual_arg_types)), - errhint("To call a procedure, use CALL."), - parser_errposition(pstate, location))); + /* + * So far so good, so do some routine-type-specific processing. + */ + if (fdresult == FUNCDETAIL_NORMAL || fdresult == FUNCDETAIL_PROCEDURE) + { + /* Nothing special to do for these cases. */ } else if (fdresult == FUNCDETAIL_AGGREGATE) { @@ -336,15 +349,6 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, Form_pg_aggregate classForm; int catDirectArgs; - if (proc_call) - ereport(ERROR, - (errcode(ERRCODE_UNDEFINED_FUNCTION), - errmsg("%s is not a procedure", - func_signature_string(funcname, nargs, - argnames, - actual_arg_types)), - parser_errposition(pstate, location))); - tup = SearchSysCache1(AGGFNOID, ObjectIdGetDatum(funcid)); if (!HeapTupleIsValid(tup)) /* should not happen */ elog(ERROR, "cache lookup failed for aggregate %u", funcid); @@ -510,6 +514,16 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs, NameListToString(funcname)), parser_errposition(pstate, location))); } + else if (fdresult == FUNCDETAIL_COERCION) + { + /* + * We interpreted it as a type coercion. coerce_type can handle these + * cases, so why duplicate code... + */ + return coerce_type(pstate, linitial(fargs), + actual_arg_types[0], rettype, -1, + COERCION_EXPLICIT, COERCE_EXPLICIT_CALL, location); + } else { /* diff --git a/src/test/regress/expected/create_procedure.out b/src/test/regress/expected/create_procedure.out index 67d671727c..30495971bc 100644 --- a/src/test/regress/expected/create_procedure.out +++ b/src/test/regress/expected/create_procedure.out @@ -126,6 +126,7 @@ CALL sum(1); -- error: not a procedure ERROR: sum(integer) is not a procedure LINE 1: CALL sum(1); ^ +HINT: To call a function, use SELECT. CREATE PROCEDURE ptestx() LANGUAGE SQL WINDOW AS $$ INSERT INTO cp_test VALUES (1, 'a') $$; ERROR: invalid attribute in procedure definition LINE 1: CREATE PROCEDURE ptestx() LANGUAGE SQL WINDOW AS $$ INSERT I...