From e240a65c7dfc5ad80ab757ecb1aa9b9032c7f8ae Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Tue, 18 Apr 2017 13:20:59 -0400
Subject: [PATCH] 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
---
src/backend/executor/execExpr.c | 4 +++-
src/backend/executor/execSRF.c | 16 +++++++++------
src/backend/executor/execUtils.c | 33 ++++++++++++++++++++++++++++++
src/backend/executor/spi.c | 6 +++---
src/include/executor/executor.h | 2 ++
src/test/regress/expected/tsrf.out | 6 ++++++
6 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/src/backend/executor/execExpr.c b/src/backend/executor/execExpr.c
index 15d693fac4d..5a34a46712d 100644
--- 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 4badd5c576f..077ac208c13 100644
--- 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 df3d6503d14..08229bd6a72 100644
--- 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 a4a6e5bae43..35021e1839b 100644
--- 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 f7f3189a1a0..3107cf5b89e 100644
--- 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 33f370b4949..c8ae361e756 100644
--- 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.
--
GitLab