]> granicus.if.org Git - postgresql/commitdiff
Fix bugs in plpgsql's handling of CALL argument lists.
authorTom Lane <tgl@sss.pgh.pa.us>
Sun, 4 Nov 2018 18:25:39 +0000 (13:25 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Sun, 4 Nov 2018 18:25:39 +0000 (13:25 -0500)
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
doc/src/sgml/ref/call.sgml
src/pl/plpgsql/src/expected/plpgsql_call.out
src/pl/plpgsql/src/pl_exec.c
src/pl/plpgsql/src/sql/plpgsql_call.sql

index 4344ceadbe48b108d5cf94cd81e492b21410066f..beb7e03bbcfb181f04e5c3287544924b17d01985 100644 (file)
@@ -1864,15 +1864,29 @@ SELECT * FROM get_available_flightid(CURRENT_DATE);
 
     <para>
      A procedure does not have a return value.  A procedure can therefore end
-     without a <command>RETURN</command> statement.  If
-     a <command>RETURN</command> statement is desired to exit the code early,
-     then <symbol>NULL</symbol> must be returned.  Returning any other value
-     will result in an error.
+     without a <command>RETURN</command> statement.  If you wish to use
+     a <command>RETURN</command> statement to exit the code early, write
+     just <command>RETURN</command> with no expression.
     </para>
 
     <para>
-     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.
+    </para>
+   </sect2>
+
+   <sect2 id="plpgsql-statements-calling-procedure">
+    <title>Calling a Procedure</title>
+
+    <para>
+     A <application>PL/pgSQL</application> function, procedure,
+     or <command>DO</command> block can call a procedure
+     using <command>CALL</command>.  Output parameters are handled
+     differently from the way that <command>CALL</command> works in plain
+     SQL.  Each <literal>INOUT</literal> parameter of the procedure must
+     correspond to a variable in the <command>CALL</command> statement, and
+     whatever the procedure returns is assigned back to that variable after
+     it returns.  For example:
 <programlisting>
 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
+$$;
 </programlisting>
     </para>
    </sect2>
index 7418e19eeba81e592a83bc0a1e81b17e0f8526e8..abaa81c78b94b2e510b2dd842a6654ae4ffbb1dc 100644 (file)
@@ -33,7 +33,8 @@ CALL <replaceable class="parameter">name</replaceable> ( [ <replaceable class="p
   </para>
 
   <para>
-   If the procedure has output arguments, then a result row will be returned.
+   If the procedure has any output parameters, then a result row will be
+   returned, containing the values of those parameters.
   </para>
  </refsect1>
 
@@ -54,7 +55,7 @@ CALL <replaceable class="parameter">name</replaceable> ( [ <replaceable class="p
     <term><replaceable class="parameter">argument</replaceable></term>
     <listitem>
      <para>
-      An argument for the procedure call.
+      An input argument for the procedure call.
       See <xref linkend="sql-syntax-calling-funcs"/> for the full details on
       function and procedure call syntax, including use of named parameters.
      </para>
@@ -81,6 +82,12 @@ CALL <replaceable class="parameter">name</replaceable> ( [ <replaceable class="p
    Transaction control statements are only allowed if <command>CALL</command>
    is executed in its own transaction.
   </para>
+
+  <para>
+   <application>PL/pgSQL</application> handles output parameters
+   in <command>CALL</command> commands differently;
+   see <xref linkend="plpgsql-statements-calling-procedure"/>.
+  </para>
  </refsect1>
 
  <refsect1>
index 547ca22a55aeb9c9e0f963b204a23cdb4f639ade..d9c88e85c8d8725b15446abc8968198f2ae86144 100644 (file)
@@ -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
@@ -228,27 +228,42 @@ 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);
+  CALL test_proc8c(_a, _b, _c);
   RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
   _a := 10; _b := 30; _c := 50;
-  CALL test_proc8c(_a, _b, _c);
+  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:  <NULL>
index 45526383f25b5493c88ac9b762c65052a22d5897..8dc716bee451913620c5b140199cb06b7d729aa5 100644 (file)
@@ -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);
        }
index 29e85803e734b3a5e24e5d50dfb32c684390a48e..4702bd14d12e89238d6a100c77c88ba3560c6af2 100644 (file)
@@ -207,16 +207,31 @@ DO $$
 DECLARE _a int; _b int; _c int;
 BEGIN
   _a := 10; _b := 30; _c := 50;
-  CALL test_proc8c(_a, _b);
+  CALL test_proc8c(_a, _b, _c);
   RAISE NOTICE '_a: %, _b: %, _c: %', _a, _b, _c;
   _a := 10; _b := 30; _c := 50;
-  CALL test_proc8c(_a, b => _b);
+  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(_a, _b, _c);
+  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(c => _c, b => _b, a => _a);
+  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
 $$;
@@ -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;
+$$;