Disallow set-returning functions inside CASE or COALESCE.

When we reimplemented SRFs in commit 69f4b9c85, our initial choice was
to allow the behavior to vary from historical practice in cases where a
SRF call appeared within a conditional-execution construct (currently,
only CASE or COALESCE).  But that was controversial to begin with, and
subsequent discussion has resulted in a consensus that it's better to
throw an error instead of executing the query differently from before,
so long as we can provide a reasonably clear error message and a way to
rewrite the query.

Hence, add a parser mechanism to allow detection of such cases during
parse analysis.  The mechanism just requires storing, in the ParseState,
a pointer to the set-returning FuncExpr or OpExpr most recently emitted
by parse analysis.  Then the parsing functions for CASE and COALESCE can
detect the presence of a SRF in their arguments by noting whether this
pointer changes while analyzing their arguments.  Furthermore, if it does,
it provides a suitable error cursor location for the complaint.  (This
means that if there's more than one SRF in the arguments, the error will
point at the last one to be analyzed not the first.  While connoisseurs of
parsing behavior might find that odd, it's unlikely the average user would
ever notice.)

While at it, we can also provide more specific error messages than before
about some pre-existing restrictions, such as no-SRFs-within-aggregates.
Also, reject at parse time cases where a NULLIF or IS DISTINCT FROM
construct would need to return a set.  We've never supported that, but the
restriction is depended on in more subtle ways now, so it seems wise to
detect it at the start.

Also, provide some documentation about how to rewrite a SRF-within-CASE
query using a custom wrapper SRF.

It turns out that the information_schema.user_mapping_options view
contained an instance of exactly the behavior we're now forbidding; but
rewriting it makes it more clear and safer too.

initdb forced because of user_mapping_options change.

Patch by me, with error message suggestions from Alvaro Herrera and
Andres Freund, pursuant to a complaint from Regina Obe.

Discussion: https://postgr.es/m/000001d2d5de$d8d66170$8a832450$@pcorp.us

7 files changed, 152 insertions, 37 deletions

diff --git a/src/backend/catalog/information_schema.sql b/src/backend/catalog/information_schema.sql
index cbcd6cfbc17..98bcfa08c65 100644
--- a/src/backend/catalog/information_schema.sql
+++ b/src/backend/catalog/information_schema.sql
@@ -2936,12 +2936,14 @@ CREATE VIEW user_mapping_options AS
     SELECT authorization_identifier,
            foreign_server_catalog,
            foreign_server_name,
-           CAST((pg_options_to_table(um.umoptions)).option_name AS sql_identifier) AS option_name,
+           CAST(opts.option_name AS sql_identifier) AS option_name,
            CAST(CASE WHEN (umuser <> 0 AND authorization_identifier = current_user)
                        OR (umuser = 0 AND pg_has_role(srvowner, 'USAGE'))
-                       OR (SELECT rolsuper FROM pg_authid WHERE rolname = current_user) THEN (pg_options_to_table(um.umoptions)).option_value
+                       OR (SELECT rolsuper FROM pg_authid WHERE rolname = current_user)
+                     THEN opts.option_value
                      ELSE NULL END AS character_data) AS option_value
-    FROM _pg_user_mappings um;
+    FROM _pg_user_mappings um,
+         pg_options_to_table(um.umoptions) opts;
 
 GRANT SELECT ON user_mapping_options TO PUBLIC;
 
diff --git a/src/backend/executor/functions.c b/src/backend/executor/functions.c
index a35ba32e6dd..89aea2fe5c5 100644
--- a/src/backend/executor/functions.c
+++ b/src/backend/executor/functions.c
@@ -388,6 +388,7 @@ sql_fn_post_column_ref(ParseState *pstate, ColumnRef *cref, Node *var)
 		param = ParseFuncOrColumn(pstate,
 								  list_make1(subfield),
 								  list_make1(param),
+								  pstate->p_last_srf,
 								  NULL,
 								  cref->location);
 	}
