Fixing bug #8228 ("set-valued function called in context that cannot accept a set") - Mailing list pgsql-hackers

I kinda forgot about this bug when I went off on vacation:
http://www.postgresql.org/message-id/E1UnCv4-0007oF-Bo@wrigleys.postgresql.org

The way to fix it seems to be to do static prechecking of whether a
function's input arguments can return sets, rather than relying on
their behavior during the first execution.  We already have the
infrastructure needed, namely expression_returns_set(), so a fix can
be pretty short, as illustrated in the attached proposed patch.

Now one might object that this will add an unwanted amount of overhead
to query startup.  expression_returns_set() is fairly cheap, since it
only requires a parsetree walk and not any catalog lookups, but it's not
zero cost.  On the other hand, some of that cost is bought back in
normal non-set cases by the fact that we needn't go through
ExecMakeFunctionResult() even once.  I tried to measure whether there
was a slowdown using this test case:
    $ pgbench -c 4 -T 60 -S -n bench
in a non-assert build using a scale-factor-10 pgbench database, on an
8-core machine.  The best reported transaction rate over five trials was
35556.680918 in current HEAD, and 35466.438468 with the patch, for an
apparent slowdown of 0.3%.  This is below what I'd normally consider
significant, since the run-to-run variability is considerably more than
that, but there may be some actual slowdown there.

We could consider a more invasive fix that would try to push the static
checking back into the planner or even parser, but that would not make
things any faster when queries are only executed once after planning,
as is true in this test scenario.  In any case, adding fields to
FuncExpr/OpExpr would not be a back-patchable fix.

Thoughts?  Unless someone has a better idea, I'm inclined to push
this patch and call it good.

            regards, tom lane

