Improve parser's and planner's handling of set-returning functions.

Teach the parser to reject misplaced set-returning functions during parse
analysis using p_expr_kind, in much the same way as we do for aggregates
and window functions (cf commit eaccfded9).  While this isn't complete
(it misses nesting-based restrictions), it's much better than the previous
error reporting for such cases, and it allows elimination of assorted
ad-hoc expression_returns_set() error checks.  We could add nesting checks
later if it seems important to catch all cases at parse time.

There is one case the parser will now throw error for although previous
versions allowed it, which is SRFs in the tlist of an UPDATE.  That never
behaved sensibly (since it's ill-defined which generated row should be
used to perform the update) and it's hard to see why it should not be
treated as an error.  It's a release-note-worthy change though.

Also, add a new Query field hasTargetSRFs reporting whether there are
any SRFs in the targetlist (including GROUP BY/ORDER BY expressions).
The parser can now set that basically for free during parse analysis,
and we can use it in a number of places to avoid expression_returns_set
searches.  (There will be more such checks soon.)  In some places, this
allows decontorting the logic since it's no longer expensive to check for
SRFs in the tlist --- so I made the checks parallel to the handling of
hasAggs/hasWindowFuncs wherever it seemed appropriate.

catversion bump because adding a Query field changes stored rules.

Andres Freund and Tom Lane

Discussion: <24639.1473782855@sss.pgh.pa.us>

1 files changed, 148 insertions, 0 deletions

diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 61af484feeb..56c9a4293df 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -25,6 +25,7 @@
 #include "parser/parse_agg.h"
 #include "parser/parse_clause.h"
 #include "parser/parse_coerce.h"
+#include "parser/parse_expr.h"
 #include "parser/parse_func.h"
 #include "parser/parse_relation.h"
 #include "parser/parse_target.h"
@@ -625,6 +626,10 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 									  exprLocation((Node *) llast(fargs)))));
 	}
 
+	/* if it returns a set, check that's OK */
+	if (retset)
+		check_srf_call_placement(pstate, location);
+
 	/* build the appropriate output structure */
 	if (fdresult == FUNCDETAIL_NORMAL)
 	{
@@ -2040,3 +2045,146 @@ LookupAggNameTypeNames(List *aggname, List *argtypes, bool noError)
 
 	return oid;
 }
