Re: [HACKERS] Re: Query fails when SRFs are part of FROM clause (Commit id: 69f4b9c85f) - Mailing list pgsql-hackers

I wrote:
> I'm a bit inclined to agree with the idea of explicitly requiring SRFs
> in FROM to appear only at the top level of the expression.

If we are going to go down this road, I think it would be a good idea
to try to provide a cursor position for the "can't accept a set" error
message, because otherwise it will be really unclear what's wrong with
something like

SELECT * FROM ROWS FROM (generate_series(1,10), abs(generate_series(1,10)));

I looked into what it would take to do that, and was pleasantly surprised
to find out that most of the infrastructure is already there since
commit 4c728f38.  Basically all we have to do is the attached.

Now that this infrastructure exists, anything that has access to a
PlanState or ExprContext, plus a parse node containing a location, is able
to report an error cursor.  It took a considerable effort of will not to
start plastering error position reports on a lot of other executor errors.
I think we should do that, but it smells new-feature-y, and hence
something to tackle for v11 not now.  But if v10 is moving the goalposts
on where you can put an SRF call, I think we should do this much in v10.

Naming note: a name like ExecErrPosition() would have been more consistent
with the other stuff that execUtils.c exports.  But since this is meant
to be used in ereport() calls, whose support functions generally aren't
camel-cased, I thought executor_errposition() was a better choice.
I'm not particularly set on that though.

            regards, tom lane

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 15d693f..5a34a46 100644
*** a/src/backend/executor/execExpr.c
--- b/src/backend/executor/execExpr.c
*************** ExecInitFunc(ExprEvalStep *scratch, Expr
*** 2103,2109 ****
      if (flinfo->fn_retset)
          ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                  errmsg("set-valued function called in context that cannot accept a set")));

      /* Build code to evaluate arguments directly into the fcinfo struct */
      argno = 0;
--- 2103,2111 ----
      if (flinfo->fn_retset)
          ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                  errmsg("set-valued function called in context that cannot accept a set"),
!                  parent ? executor_errposition(parent->state,
!                                           exprLocation((Node *) node)) : 0));

      /* Build code to evaluate arguments directly into the fcinfo struct */
      argno = 0;
diff --git a/src/backend/executor/execSRF.c b/src/backend/executor/execSRF.c
index 4badd5c..077ac20 100644
*** a/src/backend/executor/execSRF.c
--- b/src/backend/executor/execSRF.c
***************
*** 34,40 ****


  /* static function decls */
! static void init_sexpr(Oid foid, Oid input_collation, SetExprState *sexpr,
             MemoryContext sexprCxt, bool allowSRF, bool needDescForSRF);
  static void ShutdownSetExpr(Datum arg);
  static void ExecEvalFuncArgs(FunctionCallInfo fcinfo,
--- 34,41 ----


  /* static function decls */
! static void init_sexpr(Oid foid, Oid input_collation, Expr *node,
!            SetExprState *sexpr, PlanState *parent,
             MemoryContext sexprCxt, bool allowSRF, bool needDescForSRF);
  static void ShutdownSetExpr(Datum arg);
  static void ExecEvalFuncArgs(FunctionCallInfo fcinfo,
*************** ExecInitTableFunctionResult(Expr *expr,
*** 77,83 ****
          state->funcReturnsSet = func->funcretset;
          state->args = ExecInitExprList(func->args, parent);

!         init_sexpr(func->funcid, func->inputcollid, state,
                     econtext->ecxt_per_query_memory, func->funcretset, false);
      }
      else
--- 78,84 ----
          state->funcReturnsSet = func->funcretset;
          state->args = ExecInitExprList(func->args, parent);

!         init_sexpr(func->funcid, func->inputcollid, expr, state, parent,
                     econtext->ecxt_per_query_memory, func->funcretset, false);
      }
      else
*************** ExecInitFunctionResultSet(Expr *expr,
*** 438,444 ****
          FuncExpr   *func = (FuncExpr *) expr;

          state->args = ExecInitExprList(func->args, parent);
!         init_sexpr(func->funcid, func->inputcollid, state,
                     econtext->ecxt_per_query_memory, true, true);
      }
      else if (IsA(expr, OpExpr))
--- 439,445 ----
          FuncExpr   *func = (FuncExpr *) expr;

          state->args = ExecInitExprList(func->args, parent);
!         init_sexpr(func->funcid, func->inputcollid, expr, state, parent,
                     econtext->ecxt_per_query_memory, true, true);
      }
      else if (IsA(expr, OpExpr))
*************** ExecInitFunctionResultSet(Expr *expr,
*** 446,452 ****
          OpExpr       *op = (OpExpr *) expr;

          state->args = ExecInitExprList(op->args, parent);
!         init_sexpr(op->opfuncid, op->inputcollid, state,
                     econtext->ecxt_per_query_memory, true, true);
      }
      else
--- 447,453 ----
          OpExpr       *op = (OpExpr *) expr;

          state->args = ExecInitExprList(op->args, parent);
!         init_sexpr(op->opfuncid, op->inputcollid, expr, state, parent,
                     econtext->ecxt_per_query_memory, true, true);
      }
      else
*************** restart:
*** 645,651 ****
   * init_sexpr - initialize a SetExprState node during first use
   */
  static void
