Thread: bugfix: BUG #15477: Procedure call with named inout refcursorparameter - "invalid input syntax for type boolean"

On 02/11/2018 06:04, Pavel Stehule wrote:
> čt 1. 11. 2018 v 14:31 odesílatel Pavel Stehule <pavel.stehule@gmail.com
> <mailto:pavel.stehule@gmail.com>> napsal:
> 
>     Cleaned patch with regress tests
> 
> 
> minor cleaning

Could you explain your analysis of the problem and how this patch
proposes to fix it?

About the patch, I suspect printing out

NOTICE:  <unnamed portal 2>

in the regression tests might lead to unstable results if there is
concurrent activity.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Pavel Stehule <pavel.stehule@gmail.com> writes:
> pá 2. 11. 2018 v 9:02 odesílatel Peter Eisentraut <
> peter.eisentraut@2ndquadrant.com> napsal:
>> Could you explain your analysis of the problem and how this patch
>> proposes to fix it?

> The original code works with original function arguments - and this compare
> with argmodes fields. But when you use named args, this expectation is not
> valid.

Yeah.  IMO, the fundamental mistake in this code was to try to avoid
calling expand_function_arguments.  The proposed patch still tries to
avoid that, which is why it's still a mess and, I suspect, still fails
on args with default values.  You cannot process calls with named args
correctly without accounting for defaults.

I'm going to go see about converting this to just call
expand_function_arguments and then drop all the special-case code.

BTW, it looks to me like ExecuteCallStmt trashes the passed-in CallStmt
in cases where expand_function_arguments is not a no-op.  Is that
really safe?  Seems to me it'd cause problems if, for example, dealing
with a CallStmt that's part of a prepared stmt or cached plan.

            regards, tom lane

I wrote:
> I'm going to go see about converting this to just call
> expand_function_arguments and then drop all the special-case code.

So while looking at that ... isn't the behavior for non-writable output
parameters basically insane?  It certainly fails to accord with the
plpgsql documentation, which shows an example that would throw an error:

    CREATE PROCEDURE triple(INOUT x int)
    ...
    CALL triple(5);

It's even weirder that you can get away with not supplying a writable
target value for an output argument so long as it has a default.

I think the behavior here ought to be "if the actual argument is a plpgsql
variable, assign the output back to it, otherwise do nothing".  That's
much closer to the behavior of OUT arguments in other old-school
programming languages.

            regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> so 3. 11. 2018 v 22:47 odesílatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> So while looking at that ... isn't the behavior for non-writable output
>> parameters basically insane?  It certainly fails to accord with the
>> plpgsql documentation, which shows an example that would throw an error:
>> 
>> CREATE PROCEDURE triple(INOUT x int)
>> ...
>> CALL triple(5);
>> 
>> It's even weirder that you can get away with not supplying a writable
>> target value for an output argument so long as it has a default.
>> 
>> I think the behavior here ought to be "if the actual argument is a plpgsql
>> variable, assign the output back to it, otherwise do nothing".  That's
>> much closer to the behavior of OUT arguments in other old-school
>> programming languages.

> I don't agree. The constant can be used only for IN parameter. Safe
> languages like Ada does copy result to variable used as INOUT parameter.
> PL/SQL doesn't allow it too.

Well, my main point is that that ISN'T what our current code does, nor
does your patch.  The reason I'm complaining is that after I rewrote the
code to use expand_function_arguments, it started rejecting this existing
regression test case:

CREATE PROCEDURE test_proc8c(INOUT a int, INOUT b int, INOUT c int DEFAULT 11)
...

  CALL test_proc8c(_a, _b);

I do not think you can argue that that's not broken while simultaneously
claiming that this error check promotes safe coding.

I looked into SQL:2011 to see what it has to say about this.  In
10.4 <routine invocation>, syntax rule 9) g) iii) says

    For each SQL parameter Pi, 1 (one) ≤ i ≤ SRNP, that is an output SQL
    parameter or both an input SQL parameter and an output SQL parameter,
    XAi shall be a <target specification>.

The immediately preceding rules make it clear that XAi is the actual
argument corresponding to parameter Pi *after* default-insertion and
named-argument reordering.  So our existing behavior here clearly
contradicts the spec: DEFAULT is not a <target specification>.

I couldn't find any really clear statement of what PL/SQL does
in this situation, but the docs I did find say that the actual
argument for an OUT parameter has to be a variable, with no
exceptions mentioned.

In short, I think it's a bug that we allow the above.  If you
want to keep the must-be-a-variable error then it should apply in
this case too.

            regards, tom lane

