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;
+$$;