! init_sexpr(Oid foid, Oid input_collation, SetExprState *sexpr,
             MemoryContext sexprCxt, bool allowSRF, bool needDescForSRF)
  {
      AclResult    aclresult;
--- 646,653 ----
   * init_sexpr - initialize a SetExprState node during first use
   */
  static void
! init_sexpr(Oid foid, Oid input_collation, Expr *node,
!            SetExprState *sexpr, PlanState *parent,
             MemoryContext sexprCxt, bool allowSRF, bool needDescForSRF)
  {
      AclResult    aclresult;
*************** init_sexpr(Oid foid, Oid input_collation
*** 683,689 ****
      if (sexpr->func.fn_retset && !allowSRF)
          ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                  errmsg("set-valued function called in context that cannot accept a set")));

      /* Otherwise, caller should have marked the sexpr correctly */
      Assert(sexpr->func.fn_retset == sexpr->funcReturnsSet);
--- 685,693 ----
      if (sexpr->func.fn_retset && !allowSRF)
          ereport(ERROR,
                  (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
!                  errmsg("set-valued function called in context that cannot accept a set"),
!                  parent ? executor_errposition(parent->state,
!                                           exprLocation((Node *) node)) : 0));

      /* Otherwise, caller should have marked the sexpr correctly */
      Assert(sexpr->func.fn_retset == sexpr->funcReturnsSet);
diff --git a/src/backend/executor/execUtils.c b/src/backend/executor/execUtils.c
index df3d650..08229bd 100644
*** a/src/backend/executor/execUtils.c
--- b/src/backend/executor/execUtils.c
***************
*** 28,33 ****
--- 28,35 ----
   *        ExecOpenScanRelation    Common code for scan node init routines.
   *        ExecCloseScanRelation
   *
+  *        executor_errposition    Report syntactic position of an error.
+  *
   *        RegisterExprContextCallback    Register function shutdown callback
   *        UnregisterExprContextCallback  Deregister function shutdown callback
   *
***************
*** 44,49 ****
--- 46,52 ----
  #include "access/relscan.h"
  #include "access/transam.h"
  #include "executor/executor.h"
+ #include "mb/pg_wchar.h"
  #include "nodes/nodeFuncs.h"
  #include "parser/parsetree.h"
  #include "storage/lmgr.h"
*************** UpdateChangedParamSet(PlanState *node, B
*** 686,691 ****
--- 689,724 ----
  }

  /*
+  * executor_errposition
+  *        Report an execution-time cursor position, if possible.
+  *
+  * This is expected to be used within an ereport() call.  The return value
+  * is a dummy (always 0, in fact).
+  *
+  * The locations stored in parsetrees are byte offsets into the source string.
+  * We have to convert them to 1-based character indexes for reporting to
+  * clients.  (We do things this way to avoid unnecessary overhead in the
+  * normal non-error case: computing character indexes would be much more
+  * expensive than storing token offsets.)
+  */
+ int
+ executor_errposition(EState *estate, int location)
+ {
+     int            pos;
+
+     /* No-op if location was not provided */
+     if (location < 0)
+         return 0;
+     /* Can't do anything if source text is not available */
+     if (estate == NULL || estate->es_sourceText == NULL)
+         return 0;
+     /* Convert offset to character number */
+     pos = pg_mbstrlen_with_len(estate->es_sourceText, location) + 1;
+     /* And pass it to the ereport mechanism */
+     return errposition(pos);
+ }
+
+ /*
   * Register a shutdown callback in an ExprContext.
   *
   * Shutdown callbacks will be called (in reverse order of registration)
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index f7f3189..3107cf5 100644
*** a/src/include/executor/executor.h
--- b/src/include/executor/executor.h
*************** extern bool ExecRelationIsTargetRelation
*** 482,487 ****
--- 482,489 ----
  extern Relation ExecOpenScanRelation(EState *estate, Index scanrelid, int eflags);
  extern void ExecCloseScanRelation(Relation scanrel);

+ extern int    executor_errposition(EState *estate, int location);
+
  extern void RegisterExprContextCallback(ExprContext *econtext,
                              ExprContextCallbackFunction function,
                              Datum arg);
diff --git a/src/test/regress/expected/tsrf.out b/src/test/regress/expected/tsrf.out
index 33f370b..c8ae361 100644
*** a/src/test/regress/expected/tsrf.out
--- b/src/test/regress/expected/tsrf.out
*************** SELECT few.dataa, count(*) FROM few WHER
*** 193,201 ****
--- 193,205 ----
  -- SRFs are not allowed in aggregate arguments
  SELECT min(generate_series(1, 3)) FROM few;
  ERROR:  set-valued function called in context that cannot accept a set
+ LINE 1: SELECT min(generate_series(1, 3)) FROM few;
+                    ^
  -- SRFs are not allowed in window function arguments, either
  SELECT min(generate_series(1, 3)) OVER() FROM few;
  ERROR:  set-valued function called in context that cannot accept a set
+ LINE 1: SELECT min(generate_series(1, 3)) OVER() FROM few;
+                    ^
  -- SRFs are normally computed after window functions
  SELECT id,lag(id) OVER(), count(*) OVER(), generate_series(1,3) FROM few;
   id | lag | count | generate_series
*************** SELECT int4mul(generate_series(1,2), 10)
*** 424,429 ****
--- 428,435 ----
  -- but SRFs in function RTEs must be at top level (annoying restriction)
  SELECT * FROM int4mul(generate_series(1,2), 10);
  ERROR:  set-valued function called in context that cannot accept a set
+ LINE 1: SELECT * FROM int4mul(generate_series(1,2), 10);
+                               ^
  -- DISTINCT ON is evaluated before tSRF evaluation if SRF is not
  -- referenced either in ORDER BY or in the DISTINCT ON list. The ORDER
  -- BY reference can be implicitly generated, if there's no other ORDER BY.

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers