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

tglsfdc · tglsfdc · commit e240a65c7dfc · 2017-04-18T13:21:08.000-04:00
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 4c728f3 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
@@ -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
@@ -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,15 +439,15 @@ 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))
 	{
 		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 @@ ExecMakeFunctionResultSet(SetExprState *fcache,
  * 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
@@ -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
@@ -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
@@ -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
@@ -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.