+
+
+/*
+ * check_srf_call_placement
+ *		Verify that a set-returning function is called in a valid place,
+ *		and throw a nice error if not.
+ *
+ * A side-effect is to set pstate->p_hasTargetSRFs true if appropriate.
+ */
+void
+check_srf_call_placement(ParseState *pstate, int location)
+{
+	const char *err;
+	bool		errkind;
+
+	/*
+	 * Check to see if the set-returning function is in an invalid place
+	 * within the query.  Basically, we don't allow SRFs anywhere except in
+	 * the targetlist (which includes GROUP BY/ORDER BY expressions), VALUES,
+	 * and functions in FROM.
+	 *
+	 * For brevity we support two schemes for reporting an error here: set
+	 * "err" to a custom message, or set "errkind" true if the error context
+	 * is sufficiently identified by what ParseExprKindName will return, *and*
+	 * what it will return is just a SQL keyword.  (Otherwise, use a custom
+	 * message to avoid creating translation problems.)
+	 */
+	err = NULL;
+	errkind = false;
+	switch (pstate->p_expr_kind)
+	{
+		case EXPR_KIND_NONE:
+			Assert(false);		/* can't happen */
+			break;
+		case EXPR_KIND_OTHER:
+			/* Accept SRF here; caller must throw error if wanted */
+			break;
+		case EXPR_KIND_JOIN_ON:
+		case EXPR_KIND_JOIN_USING:
+			err = _("set-returning functions are not allowed in JOIN conditions");
+			break;
+		case EXPR_KIND_FROM_SUBSELECT:
+			/* can't get here, but just in case, throw an error */
+			errkind = true;
+			break;
+		case EXPR_KIND_FROM_FUNCTION:
+			/* okay ... but we can't check nesting here */
+			break;
+		case EXPR_KIND_WHERE:
+			errkind = true;
+			break;
+		case EXPR_KIND_POLICY:
+			err = _("set-returning functions are not allowed in policy expressions");
+			break;
+		case EXPR_KIND_HAVING:
+			errkind = true;
+			break;
+		case EXPR_KIND_FILTER:
+			errkind = true;
+			break;
+		case EXPR_KIND_WINDOW_PARTITION:
+		case EXPR_KIND_WINDOW_ORDER:
+			/* okay, these are effectively GROUP BY/ORDER BY */
+			pstate->p_hasTargetSRFs = true;
+			break;
+		case EXPR_KIND_WINDOW_FRAME_RANGE:
+		case EXPR_KIND_WINDOW_FRAME_ROWS:
+			err = _("set-returning functions are not allowed in window definitions");
+			break;
+		case EXPR_KIND_SELECT_TARGET:
+		case EXPR_KIND_INSERT_TARGET:
+			/* okay */
+			pstate->p_hasTargetSRFs = true;
+			break;
+		case EXPR_KIND_UPDATE_SOURCE:
+		case EXPR_KIND_UPDATE_TARGET:
+			/* disallowed because it would be ambiguous what to do */
+			errkind = true;
+			break;
+		case EXPR_KIND_GROUP_BY:
+		case EXPR_KIND_ORDER_BY:
+			/* okay */
+			pstate->p_hasTargetSRFs = true;
+			break;
+		case EXPR_KIND_DISTINCT_ON:
+			/* okay */
+			pstate->p_hasTargetSRFs = true;
+			break;
+		case EXPR_KIND_LIMIT:
+		case EXPR_KIND_OFFSET:
+			errkind = true;
+			break;
+		case EXPR_KIND_RETURNING:
+			errkind = true;
+			break;
+		case EXPR_KIND_VALUES:
+			/* okay */
+			break;
+		case EXPR_KIND_CHECK_CONSTRAINT:
+		case EXPR_KIND_DOMAIN_CHECK:
+			err = _("set-returning functions are not allowed in check constraints");
+			break;
+		case EXPR_KIND_COLUMN_DEFAULT:
+		case EXPR_KIND_FUNCTION_DEFAULT:
+			err = _("set-returning functions are not allowed in DEFAULT expressions");
+			break;
+		case EXPR_KIND_INDEX_EXPRESSION:
+			err = _("set-returning functions are not allowed in index expressions");
+			break;
+		case EXPR_KIND_INDEX_PREDICATE:
+			err = _("set-returning functions are not allowed in index predicates");
+			break;
+		case EXPR_KIND_ALTER_COL_TRANSFORM:
+			err = _("set-returning functions are not allowed in transform expressions");
+			break;
+		case EXPR_KIND_EXECUTE_PARAMETER:
+			err = _("set-returning functions are not allowed in EXECUTE parameters");
+			break;
+		case EXPR_KIND_TRIGGER_WHEN:
+			err = _("set-returning functions are not allowed in trigger WHEN conditions");
+			break;
+
+			/*
+			 * There is intentionally no default: case here, so that the
+			 * compiler will warn if we add a new ParseExprKind without
+			 * extending this switch.  If we do see an unrecognized value at
+			 * runtime, the behavior will be the same as for EXPR_KIND_OTHER,
+			 * which is sane anyway.
+			 */
+	}
+	if (err)
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg_internal("%s", err),
+				 parser_errposition(pstate, location)));
+	if (errkind)
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+		/* translator: %s is name of a SQL construct, eg GROUP BY */
+				 errmsg("set-returning functions are not allowed in %s",
+						ParseExprKindName(pstate->p_expr_kind)),
+				 parser_errposition(pstate, location)));
+}