diff --git a/src/backend/parser/parse_agg.c b/src/backend/parser/parse_agg.c
index efe1c371efc..6218ee94fa4 100644
--- a/src/backend/parser/parse_agg.c
+++ b/src/backend/parser/parse_agg.c
@@ -705,6 +705,14 @@ check_agg_arguments_walker(Node *node,
 		}
 		/* Continue and descend into subtree */
 	}
+	/* We can throw error on sight for a set-returning function */
+	if ((IsA(node, FuncExpr) &&((FuncExpr *) node)->funcretset) ||
+		(IsA(node, OpExpr) &&((OpExpr *) node)->opretset))
+		ereport(ERROR,
+				(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+				 errmsg("aggregate function calls cannot contain set-returning function calls"),
+				 errhint("You might be able to move the set-returning function into a LATERAL FROM item."),
+				 parser_errposition(context->pstate, exprLocation(node))));
 	/* We can throw error on sight for a window function */
 	if (IsA(node, WindowFunc))
 		ereport(ERROR,
diff --git a/src/backend/parser/parse_clause.c b/src/backend/parser/parse_clause.c
index 27dd49d3019..3d5b20836f6 100644
--- a/src/backend/parser/parse_clause.c
+++ b/src/backend/parser/parse_clause.c
@@ -572,6 +572,8 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r)
 		List	   *pair = (List *) lfirst(lc);
 		Node	   *fexpr;
 		List	   *coldeflist;
+		Node	   *newfexpr;
+		Node	   *last_srf;
 
 		/* Disassemble the function-call/column-def-list pairs */
 		Assert(list_length(pair) == 2);
@@ -618,13 +620,25 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r)
 					Node	   *arg = (Node *) lfirst(lc);
 					FuncCall   *newfc;
 
+					last_srf = pstate->p_last_srf;
+
 					newfc = makeFuncCall(SystemFuncName("unnest"),
 										 list_make1(arg),
 										 fc->location);
 
-					funcexprs = lappend(funcexprs,
-										transformExpr(pstate, (Node *) newfc,
-												   EXPR_KIND_FROM_FUNCTION));
+					newfexpr = transformExpr(pstate, (Node *) newfc,
+											 EXPR_KIND_FROM_FUNCTION);
+
+					/* nodeFunctionscan.c requires SRFs to be at top level */
+					if (pstate->p_last_srf != last_srf &&
+						pstate->p_last_srf != newfexpr)
+						ereport(ERROR,
+								(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+								 errmsg("set-returning functions must appear at top level of FROM"),
+								 parser_errposition(pstate,
+										 exprLocation(pstate->p_last_srf))));
+
+					funcexprs = lappend(funcexprs, newfexpr);
 
 					funcnames = lappend(funcnames,
 										FigureColname((Node *) newfc));
@@ -638,9 +652,21 @@ transformRangeFunction(ParseState *pstate, RangeFunction *r)
 		}
 
 		/* normal case ... */
-		funcexprs = lappend(funcexprs,
-							transformExpr(pstate, fexpr,
-										  EXPR_KIND_FROM_FUNCTION));
+		last_srf = pstate->p_last_srf;
+
+		newfexpr = transformExpr(pstate, fexpr,
+								 EXPR_KIND_FROM_FUNCTION);
+
+		/* nodeFunctionscan.c requires SRFs to be at top level */
+		if (pstate->p_last_srf != last_srf &&
+			pstate->p_last_srf != newfexpr)
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("set-returning functions must appear at top level of FROM"),
+					 parser_errposition(pstate,
+										exprLocation(pstate->p_last_srf))));
+
+		funcexprs = lappend(funcexprs, newfexpr);
 
 		funcnames = lappend(funcnames,
 							FigureColname(fexpr));
