From 15c7293477a6de03234f58898da7fb29f3ab5b94 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 4 Nov 2018 13:25:39 -0500 Subject: [PATCH] Fix bugs in plpgsql's handling of CALL argument lists. exec_stmt_call() tried to extract information out of a CALL statement's argument list without using expand_function_arguments(), apparently in the hope of saving a few nanoseconds by not processing defaulted arguments. It got that quite wrong though, leading to crashes with named arguments, as well as failure to enforce writability of the argument for a defaulted INOUT parameter. Fix and simplify the logic by using expand_function_arguments() before examining the list. Also, move the argument-examination to just after producing the CALL command's plan, before invoking the called procedure. This ensures that we'll track possible changes in the procedure's argument list correctly, and avoids a hazard of the plan cache being flushed while the procedure executes. Also fix assorted falsehoods and omissions in associated documentation. Per bug #15477 from Alexey Stepanov. Patch by me, with some help from Pavel Stehule. Back-patch to v11. Discussion: https://postgr.es/m/15477-86075b1d1d319e0a@postgresql.org Discussion: https://postgr.es/m/CAFj8pRA6UsujpTs9Sdwmk-R6yQykPx46wgjj+YZ7zxm4onrDyw@mail.gmail.com --- doc/src/sgml/plpgsql.sgml | 34 ++- doc/src/sgml/ref/call.sgml | 11 +- src/pl/plpgsql/src/expected/plpgsql_call.out | 84 ++++++- src/pl/plpgsql/src/pl_exec.c | 221 ++++++++++--------- src/pl/plpgsql/src/sql/plpgsql_call.sql | 76 ++++++- 5 files changed, 300 insertions(+), 126 deletions(-) diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml index 4344ceadbe..beb7e03bbc 100644 --- a/doc/src/sgml/plpgsql.sgml +++ b/doc/src/sgml/plpgsql.sgml @@ -1864,15 +1864,29 @@ SELECT * FROM get_available_flightid(CURRENT_DATE); A procedure does not have a return value. A procedure can therefore end - without a RETURN statement. If - a RETURN statement is desired to exit the code early, - then NULL must be returned. Returning any other value - will result in an error. + without a RETURN statement. If you wish to use + a RETURN statement to exit the code early, write + just RETURN with no expression. - If a procedure has output parameters, then the output values can be - assigned to the parameters as if they were variables. For example: + If the procedure has output parameters, the final values of the output + parameter variables will be returned to the caller. + + + + + Calling a Procedure + + + A PL/pgSQL function, procedure, + or DO block can call a procedure + using CALL. Output parameters are handled + differently from the way that CALL works in plain + SQL. Each INOUT parameter of the procedure must + correspond to a variable in the CALL statement, and + whatever the procedure returns is assigned back to that variable after + it returns. For example: CREATE PROCEDURE triple(INOUT x int) LANGUAGE plpgsql @@ -1882,7 +1896,13 @@ BEGIN END; $$; -CALL triple(5); +DO $$ +DECLARE myvar int := 5; +BEGIN + CALL triple(myvar); + RAISE NOTICE 'myvar = %', myvar; -- prints 15 +END +$$; diff --git a/doc/src/sgml/ref/call.sgml b/doc/src/sgml/ref/call.sgml index 7418e19eeb..abaa81c78b 100644 --- a/doc/src/sgml/ref/call.sgml +++ b/doc/src/sgml/ref/call.sgml @@ -33,7 +33,8 @@ CALL name ( [ name ( [ argument - An argument for the procedure call. + An input argument for the procedure call. See for the full details on function and procedure call syntax, including use of named parameters. @@ -81,6 +82,12 @@ CALL name ( [ . + diff --git a/src/pl/plpgsql/src/expected/plpgsql_call.out b/src/pl/plpgsql/src/expected/plpgsql_call.out index 547ca22a55..d9c88e85c8 100644 --- a/src/pl/plpgsql/src/expected/plpgsql_call.out +++ b/src/pl/plpgsql/src/expected/plpgsql_call.out @@ -114,7 +114,7 @@ BEGIN RAISE INFO 'x = %, y = %', x, y; END; $$; -ERROR: argument 2 is an output argument but is not writable +ERROR: procedure parameter "b" is an output parameter but corresponding argument is not writable CONTEXT: PL/pgSQL function inline_code_block line 6 at CALL DO LANGUAGE plpgsql @@ -227,28 +227,43 @@ $$; DO $$ DECLARE _a int; _b int; _c int; BEGIN - _a := 10; _b := 30; _c := 50; - CALL test_proc8c(_a, _b); - RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; - _a := 10; _b := 30; _c := 50; - CALL test_proc8c(_a, b => _b); - RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; _a := 10; _b := 30; _c := 50; CALL test_proc8c(_a, _b, _c); RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; _a := 10; _b := 30; _c := 50; + CALL test_proc8c(_a, c => _c, b => _b); + RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; + _a := 10; _b := 30; _c := 50; CALL test_proc8c(c => _c, b => _b, a => _a); RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; END $$; -NOTICE: a: 10, b: 30, c: 11 -NOTICE: _a: 100, _b: 40, _c: 50 -NOTICE: a: 10, b: 30, c: 11 -NOTICE: _a: 100, _b: 40, _c: 50 NOTICE: a: 10, b: 30, c: 50 NOTICE: _a: 100, _b: 40, _c: -500 NOTICE: a: 10, b: 30, c: 50 NOTICE: _a: 100, _b: 40, _c: -500 +NOTICE: a: 10, b: 30, c: 50 +NOTICE: _a: 100, _b: 40, _c: -500 +DO $$ +DECLARE _a int; _b int; _c int; +BEGIN + _a := 10; _b := 30; _c := 50; + CALL test_proc8c(_a, _b); -- fail, no output argument for c + RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; +END +$$; +ERROR: procedure parameter "c" is an output parameter but corresponding argument is not writable +CONTEXT: PL/pgSQL function inline_code_block line 5 at CALL +DO $$ +DECLARE _a int; _b int; _c int; +BEGIN + _a := 10; _b := 30; _c := 50; + CALL test_proc8c(_a, b => _b); -- fail, no output argument for c + RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; +END +$$; +ERROR: procedure parameter "c" is an output parameter but corresponding argument is not writable +CONTEXT: PL/pgSQL function inline_code_block line 5 at CALL -- transition variable assignment TRUNCATE test1; CREATE FUNCTION triggerfunc1() RETURNS trigger @@ -276,3 +291,50 @@ DROP PROCEDURE test_proc1; DROP PROCEDURE test_proc3; DROP PROCEDURE test_proc4; DROP TABLE test1; +-- more checks for named-parameter handling +CREATE PROCEDURE p1(v_cnt int, v_Text inout text = NULL) +AS $$ +BEGIN + v_Text := 'v_cnt = ' || v_cnt; +END +$$ LANGUAGE plpgsql; +DO $$ +DECLARE + v_Text text; + v_cnt integer := 42; +BEGIN + CALL p1(v_cnt := v_cnt); -- error, must supply something for v_Text + RAISE NOTICE '%', v_Text; +END; +$$; +ERROR: procedure parameter "v_text" is an output parameter but corresponding argument is not writable +CONTEXT: PL/pgSQL function inline_code_block line 6 at CALL +DO $$ +DECLARE + v_Text text; + v_cnt integer := 42; +BEGIN + CALL p1(v_cnt := v_cnt, v_Text := v_Text); + RAISE NOTICE '%', v_Text; +END; +$$; +NOTICE: v_cnt = 42 +DO $$ +DECLARE + v_Text text; +BEGIN + CALL p1(10, v_Text := v_Text); + RAISE NOTICE '%', v_Text; +END; +$$; +NOTICE: v_cnt = 10 +DO $$ +DECLARE + v_Text text; + v_cnt integer; +BEGIN + CALL p1(v_Text := v_Text, v_cnt := v_cnt); + RAISE NOTICE '%', v_Text; +END; +$$; +NOTICE: diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 45526383f2..8dc716bee4 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -30,6 +30,7 @@ #include "funcapi.h" #include "miscadmin.h" #include "nodes/nodeFuncs.h" +#include "optimizer/clauses.h" #include "optimizer/planner.h" #include "parser/parse_coerce.h" #include "parser/scansup.h" @@ -2071,7 +2072,6 @@ static int exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt) { PLpgSQL_expr *expr = stmt->expr; - SPIPlanPtr plan; ParamListInfo paramLI; LocalTransactionId before_lxid; LocalTransactionId after_lxid; @@ -2080,6 +2080,8 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt) if (expr->plan == NULL) { + SPIPlanPtr plan; + /* * Don't save the plan if not in atomic context. Otherwise, * transaction ends would cause errors about plancache leaks. XXX @@ -2093,7 +2095,117 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt) * snapshot management in SPI_execute*, so don't let it do it. * Instead, we set the snapshots ourselves below. */ - expr->plan->no_snapshots = true; + plan = expr->plan; + plan->no_snapshots = true; + + /* + * We construct a DTYPE_ROW datum representing the plpgsql variables + * associated with the procedure's output arguments. Then we can use + * exec_move_row() to do the assignments. (We do this each time the + * plan changes, in case the procedure's argument list has changed.) + */ + if (stmt->is_call) + { + Node *node; + FuncExpr *funcexpr; + HeapTuple func_tuple; + List *funcargs; + Oid *argtypes; + char **argnames; + char *argmodes; + MemoryContext oldcontext; + PLpgSQL_row *row; + int nfields; + int i; + ListCell *lc; + + /* + * Get the parsed CallStmt, and look up the called procedure + */ + node = linitial_node(Query, + ((CachedPlanSource *) linitial(plan->plancache_list))->query_list)->utilityStmt; + if (node == NULL || !IsA(node, CallStmt)) + elog(ERROR, "query for CALL statement is not a CallStmt"); + + funcexpr = ((CallStmt *) node)->funcexpr; + + func_tuple = SearchSysCache1(PROCOID, + ObjectIdGetDatum(funcexpr->funcid)); + if (!HeapTupleIsValid(func_tuple)) + elog(ERROR, "cache lookup failed for function %u", + funcexpr->funcid); + + /* + * Extract function arguments, and expand any named-arg notation + */ + funcargs = expand_function_arguments(funcexpr->args, + funcexpr->funcresulttype, + func_tuple); + + /* + * Get the argument names and modes, too + */ + get_func_arg_info(func_tuple, &argtypes, &argnames, &argmodes); + + ReleaseSysCache(func_tuple); + + /* + * Begin constructing row Datum + */ + oldcontext = MemoryContextSwitchTo(estate->func->fn_cxt); + + row = (PLpgSQL_row *) palloc0(sizeof(PLpgSQL_row)); + row->dtype = PLPGSQL_DTYPE_ROW; + row->refname = "(unnamed row)"; + row->lineno = -1; + row->varnos = (int *) palloc(sizeof(int) * list_length(funcargs)); + + MemoryContextSwitchTo(oldcontext); + + /* + * Examine procedure's argument list. Each output arg position + * should be an unadorned plpgsql variable (Datum), which we can + * insert into the row Datum. + */ + nfields = 0; + i = 0; + foreach(lc, funcargs) + { + Node *n = lfirst(lc); + + if (argmodes && + (argmodes[i] == PROARGMODE_INOUT || + argmodes[i] == PROARGMODE_OUT)) + { + if (IsA(n, Param)) + { + Param *param = (Param *) n; + + /* paramid is offset by 1 (see make_datum_param()) */ + row->varnos[nfields++] = param->paramid - 1; + } + else + { + /* report error using parameter name, if available */ + if (argnames && argnames[i] && argnames[i][0]) + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("procedure parameter \"%s\" is an output parameter but corresponding argument is not writable", + argnames[i]))); + else + ereport(ERROR, + (errcode(ERRCODE_SYNTAX_ERROR), + errmsg("procedure parameter %d is an output parameter but corresponding argument is not writable", + i + 1))); + } + } + i++; + } + + row->nfields = nfields; + + stmt->target = (PLpgSQL_variable *) row; + } } paramLI = setup_param_list(estate, expr); @@ -2127,17 +2239,15 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt) } PG_END_TRY(); - plan = expr->plan; - if (expr->plan && !expr->plan->saved) expr->plan = NULL; - after_lxid = MyProc->lxid; - if (rc < 0) elog(ERROR, "SPI_execute_plan_with_paramlist failed executing query \"%s\": %s", expr->query, SPI_result_code_string(rc)); + after_lxid = MyProc->lxid; + if (before_lxid == after_lxid) { /* @@ -2158,105 +2268,16 @@ exec_stmt_call(PLpgSQL_execstate *estate, PLpgSQL_stmt_call *stmt) plpgsql_create_econtext(estate); } + /* + * Check result rowcount; if there's one row, assign procedure's output + * values back to the appropriate variables. + */ if (SPI_processed == 1) { SPITupleTable *tuptab = SPI_tuptable; - /* - * Construct a dummy target row based on the output arguments of the - * procedure call. - */ if (!stmt->target) - { - Node *node; - ListCell *lc; - FuncExpr *funcexpr; - int i; - HeapTuple tuple; - Oid *argtypes; - char **argnames; - char *argmodes; - MemoryContext oldcontext; - PLpgSQL_row *row; - int nfields; - - /* - * Get the original CallStmt - */ - node = linitial_node(Query, ((CachedPlanSource *) linitial(plan->plancache_list))->query_list)->utilityStmt; - if (!IsA(node, CallStmt)) - elog(ERROR, "returned row from not a CallStmt"); - - funcexpr = castNode(CallStmt, node)->funcexpr; - - /* - * Get the argument modes - */ - tuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcexpr->funcid)); - if (!HeapTupleIsValid(tuple)) - elog(ERROR, "cache lookup failed for function %u", funcexpr->funcid); - get_func_arg_info(tuple, &argtypes, &argnames, &argmodes); - ReleaseSysCache(tuple); - - /* - * Construct row - */ - oldcontext = MemoryContextSwitchTo(estate->func->fn_cxt); - - row = palloc0(sizeof(*row)); - row->dtype = PLPGSQL_DTYPE_ROW; - row->refname = "(unnamed row)"; - row->lineno = -1; - row->varnos = palloc(sizeof(int) * FUNC_MAX_ARGS); - - nfields = 0; - i = 0; - foreach(lc, funcexpr->args) - { - Node *n = lfirst(lc); - - if (argmodes && argmodes[i] == PROARGMODE_INOUT) - { - if (IsA(n, Param)) - { - Param *param = castNode(Param, n); - - /* paramid is offset by 1 (see make_datum_param()) */ - row->varnos[nfields++] = param->paramid - 1; - } - else if (IsA(n, NamedArgExpr)) - { - NamedArgExpr *nexpr = castNode(NamedArgExpr, n); - Param *param; - - if (!IsA(nexpr->arg, Param)) - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("argument %d is an output argument but is not writable", i + 1))); - - param = castNode(Param, nexpr->arg); - - /* - * Named arguments must be after positional arguments, - * so we can increase nfields. - */ - row->varnos[nexpr->argnumber] = param->paramid - 1; - nfields++; - } - else - ereport(ERROR, - (errcode(ERRCODE_SYNTAX_ERROR), - errmsg("argument %d is an output argument but is not writable", i + 1))); - } - i++; - } - - row->nfields = nfields; - - MemoryContextSwitchTo(oldcontext); - - stmt->target = (PLpgSQL_variable *) row; - } + elog(ERROR, "DO statement returned a row"); exec_move_row(estate, stmt->target, tuptab->vals[0], tuptab->tupdesc); } diff --git a/src/pl/plpgsql/src/sql/plpgsql_call.sql b/src/pl/plpgsql/src/sql/plpgsql_call.sql index 29e85803e7..4702bd14d1 100644 --- a/src/pl/plpgsql/src/sql/plpgsql_call.sql +++ b/src/pl/plpgsql/src/sql/plpgsql_call.sql @@ -206,21 +206,36 @@ $$; DO $$ DECLARE _a int; _b int; _c int; BEGIN - _a := 10; _b := 30; _c := 50; - CALL test_proc8c(_a, _b); - RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; - _a := 10; _b := 30; _c := 50; - CALL test_proc8c(_a, b => _b); - RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; _a := 10; _b := 30; _c := 50; CALL test_proc8c(_a, _b, _c); RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; _a := 10; _b := 30; _c := 50; + CALL test_proc8c(_a, c => _c, b => _b); + RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; + _a := 10; _b := 30; _c := 50; CALL test_proc8c(c => _c, b => _b, a => _a); RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; END $$; +DO $$ +DECLARE _a int; _b int; _c int; +BEGIN + _a := 10; _b := 30; _c := 50; + CALL test_proc8c(_a, _b); -- fail, no output argument for c + RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; +END +$$; + +DO $$ +DECLARE _a int; _b int; _c int; +BEGIN + _a := 10; _b := 30; _c := 50; + CALL test_proc8c(_a, b => _b); -- fail, no output argument for c + RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c; +END +$$; + -- transition variable assignment @@ -251,3 +266,52 @@ DROP PROCEDURE test_proc3; DROP PROCEDURE test_proc4; DROP TABLE test1; + + +-- more checks for named-parameter handling + +CREATE PROCEDURE p1(v_cnt int, v_Text inout text = NULL) +AS $$ +BEGIN + v_Text := 'v_cnt = ' || v_cnt; +END +$$ LANGUAGE plpgsql; + +DO $$ +DECLARE + v_Text text; + v_cnt integer := 42; +BEGIN + CALL p1(v_cnt := v_cnt); -- error, must supply something for v_Text + RAISE NOTICE '%', v_Text; +END; +$$; + +DO $$ +DECLARE + v_Text text; + v_cnt integer := 42; +BEGIN + CALL p1(v_cnt := v_cnt, v_Text := v_Text); + RAISE NOTICE '%', v_Text; +END; +$$; + +DO $$ +DECLARE + v_Text text; +BEGIN + CALL p1(10, v_Text := v_Text); + RAISE NOTICE '%', v_Text; +END; +$$; + +DO $$ +DECLARE + v_Text text; + v_cnt integer; +BEGIN + CALL p1(v_Text := v_Text, v_cnt := v_cnt); + RAISE NOTICE '%', v_Text; +END; +$$;