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

@@ -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
+     without a <command>RETURN</command> statement.  If you wish to use
-     a <command>RETURN</command> statement is desired to exit the code early,
+     a <command>RETURN</command> statement to exit the code early, write
-     then <symbol>NULL</symbol> must be returned.  Returning any other value
+     just <command>RETURN</command> with no expression.
-     will result in an error.
     </para>
 
     <para>
-     If a procedure has output parameters, then the output values can be
+     If the procedure has output parameters, the final values of the output
-     assigned to the parameters as if they were variables.  For example:
+     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>

doc/src/sgml/ref/call.sgml

@@ -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>

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:  <NULL>

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)
-		{
+			elog(ERROR, "DO statement returned a row");
-			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;
-		}
 
 		exec_move_row(estate, stmt->target, tuptab->vals[0], tuptab->tupdesc);
 	}

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