diff --git a/src/backend/parser/parse_expr.c b/src/backend/parser/parse_expr.c
index 92101c9103d..65bd51e1fd6 100644
--- a/src/backend/parser/parse_expr.c
+++ b/src/backend/parser/parse_expr.c
@@ -118,8 +118,7 @@ static Node *transformCurrentOfExpr(ParseState *pstate, CurrentOfExpr *cexpr);
 static Node *transformColumnRef(ParseState *pstate, ColumnRef *cref);
 static Node *transformWholeRowRef(ParseState *pstate, RangeTblEntry *rte,
 					 int location);
-static Node *transformIndirection(ParseState *pstate, Node *basenode,
-					 List *indirection);
+static Node *transformIndirection(ParseState *pstate, A_Indirection *ind);
 static Node *transformTypeCast(ParseState *pstate, TypeCast *tc);
 static Node *transformCollateClause(ParseState *pstate, CollateClause *c);
 static Node *make_row_comparison_op(ParseState *pstate, List *opname,
@@ -192,14 +191,8 @@ transformExprRecurse(ParseState *pstate, Node *expr)
 			}
 
 		case T_A_Indirection:
-			{
-				A_Indirection *ind = (A_Indirection *) expr;
-
-				result = transformExprRecurse(pstate, ind->arg);
-				result = transformIndirection(pstate, result,
-											  ind->indirection);
-				break;
-			}
+			result = transformIndirection(pstate, (A_Indirection *) expr);
+			break;
 
 		case T_A_ArrayExpr:
 			result = transformArrayExpr(pstate, (A_ArrayExpr *) expr,
@@ -439,11 +432,12 @@ unknown_attribute(ParseState *pstate, Node *relref, char *attname,
 }
 
 static Node *
-transformIndirection(ParseState *pstate, Node *basenode, List *indirection)
+transformIndirection(ParseState *pstate, A_Indirection *ind)
 {
-	Node	   *result = basenode;
+	Node	   *last_srf = pstate->p_last_srf;
+	Node	   *result = transformExprRecurse(pstate, ind->arg);
 	List	   *subscripts = NIL;
-	int			location = exprLocation(basenode);
+	int			location = exprLocation(result);
 	ListCell   *i;
 
 	/*
@@ -451,7 +445,7 @@ transformIndirection(ParseState *pstate, Node *basenode, List *indirection)
 	 * subscripting.  Adjacent A_Indices nodes have to be treated as a single
 	 * multidimensional subscript operation.
 	 */
-	foreach(i, indirection)
+	foreach(i, ind->indirection)
 	{
 		Node	   *n = lfirst(i);
 
@@ -484,6 +478,7 @@ transformIndirection(ParseState *pstate, Node *basenode, List *indirection)
 			newresult = ParseFuncOrColumn(pstate,
 										  list_make1(n),
 										  list_make1(result),
+										  last_srf,
 										  NULL,
 										  location);
 			if (newresult == NULL)
@@ -632,6 +627,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 					node = ParseFuncOrColumn(pstate,
 											 list_make1(makeString(colname)),
 											 list_make1(node),
+											 pstate->p_last_srf,
 											 NULL,
 											 cref->location);
 				}
@@ -678,6 +674,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 					node = ParseFuncOrColumn(pstate,
 											 list_make1(makeString(colname)),
 											 list_make1(node),
+											 pstate->p_last_srf,
 											 NULL,
 											 cref->location);
 				}
@@ -737,6 +734,7 @@ transformColumnRef(ParseState *pstate, ColumnRef *cref)
 					node = ParseFuncOrColumn(pstate,
 											 list_make1(makeString(colname)),
 											 list_make1(node),
+											 pstate->p_last_srf,
 											 NULL,
 											 cref->location);
 				}
