Provide an error cursor for "can't call an SRF here" errors.

Since it appears that v10 is going to move the goalposts by some amount
in terms of where you can and can't invoke set-returning functions,
arrange for the executor's "set-valued function called in context that
cannot accept a set" errors to include a syntax position if possible,
pointing to the specific SRF that can't be called where it's located.

The main bit of infrastructure needed for this is to make the query source
text accessible in the executor; but it turns out that commit 4c728f382
already did that.  We just need a new function executor_errposition()
modeled on parser_errposition(), and we're ready to rock.

While experimenting with this, I noted that the error position wasn't
properly reported if it occurred in a plpgsql FOR-over-query loop,
which turned out to be because SPI_cursor_open_internal wasn't providing
an error context callback during PortalStart.  Fix that.

There's a whole lot more that could be done with this infrastructure
now that it's there, but this is not the right time in the development
cycle for that sort of work.  Hence, resist the temptation to plaster
executor_errposition() calls everywhere ... for the moment.

Discussion: https://postgr.es/m/5263.1492471571@sss.pgh.pa.us

diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 15d693fac4d45929b441cf384a323afbc5dbbdca..5a34a46712d964a5249520d9891a168fd91d5fff 100644 (file)
--- a/src/backend/executor/execExpr.c
+++ b/src/backend/executor/execExpr.c
@@ -2103,7 +2103,9 @@ ExecInitFunc(ExprEvalStep *scratch, Expr *node, List *args, Oid funcid,
        if (flinfo->fn_retset)
                ereport(ERROR,
                                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                errmsg("set-valued function called in context that cannot accept a set")));
+                                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 4badd5c576fc12d07b7a2af3800c2cd7da3cf1ab..077ac208c13e3a6db2d6ed735961466ab27073ce 100644 (file)
--- a/src/backend/executor/execSRF.c
+++ b/src/backend/executor/execSRF.c
@@ -34,7 +34,8 @@
 
 
 /* static function decls */
-static void init_sexpr(Oid foid, Oid input_collation, SetExprState *sexpr,
+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,
@@ -77,7 +78,7 @@ ExecInitTableFunctionResult(Expr *expr,
                state->funcReturnsSet = func->funcretset;
                state->args = ExecInitExprList(func->args, parent);
 
-               init_sexpr(func->funcid, func->inputcollid, state,
+               init_sexpr(func->funcid, func->inputcollid, expr, state, parent,
                                   econtext->ecxt_per_query_memory, func->funcretset, false);
        }
        else
@@ -438,7 +439,7 @@ ExecInitFunctionResultSet(Expr *expr,
                FuncExpr   *func = (FuncExpr *) expr;
 
                state->args = ExecInitExprList(func->args, parent);
-               init_sexpr(func->funcid, func->inputcollid, state,
+               init_sexpr(func->funcid, func->inputcollid, expr, state, parent,
                                   econtext->ecxt_per_query_memory, true, true);
        }
        else if (IsA(expr, OpExpr))
@@ -446,7 +447,7 @@ ExecInitFunctionResultSet(Expr *expr,
                OpExpr     *op = (OpExpr *) expr;
 
                state->args = ExecInitExprList(op->args, parent);
-               init_sexpr(op->opfuncid, op->inputcollid, state,
+               init_sexpr(op->opfuncid, op->inputcollid, expr, state, parent,
                                   econtext->ecxt_per_query_memory, true, true);
        }
        else
@@ -645,7 +646,8 @@ restart:
  * init_sexpr - initialize a SetExprState node during first use
  */
 static void
-init_sexpr(Oid foid, Oid input_collation, SetExprState *sexpr,
+init_sexpr(Oid foid, Oid input_collation, Expr *node,
+                  SetExprState *sexpr, PlanState *parent,
                   MemoryContext sexprCxt, bool allowSRF, bool needDescForSRF)
 {
        AclResult       aclresult;
@@ -683,7 +685,9 @@ init_sexpr(Oid foid, Oid input_collation, SetExprState *sexpr,
        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")));
+                                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 df3d6503d149d27f6d06818c2ffc062274f60ea7..08229bd6a727a4fd19519e793fc76b2794934086 100644 (file)
--- a/src/backend/executor/execUtils.c
+++ b/src/backend/executor/execUtils.c
@@ -28,6 +28,8 @@
  *             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,6 +46,7 @@
 #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"
@@ -685,6 +688,36 @@ UpdateChangedParamSet(PlanState *node, Bitmapset *newchg)
                bms_free(parmset);
 }
 
+/*
+ * 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.
  *

diff --git a/src/backend/executor/spi.c b/src/backend/executor/spi.c
index a4a6e5bae437977b340858587faed1f4b20fdda4..35021e1839b4cbba04003ba0eb79901049cd6539 100644 (file)
--- a/src/backend/executor/spi.c
+++ b/src/backend/executor/spi.c
@@ -1197,9 +1197,6 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
        cplan = GetCachedPlan(plansource, paramLI, false, _SPI_current->queryEnv);
        stmt_list = cplan->stmt_list;
 
-       /* Pop the error context stack */
-       error_context_stack = spierrcontext.previous;
-
        if (!plan->saved)
        {
                /*
@@ -1318,6 +1315,9 @@ SPI_cursor_open_internal(const char *name, SPIPlanPtr plan,
 
        Assert(portal->strategy != PORTAL_MULTI_QUERY);
 
+       /* Pop the error context stack */
+       error_context_stack = spierrcontext.previous;
+
        /* Pop the SPI stack */
        _SPI_end_call(true);
 

diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index f7f3189a1a096d2ccefafe3ba9a2015c97d22c0d..3107cf5b89e9b481fd86ec60f9313340c3278187 100644 (file)
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -482,6 +482,8 @@ extern bool ExecRelationIsTargetRelation(EState *estate, Index scanrelid);
 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 33f370b49498f88d4ec6ffa5007a84f1e60d9942..c8ae361e75638e0a7fb1d3f1619834ad31beb555 100644 (file)
--- a/src/test/regress/expected/tsrf.out
+++ b/src/test/regress/expected/tsrf.out
@@ -193,9 +193,13 @@ SELECT few.dataa, count(*) FROM few WHERE dataa = 'a' GROUP BY few.dataa, unnest
 -- 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 
@@ -424,6 +428,8 @@ SELECT int4mul(generate_series(1,2), 10);
 -- 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.