Pavel Stehule <pavel.stehule@gmail.com> writes:
> ne 4. 11. 2018 v 16:54 odesilatel Tom Lane <tgl@sss.pgh.pa.us> napsal:
>> In short, I think it's a bug that we allow the above.  If you
>> want to keep the must-be-a-variable error then it should apply in
>> this case too.

> I agree. This should be prohibited from PLpgSQL.

OK.  In that case I'll run with my patch.  The attached is what I had
code-wise, but I've not changed the regression tests to match ... gotta
go fix the docs too I guess.

            regards, tom lane

diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c
index 4552638..4e6d5b1 100644
*** a/src/pl/plpgsql/src/pl_exec.c
--- b/src/pl/plpgsql/src/pl_exec.c
***************
*** 30,35 ****
--- 30,36 ----
  #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"
*************** exec_stmt_call(PLpgSQL_execstate *estate
*** 2158,2260 ****
          plpgsql_create_econtext(estate);
      }

      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;
          }

--- 2159,2268 ----
          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;

          /*
!          * If first time through, 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.
           */
          if (!stmt->target)
          {
              Node       *node;
              FuncExpr   *funcexpr;
!             HeapTuple    func_tuple;
!             List       *funcargs;
              Oid           *argtypes;
              char      **argnames;
              char       *argmodes;
              MemoryContext oldcontext;
              PLpgSQL_row *row;
              int            nfields;
+             ListCell   *lc;
+             int            i;

              /*
!              * 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 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
                          ereport(ERROR,
                                  (errcode(ERRCODE_SYNTAX_ERROR),
!                                  errmsg("CALL argument %d is an output argument but is not writable",
!                                         i + 1)));
                  }
                  i++;
              }

              row->nfields = nfields;

              stmt->target = (PLpgSQL_variable *) row;
          }

Pavel Stehule <pavel.stehule@gmail.com> writes:
> I am not sure how safe is read argmodes from syscache after procedure
> execution. Theoretically, the procedure pg_proc tuple can be modified from
> procedure, and can be committed from procedure. Isn't better to safe
> argmodes before execution?

Hm.  That would mean throwing non-writable-arg errors before rather than
after, but that's probably fine.

            regards, tom lane

I wrote:
> BTW, it looks to me like ExecuteCallStmt trashes the passed-in CallStmt
> in cases where expand_function_arguments is not a no-op.  Is that
> really safe?  Seems to me it'd cause problems if, for example, dealing
> with a CallStmt that's part of a prepared stmt or cached plan.

I dug into why this wasn't falling over like mad, and the answer is that
the only reason it doesn't fall over is that plancache.c *always* decides
to build a new custom plan for any cached plan consisting strictly of
utility statements.  That's because cached_plan_cost() ignores utility
statements altogether, causing both the generic and custom plan costs
to be exactly zero, and choose_custom_plan() figures it might as well
break a tie in favor of generating a custom plan.  Therefore, even when
dealing with a cached CALL statement, ExecuteCallStmt always sees a
virgin new parse tree, and the fact that it corrupts it isn't a problem.

If I adjust the plancache logic so that it's capable of deciding to
use a generic cached plan, repeated execution of a CALL statement with
named parameters crashes at the seventh iteration, which is the first
one that would actually try to re-use a cached plan tree.

Nonetheless, make check-world passes, indicating that there is no place
in our regression tests that stresses this type of situation.  (On the
other hand, if I set plan_cache_mode = force_generic_plan, kaboom.)

This leaves me worried that there are probably other places besides
CallStmt that think they can scribble on a utility statement's parse
tree at execution time.  We're clearly not testing that situation
enough.

For the moment, I'm going to go fix ExecuteCallStmt so it doesn't do
this, but I'm wondering how we could detect such problems more
generically.

Also, it's just silly that the plancache won't use a generic plan for
utility statements, so I think we should change that (in HEAD only).
The easiest mod is to adjust cached_plan_cost() so that it charges
some nonzero amount for "planning" of utility statements.

            regards, tom lane

On 04/11/2018 16:54, Tom Lane wrote:
> I looked into SQL:2011 to see what it has to say about this.  In
> 10.4 <routine invocation>, syntax rule 9) g) iii) says
> 
>     For each SQL parameter Pi, 1 (one) ≤ i ≤ SRNP, that is an output SQL
>     parameter or both an input SQL parameter and an output SQL parameter,
>     XAi shall be a <target specification>.
> 
> The immediately preceding rules make it clear that XAi is the actual
> argument corresponding to parameter Pi *after* default-insertion and
> named-argument reordering.  So our existing behavior here clearly
> contradicts the spec: DEFAULT is not a <target specification>.

Note that parameter defaults with output parameters was only added in
SQL:2016.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services