@@ -927,6 +925,8 @@ transformAExprOp(ParseState *pstate, A_Expr *a)
 	else
 	{
 		/* Ordinary scalar operator */
+		Node	   *last_srf = pstate->p_last_srf;
+
 		lexpr = transformExprRecurse(pstate, lexpr);
 		rexpr = transformExprRecurse(pstate, rexpr);
 
@@ -934,6 +934,7 @@ transformAExprOp(ParseState *pstate, A_Expr *a)
 								  a->name,
 								  lexpr,
 								  rexpr,
+								  last_srf,
 								  a->location);
 	}
 
@@ -1053,6 +1054,7 @@ transformAExprNullIf(ParseState *pstate, A_Expr *a)
 								a->name,
 								lexpr,
 								rexpr,
+								pstate->p_last_srf,
 								a->location);
 
 	/*
@@ -1063,6 +1065,12 @@ transformAExprNullIf(ParseState *pstate, A_Expr *a)
 				(errcode(ERRCODE_DATATYPE_MISMATCH),
 				 errmsg("NULLIF requires = operator to yield boolean"),
 				 parser_errposition(pstate, a->location)));
+	if (result->opretset)
+		ereport(ERROR,
+				(errcode(ERRCODE_DATATYPE_MISMATCH),
+		/* translator: %s is name of a SQL construct, eg NULLIF */
+				 errmsg("%s must not return a set", "NULLIF"),
+				 parser_errposition(pstate, a->location)));
 
 	/*
 	 * ... but the NullIfExpr will yield the first operand's type.
@@ -1266,6 +1274,7 @@ transformAExprIn(ParseState *pstate, A_Expr *a)
 								   a->name,
 								   copyObject(lexpr),
 								   rexpr,
+								   pstate->p_last_srf,
 								   a->location);
 		}
 
@@ -1430,6 +1439,7 @@ transformBoolExpr(ParseState *pstate, BoolExpr *a)
 static Node *
 transformFuncCall(ParseState *pstate, FuncCall *fn)
 {
+	Node	   *last_srf = pstate->p_last_srf;
 	List	   *targs;
 	ListCell   *args;
 
@@ -1465,6 +1475,7 @@ transformFuncCall(ParseState *pstate, FuncCall *fn)
 	return ParseFuncOrColumn(pstate,
 							 fn->funcname,
 							 targs,
+							 last_srf,
 							 fn,
 							 fn->location);
 }
@@ -1619,7 +1630,8 @@ transformMultiAssignRef(ParseState *pstate, MultiAssignRef *maref)
 static Node *
 transformCaseExpr(ParseState *pstate, CaseExpr *c)
 {
-	CaseExpr   *newc;
+	CaseExpr   *newc = makeNode(CaseExpr);
+	Node	   *last_srf = pstate->p_last_srf;
 	Node	   *arg;
 	CaseTestExpr *placeholder;
 	List	   *newargs;
@@ -1628,8 +1640,6 @@ transformCaseExpr(ParseState *pstate, CaseExpr *c)
 	Node	   *defresult;
 	Oid			ptype;
 
-	newc = makeNode(CaseExpr);
-
 	/* transform the test expression, if any */
 	arg = transformExprRecurse(pstate, (Node *) c->arg);
 
@@ -1741,6 +1751,17 @@ transformCaseExpr(ParseState *pstate, CaseExpr *c)
 								  "CASE/WHEN");
 	}
 
+	/* if any subexpression contained a SRF, complain */
+	if (pstate->p_last_srf != last_srf)
+		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",
+						"CASE"),
+				 errhint("You might be able to move the set-returning function into a LATERAL FROM item."),
+				 parser_errposition(pstate,
+									exprLocation(pstate->p_last_srf))));
+
 	newc->location = c->location;
 
 	return (Node *) newc;
@@ -2177,6 +2198,7 @@ static Node *
 transformCoalesceExpr(ParseState *pstate, CoalesceExpr *c)
 {
 	CoalesceExpr *newc = makeNode(CoalesceExpr);
+	Node	   *last_srf = pstate->p_last_srf;
 	List	   *newargs = NIL;
 	List	   *newcoercedargs = NIL;
 	ListCell   *args;
@@ -2205,6 +2227,17 @@ transformCoalesceExpr(ParseState *pstate, CoalesceExpr *c)
 		newcoercedargs = lappend(newcoercedargs, newe);
 	}
 