diff --git a/src/backend/executor/execQual.c b/src/backend/executor/execQual.c
index 67dca78..6f45973 100644
*** a/src/backend/executor/execQual.c
--- b/src/backend/executor/execQual.c
*************** tupledesc_match(TupleDesc dst_tupdesc, T
*** 1634,1642 ****
   * init_fcache is presumed already run on the FuncExprState.
   *
   * This function handles the most general case, wherein the function or
!  * one of its arguments might (or might not) return a set.    If we find
!  * no sets involved, we will change the FuncExprState's function pointer
!  * to use a simpler method on subsequent calls.
   */
  static Datum
  ExecMakeFunctionResult(FuncExprState *fcache,
--- 1634,1640 ----
   * init_fcache is presumed already run on the FuncExprState.
   *
   * This function handles the most general case, wherein the function or
!  * one of its arguments can return a set.
   */
  static Datum
  ExecMakeFunctionResult(FuncExprState *fcache,
*************** restart:
*** 1906,1918 ****
          /*
           * Non-set case: much easier.
           *
!          * We change the ExprState function pointer to use the simpler
!          * ExecMakeFunctionResultNoSets on subsequent calls.  This amounts to
!          * assuming that no argument can return a set if it didn't do so the
!          * first time.
           */
-         fcache->xprstate.evalfunc = (ExprStateEvalFunc) ExecMakeFunctionResultNoSets;
-
          if (isDone)
              *isDone = ExprSingleResult;

--- 1904,1915 ----
          /*
           * Non-set case: much easier.
           *
!          * In common cases, this code path is unreachable because we'd have
!          * selected ExecMakeFunctionResultNoSets instead.  However, it's
!          * possible to get here if an argument sometimes produces set results
!          * and sometimes scalar results.  For example, a CASE expression might
!          * call a set-returning function in only some of its arms.
           */
          if (isDone)
              *isDone = ExprSingleResult;

*************** ExecEvalFunc(FuncExprState *fcache,
*** 2371,2380 ****
      init_fcache(func->funcid, func->inputcollid, fcache,
                  econtext->ecxt_per_query_memory, true);

!     /* Go directly to ExecMakeFunctionResult on subsequent uses */
!     fcache->xprstate.evalfunc = (ExprStateEvalFunc) ExecMakeFunctionResult;
!
!     return ExecMakeFunctionResult(fcache, econtext, isNull, isDone);
  }

  /* ----------------------------------------------------------------
--- 2368,2389 ----
      init_fcache(func->funcid, func->inputcollid, fcache,
                  econtext->ecxt_per_query_memory, true);

!     /*
!      * We need to invoke ExecMakeFunctionResult if either the function itself
!      * or any of its input expressions can return a set.  Otherwise, invoke
!      * ExecMakeFunctionResultNoSets.  In either case, change the evalfunc
!      * pointer to go directly there on subsequent uses.
!      */
!     if (fcache->func.fn_retset || expression_returns_set((Node *) func->args))
!     {
!         fcache->xprstate.evalfunc = (ExprStateEvalFunc) ExecMakeFunctionResult;
!         return ExecMakeFunctionResult(fcache, econtext, isNull, isDone);
!     }
!     else
!     {
!         fcache->xprstate.evalfunc = (ExprStateEvalFunc) ExecMakeFunctionResultNoSets;
!         return ExecMakeFunctionResultNoSets(fcache, econtext, isNull, isDone);
!     }
  }

  /* ----------------------------------------------------------------
*************** ExecEvalOper(FuncExprState *fcache,
*** 2394,2403 ****
      init_fcache(op->opfuncid, op->inputcollid, fcache,
                  econtext->ecxt_per_query_memory, true);

!     /* Go directly to ExecMakeFunctionResult on subsequent uses */
!     fcache->xprstate.evalfunc = (ExprStateEvalFunc) ExecMakeFunctionResult;
!
!     return ExecMakeFunctionResult(fcache, econtext, isNull, isDone);
  }

  /* ----------------------------------------------------------------
--- 2403,2424 ----
      init_fcache(op->opfuncid, op->inputcollid, fcache,
                  econtext->ecxt_per_query_memory, true);

!     /*
!      * We need to invoke ExecMakeFunctionResult if either the function itself
!      * or any of its input expressions can return a set.  Otherwise, invoke
!      * ExecMakeFunctionResultNoSets.  In either case, change the evalfunc
!      * pointer to go directly there on subsequent uses.
!      */
!     if (fcache->func.fn_retset || expression_returns_set((Node *) op->args))
!     {
!         fcache->xprstate.evalfunc = (ExprStateEvalFunc) ExecMakeFunctionResult;
!         return ExecMakeFunctionResult(fcache, econtext, isNull, isDone);
!     }
!     else
!     {
!         fcache->xprstate.evalfunc = (ExprStateEvalFunc) ExecMakeFunctionResultNoSets;
!         return ExecMakeFunctionResultNoSets(fcache, econtext, isNull, isDone);
!     }
  }

  /* ----------------------------------------------------------------
diff --git a/src/test/regress/expected/rangefuncs.out b/src/test/regress/expected/rangefuncs.out
index a988dd0..9d40510 100644
*** a/src/test/regress/expected/rangefuncs.out
--- b/src/test/regress/expected/rangefuncs.out
*************** select * from foobar();  -- fail
*** 1992,1994 ****
--- 1992,2008 ----
  ERROR:  function return row and query-specified return row do not match
  DETAIL:  Returned row contains 3 attributes, but query expects 2.
  drop function foobar();
+ -- check behavior when a function's input sometimes returns a set (bug #8228)
+ SELECT *,
+   lower(CASE WHEN id = 2 THEN (regexp_matches(str, '^0*([1-9]\d+)$'))[1]
+         ELSE str
+         END)
+ FROM
+   (VALUES (1,''), (2,'0000000049404'), (3,'FROM 10000000876')) v(id, str);
+  id |       str        |      lower
+ ----+------------------+------------------
+   1 |                  |
+   2 | 0000000049404    | 49404
+   3 | FROM 10000000876 | from 10000000876
+ (3 rows)
+
diff --git a/src/test/regress/sql/rangefuncs.sql b/src/test/regress/sql/rangefuncs.sql
index ac2769f..07d2e1a 100644
*** a/src/test/regress/sql/rangefuncs.sql
--- b/src/test/regress/sql/rangefuncs.sql
*************** $$ select (1, 2.1, 3) $$ language sql;
*** 599,601 ****
--- 599,610 ----
  select * from foobar();  -- fail

  drop function foobar();
+
+ -- check behavior when a function's input sometimes returns a set (bug #8228)
+
+ SELECT *,
+   lower(CASE WHEN id = 2 THEN (regexp_matches(str, '^0*([1-9]\d+)$'))[1]
+         ELSE str
+         END)
+ FROM
+   (VALUES (1,''), (2,'0000000049404'), (3,'FROM 10000000876')) v(id, str);