+	/* if any subexpression contained a SRF, complain */
+	if (pstate->p_last_srf != last_srf)
+		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",
+						"COALESCE"),
+				 errhint("You might be able to move the set-returning function into a LATERAL FROM item."),
+				 parser_errposition(pstate,
+									exprLocation(pstate->p_last_srf))));
+
 	newc->args = newcoercedargs;
 	newc->location = c->location;
 	return (Node *) newc;
@@ -2793,7 +2826,8 @@ make_row_comparison_op(ParseState *pstate, List *opname,
 		Node	   *rarg = (Node *) lfirst(r);
 		OpExpr	   *cmp;
 
-		cmp = castNode(OpExpr, make_op(pstate, opname, larg, rarg, location));
+		cmp = castNode(OpExpr, make_op(pstate, opname, larg, rarg,
+									   pstate->p_last_srf, location));
 
 		/*
 		 * We don't use coerce_to_boolean here because we insist on the
@@ -3000,12 +3034,19 @@ make_distinct_op(ParseState *pstate, List *opname, Node *ltree, Node *rtree,
 {
 	Expr	   *result;
 
-	result = make_op(pstate, opname, ltree, rtree, location);
+	result = make_op(pstate, opname, ltree, rtree,
+					 pstate->p_last_srf, location);
 	if (((OpExpr *) result)->opresulttype != BOOLOID)
 		ereport(ERROR,
 				(errcode(ERRCODE_DATATYPE_MISMATCH),
 			 errmsg("IS DISTINCT FROM requires = operator to yield boolean"),
 				 parser_errposition(pstate, location)));
+	if (((OpExpr *) result)->opretset)
+		ereport(ERROR,
+				(errcode(ERRCODE_DATATYPE_MISMATCH),
+		/* translator: %s is name of a SQL construct, eg NULLIF */
+				 errmsg("%s must not return a set", "IS DISTINCT FROM"),
+				 parser_errposition(pstate, location)));
 
 	/*
 	 * We rely on DistinctExpr and OpExpr being same struct
diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c
index 55853c20bb4..34f1cf82ee0 100644
--- a/src/backend/parser/parse_func.c
+++ b/src/backend/parser/parse_func.c
@@ -64,10 +64,14 @@ static Node *ParseComplexProjection(ParseState *pstate, char *funcname,
  *
  *	The argument expressions (in fargs) must have been transformed
  *	already.  However, nothing in *fn has been transformed.
+ *
+ *	last_srf should be a copy of pstate->p_last_srf from just before we
+ *	started transforming fargs.  If the caller knows that fargs couldn't
+ *	contain any SRF calls, last_srf can just be pstate->p_last_srf.
  */
 Node *
 ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
-				  FuncCall *fn, int location)
+				  Node *last_srf, FuncCall *fn, int location)
 {
 	bool		is_column = (fn == NULL);
 	List	   *agg_order = (fn ? fn->agg_order : NIL);
@@ -628,7 +632,7 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 
 	/* if it returns a set, check that's OK */
 	if (retset)
-		check_srf_call_placement(pstate, location);
+		check_srf_call_placement(pstate, last_srf, location);
 
 	/* build the appropriate output structure */
 	if (fdresult == FUNCDETAIL_NORMAL)
@@ -759,6 +763,17 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 					 errmsg("FILTER is not implemented for non-aggregate window functions"),
 					 parser_errposition(pstate, location)));
 
+		/*
+		 * Window functions can't either take or return sets
+		 */
+		if (pstate->p_last_srf != last_srf)
+			ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("window function calls cannot contain set-returning function calls"),
+					 errhint("You might be able to move the set-returning function into a LATERAL FROM item."),
+					 parser_errposition(pstate,
+										exprLocation(pstate->p_last_srf))));
+
 		if (retset)
 			ereport(ERROR,
 					(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
@@ -771,6 +786,10 @@ ParseFuncOrColumn(ParseState *pstate, List *funcname, List *fargs,
 		retval = (Node *) wfunc;
 	}
 
+	/* if it returns a set, remember it for error checks at higher levels */
+	if (retset)
+		pstate->p_last_srf = retval;
+
 	return retval;
 }
 
@@ -2083,9 +2102,13 @@ LookupAggWithArgs(ObjectWithArgs *agg, bool noError)
  *		and throw a nice error if not.
  *
  * A side-effect is to set pstate->p_hasTargetSRFs true if appropriate.
+ *
+ * last_srf should be a copy of pstate->p_last_srf from just before we
+ * started transforming the function's arguments.  This allows detection
+ * of whether the SRF's arguments contain any SRFs.
  */
 void
-check_srf_call_placement(ParseState *pstate, int location)
+check_srf_call_placement(ParseState *pstate, Node *last_srf, int location)
 {
 	const char *err;
 	bool		errkind;
@@ -2121,7 +2144,15 @@ check_srf_call_placement(ParseState *pstate, int location)
 			errkind = true;
 			break;
 		case EXPR_KIND_FROM_FUNCTION:
-			/* okay ... but we can't check nesting here */
+			/* okay, but we don't allow nested SRFs here */
+			/* errmsg is chosen to match transformRangeFunction() */
+			/* errposition should point to the inner SRF */
+			if (pstate->p_last_srf != last_srf)
+				ereport(ERROR,
+						(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+						 errmsg("set-returning functions must appear at top level of FROM"),
+						 parser_errposition(pstate,
+										 exprLocation(pstate->p_last_srf))));
 			break;
 		case EXPR_KIND_WHERE:
 			errkind = true;
@@ -2202,7 +2233,7 @@ check_srf_call_placement(ParseState *pstate, int location)
 			err = _("set-returning functions are not allowed in trigger WHEN conditions");
 			break;
 		case EXPR_KIND_PARTITION_EXPRESSION:
-			err = _("set-returning functions are not allowed in partition key expression");
+			err = _("set-returning functions are not allowed in partition key expressions");
 			break;
 
 			/*
diff --git a/src/backend/parser/parse_oper.c b/src/backend/parser/parse_oper.c
index e40b10d4f61..4b1db76e19f 100644
--- a/src/backend/parser/parse_oper.c
+++ b/src/backend/parser/parse_oper.c
@@ -735,12 +735,14 @@ op_error(ParseState *pstate, List *op, char oprkind,
  * Transform operator expression ensuring type compatibility.
  * This is where some type conversion happens.
  *
- * As with coerce_type, pstate may be NULL if no special unknown-Param
- * processing is wanted.
+ * last_srf should be a copy of pstate->p_last_srf from just before we
+ * started transforming the operator's arguments; this is used for nested-SRF
+ * detection.  If the caller will throw an error anyway for a set-returning
+ * expression, it's okay to cheat and just pass pstate->p_last_srf.
  */
 Expr *
 make_op(ParseState *pstate, List *opname, Node *ltree, Node *rtree,
-		int location)
+		Node *last_srf, int location)
 {
 	Oid			ltypeId,
 				rtypeId;
@@ -843,7 +845,11 @@ make_op(ParseState *pstate, List *opname, Node *ltree, Node *rtree,
 
 	/* if it returns a set, check that's OK */
 	if (result->opretset)
-		check_srf_call_placement(pstate, location);
+	{
+		check_srf_call_placement(pstate, last_srf, location);
+		/* ... and remember it for error checks at higher levels */
+		pstate->p_last_srf = (Node *) result;
+	}
 
 	ReleaseSysCache(tup);