From a4c35ea1c2f05dd5b56739fcd0dc36a4870ea0c0 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 13 Sep 2016 13:54:24 -0400 Subject: [PATCH] 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> --- src/backend/catalog/heap.c | 9 +- src/backend/nodes/copyfuncs.c | 1 + src/backend/nodes/equalfuncs.c | 1 + src/backend/nodes/outfuncs.c | 1 + src/backend/nodes/readfuncs.c | 1 + src/backend/optimizer/path/allpaths.c | 6 +- src/backend/optimizer/plan/analyzejoins.c | 7 +- src/backend/optimizer/plan/planner.c | 20 ++- src/backend/optimizer/plan/subselect.c | 10 +- src/backend/optimizer/prep/prepjointree.c | 18 +-- src/backend/optimizer/util/clauses.c | 17 +-- src/backend/parser/analyze.c | 7 +- src/backend/parser/parse_func.c | 148 ++++++++++++++++++++++ src/backend/parser/parse_oper.c | 4 + src/backend/parser/parse_utilcmd.c | 20 +-- src/backend/rewrite/rewriteHandler.c | 2 +- src/include/catalog/catversion.h | 2 +- src/include/nodes/parsenodes.h | 1 + src/include/parser/parse_func.h | 2 + src/include/parser/parse_node.h | 3 +- src/pl/plpgsql/src/pl_exec.c | 1 + src/test/regress/expected/tsrf.out | 13 +- src/test/regress/sql/tsrf.sql | 4 +- 23 files changed, 225 insertions(+), 73 deletions(-) diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c index e997b574ca..dbd609493f 100644 --- a/src/backend/catalog/heap.c +++ b/src/backend/catalog/heap.c @@ -2560,14 +2560,9 @@ cookDefault(ParseState *pstate, /* * transformExpr() should have already rejected subqueries, aggregates, - * and window functions, based on the EXPR_KIND_ for a default expression. - * - * It can't return a set either. + * window functions, and SRFs, based on the EXPR_KIND_ for a default + * expression. */ - if (expression_returns_set(expr)) - ereport(ERROR, - (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("default expression must not return a set"))); /* * Coerce the expression to the correct type and typmod, if given. This diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c index 4f39dad66b..71714bc1d6 100644 --- a/src/backend/nodes/copyfuncs.c +++ b/src/backend/nodes/copyfuncs.c @@ -2731,6 +2731,7 @@ _copyQuery(const Query *from) COPY_SCALAR_FIELD(resultRelation); COPY_SCALAR_FIELD(hasAggs); COPY_SCALAR_FIELD(hasWindowFuncs); + COPY_SCALAR_FIELD(hasTargetSRFs); COPY_SCALAR_FIELD(hasSubLinks); COPY_SCALAR_FIELD(hasDistinctOn); COPY_SCALAR_FIELD(hasRecursive); diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c index 4800165a91..29a090fc48 100644 --- a/src/backend/nodes/equalfuncs.c +++ b/src/backend/nodes/equalfuncs.c @@ -921,6 +921,7 @@ _equalQuery(const Query *a, const Query *b) COMPARE_SCALAR_FIELD(resultRelation); COMPARE_SCALAR_FIELD(hasAggs); COMPARE_SCALAR_FIELD(hasWindowFuncs); + COMPARE_SCALAR_FIELD(hasTargetSRFs); COMPARE_SCALAR_FIELD(hasSubLinks); COMPARE_SCALAR_FIELD(hasDistinctOn); COMPARE_SCALAR_FIELD(hasRecursive); diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c index 90fecb1338..7e092d700c 100644 --- a/src/backend/nodes/outfuncs.c +++ b/src/backend/nodes/outfuncs.c @@ -2683,6 +2683,7 @@ _outQuery(StringInfo str, const Query *node) WRITE_INT_FIELD(resultRelation); WRITE_BOOL_FIELD(hasAggs); WRITE_BOOL_FIELD(hasWindowFuncs); + WRITE_BOOL_FIELD(hasTargetSRFs); WRITE_BOOL_FIELD(hasSubLinks); WRITE_BOOL_FIELD(hasDistinctOn); WRITE_BOOL_FIELD(hasRecursive); diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c index 894a48fb4f..917e6c8a65 100644 --- a/src/backend/nodes/readfuncs.c +++ b/src/backend/nodes/readfuncs.c @@ -238,6 +238,7 @@ _readQuery(void) READ_INT_FIELD(resultRelation); READ_BOOL_FIELD(hasAggs); READ_BOOL_FIELD(hasWindowFuncs); + READ_BOOL_FIELD(hasTargetSRFs); READ_BOOL_FIELD(hasSubLinks); READ_BOOL_FIELD(hasDistinctOn); READ_BOOL_FIELD(hasRecursive); diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index 04264b4335..99b6bc8f5a 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -2422,7 +2422,8 @@ check_output_expressions(Query *subquery, pushdown_safety_info *safetyInfo) continue; /* Functions returning sets are unsafe (point 1) */ - if (expression_returns_set((Node *) tle->expr)) + if (subquery->hasTargetSRFs && + expression_returns_set((Node *) tle->expr)) { safetyInfo->unsafeColumns[tle->resno] = true; continue; @@ -2835,7 +2836,8 @@ remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel) * If it contains a set-returning function, we can't remove it since * that could change the number of rows returned by the subquery. */ - if (expression_returns_set(texpr)) + if (subquery->hasTargetSRFs && + expression_returns_set(texpr)) continue; /* diff --git a/src/backend/optimizer/plan/analyzejoins.c b/src/backend/optimizer/plan/analyzejoins.c index e28a8dc533..74e4245122 100644 --- a/src/backend/optimizer/plan/analyzejoins.c +++ b/src/backend/optimizer/plan/analyzejoins.c @@ -650,6 +650,11 @@ rel_is_distinct_for(PlannerInfo *root, RelOptInfo *rel, List *clause_list) bool query_supports_distinctness(Query *query) { + /* we don't cope with SRFs, see comment below */ + if (query->hasTargetSRFs) + return false; + + /* check for features we can prove distinctness with */ if (query->distinctClause != NIL || query->groupClause != NIL || query->groupingSets != NIL || @@ -695,7 +700,7 @@ query_is_distinct_for(Query *query, List *colnos, List *opids) * specified columns, since those must be evaluated before de-duplication; * but it doesn't presently seem worth the complication to check that.) */ - if (expression_returns_set((Node *) query->targetList)) + if (query->hasTargetSRFs) return false; /* diff --git a/src/backend/optimizer/plan/planner.c b/src/backend/optimizer/plan/planner.c index 174210be6c..f657ffc446 100644 --- a/src/backend/optimizer/plan/planner.c +++ b/src/backend/optimizer/plan/planner.c @@ -604,6 +604,10 @@ subquery_planner(PlannerGlobal *glob, Query *parse, preprocess_expression(root, (Node *) parse->targetList, EXPRKIND_TARGET); + /* Constant-folding might have removed all set-returning functions */ + if (parse->hasTargetSRFs) + parse->hasTargetSRFs = expression_returns_set((Node *) parse->targetList); + newWithCheckOptions = NIL; foreach(l, parse->withCheckOptions) { @@ -1702,16 +1706,14 @@ grouping_planner(PlannerInfo *root, bool inheritance_update, * Figure out whether there's a hard limit on the number of rows that * query_planner's result subplan needs to return. Even if we know a * hard limit overall, it doesn't apply if the query has any - * grouping/aggregation operations. (XXX it also doesn't apply if the - * tlist contains any SRFs; but checking for that here seems more - * costly than it's worth, since root->limit_tuples is only used for - * cost estimates, and only in a small number of cases.) + * grouping/aggregation operations, or SRFs in the tlist. */ if (parse->groupClause || parse->groupingSets || parse->distinctClause || parse->hasAggs || parse->hasWindowFuncs || + parse->hasTargetSRFs || root->hasHavingQual) root->limit_tuples = -1.0; else @@ -1928,7 +1930,11 @@ grouping_planner(PlannerInfo *root, bool inheritance_update, * weird usage that it doesn't seem worth greatly complicating matters to * account for it. */ - tlist_rows = tlist_returns_set_rows(tlist); + if (parse->hasTargetSRFs) + tlist_rows = tlist_returns_set_rows(tlist); + else + tlist_rows = 1; + if (tlist_rows > 1) { foreach(lc, current_rel->pathlist) @@ -4995,7 +5001,8 @@ make_sort_input_target(PlannerInfo *root, * Check for SRF or volatile functions. Check the SRF case first * because we must know whether we have any postponed SRFs. */ - if (expression_returns_set((Node *) expr)) + if (parse->hasTargetSRFs && + expression_returns_set((Node *) expr)) { /* We'll decide below whether these are postponable */ col_is_srf[i] = true; @@ -5034,6 +5041,7 @@ make_sort_input_target(PlannerInfo *root, { /* For sortgroupref cols, just check if any contain SRFs */ if (!have_srf_sortcols && + parse->hasTargetSRFs && expression_returns_set((Node *) expr)) have_srf_sortcols = true; } diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index 6edefb1138..263ba45f9f 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -1562,7 +1562,7 @@ simplify_EXISTS_query(PlannerInfo *root, Query *query) { /* * We don't try to simplify at all if the query uses set operations, - * aggregates, grouping sets, modifying CTEs, HAVING, OFFSET, or FOR + * aggregates, grouping sets, SRFs, modifying CTEs, HAVING, OFFSET, or FOR * UPDATE/SHARE; none of these seem likely in normal usage and their * possible effects are complex. (Note: we could ignore an "OFFSET 0" * clause, but that traditionally is used as an optimization fence, so we @@ -1573,6 +1573,7 @@ simplify_EXISTS_query(PlannerInfo *root, Query *query) query->hasAggs || query->groupingSets || query->hasWindowFuncs || + query->hasTargetSRFs || query->hasModifyingCTE || query->havingQual || query->limitOffset || @@ -1613,13 +1614,6 @@ simplify_EXISTS_query(PlannerInfo *root, Query *query) query->limitCount = NULL; } - /* - * Mustn't throw away the targetlist if it contains set-returning - * functions; those could affect whether zero rows are returned! - */ - if (expression_returns_set((Node *) query->targetList)) - return false; - /* * Otherwise, we can throw away the targetlist, as well as any GROUP, * WINDOW, DISTINCT, and ORDER BY clauses; none of those clauses will diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c index a334f15773..878db9b4ab 100644 --- a/src/backend/optimizer/prep/prepjointree.c +++ b/src/backend/optimizer/prep/prepjointree.c @@ -1188,8 +1188,8 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte, parse->hasSubLinks |= subquery->hasSubLinks; /* - * subquery won't be pulled up if it hasAggs or hasWindowFuncs, so no work - * needed on those flags + * subquery won't be pulled up if it hasAggs, hasWindowFuncs, or + * hasTargetSRFs, so no work needed on those flags */ /* @@ -1419,8 +1419,8 @@ is_simple_subquery(Query *subquery, RangeTblEntry *rte, return false; /* - * Can't pull up a subquery involving grouping, aggregation, sorting, - * limiting, or WITH. (XXX WITH could possibly be allowed later) + * Can't pull up a subquery involving grouping, aggregation, SRFs, + * sorting, limiting, or WITH. (XXX WITH could possibly be allowed later) * * We also don't pull up a subquery that has explicit FOR UPDATE/SHARE * clauses, because pullup would cause the locking to occur semantically @@ -1430,6 +1430,7 @@ is_simple_subquery(Query *subquery, RangeTblEntry *rte, */ if (subquery->hasAggs || subquery->hasWindowFuncs || + subquery->hasTargetSRFs || subquery->groupClause || subquery->groupingSets || subquery->havingQual || @@ -1542,15 +1543,6 @@ is_simple_subquery(Query *subquery, RangeTblEntry *rte, } } - /* - * Don't pull up a subquery that has any set-returning functions in its - * targetlist. Otherwise we might well wind up inserting set-returning - * functions into places where they mustn't go, such as quals of higher - * queries. This also ensures deletion of an empty jointree is valid. - */ - if (expression_returns_set((Node *) subquery->targetList)) - return false; - /* * Don't pull up a subquery that has any volatile functions in its * targetlist. Otherwise we might introduce multiple evaluations of these diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c index e1baf71e38..663ffe0535 100644 --- a/src/backend/optimizer/util/clauses.c +++ b/src/backend/optimizer/util/clauses.c @@ -4449,6 +4449,7 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid, querytree->utilityStmt || querytree->hasAggs || querytree->hasWindowFuncs || + querytree->hasTargetSRFs || querytree->hasSubLinks || querytree->cteList || querytree->rtable || @@ -4489,17 +4490,13 @@ inline_function(Oid funcid, Oid result_type, Oid result_collid, Assert(!modifyTargetList); /* - * Additional validity checks on the expression. It mustn't return a set, - * and it mustn't be more volatile than the surrounding function (this is - * to avoid breaking hacks that involve pretending a function is immutable - * when it really ain't). If the surrounding function is declared strict, - * then the expression must contain only strict constructs and must use - * all of the function parameters (this is overkill, but an exact analysis - * is hard). + * Additional validity checks on the expression. It mustn't be more + * volatile than the surrounding function (this is to avoid breaking hacks + * that involve pretending a function is immutable when it really ain't). + * If the surrounding function is declared strict, then the expression + * must contain only strict constructs and must use all of the function + * parameters (this is overkill, but an exact analysis is hard). */ - if (expression_returns_set(newexpr)) - goto fail; - if (funcform->provolatile == PROVOLATILE_IMMUTABLE && contain_mutable_functions(newexpr)) goto fail; diff --git a/src/backend/parser/analyze.c b/src/backend/parser/analyze.c index eac86cce3e..870fae3f51 100644 --- a/src/backend/parser/analyze.c +++ b/src/backend/parser/analyze.c @@ -417,6 +417,7 @@ transformDeleteStmt(ParseState *pstate, DeleteStmt *stmt) qry->hasSubLinks = pstate->p_hasSubLinks; qry->hasWindowFuncs = pstate->p_hasWindowFuncs; + qry->hasTargetSRFs = pstate->p_hasTargetSRFs; qry->hasAggs = pstate->p_hasAggs; if (pstate->p_hasAggs) parseCheckAggregates(pstate, qry); @@ -819,6 +820,7 @@ transformInsertStmt(ParseState *pstate, InsertStmt *stmt) qry->rtable = pstate->p_rtable; qry->jointree = makeFromExpr(pstate->p_joinlist, NULL); + qry->hasTargetSRFs = pstate->p_hasTargetSRFs; qry->hasSubLinks = pstate->p_hasSubLinks; assign_query_collations(pstate, qry); @@ -1231,6 +1233,7 @@ transformSelectStmt(ParseState *pstate, SelectStmt *stmt) qry->hasSubLinks = pstate->p_hasSubLinks; qry->hasWindowFuncs = pstate->p_hasWindowFuncs; + qry->hasTargetSRFs = pstate->p_hasTargetSRFs; qry->hasAggs = pstate->p_hasAggs; if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual) parseCheckAggregates(pstate, qry); @@ -1691,6 +1694,7 @@ transformSetOperationStmt(ParseState *pstate, SelectStmt *stmt) qry->hasSubLinks = pstate->p_hasSubLinks; qry->hasWindowFuncs = pstate->p_hasWindowFuncs; + qry->hasTargetSRFs = pstate->p_hasTargetSRFs; qry->hasAggs = pstate->p_hasAggs; if (pstate->p_hasAggs || qry->groupClause || qry->groupingSets || qry->havingQual) parseCheckAggregates(pstate, qry); @@ -2170,6 +2174,7 @@ transformUpdateStmt(ParseState *pstate, UpdateStmt *stmt) qry->rtable = pstate->p_rtable; qry->jointree = makeFromExpr(pstate->p_joinlist, qual); + qry->hasTargetSRFs = pstate->p_hasTargetSRFs; qry->hasSubLinks = pstate->p_hasSubLinks; assign_query_collations(pstate, qry); @@ -2565,7 +2570,7 @@ CheckSelectLocking(Query *qry, LockClauseStrength strength) translator: %s is a SQL row locking clause such as FOR UPDATE */ errmsg("%s is not allowed with window functions", LCS_asString(strength)))); - if (expression_returns_set((Node *) qry->targetList)) + if (qry->hasTargetSRFs) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), /*------ diff --git a/src/backend/parser/parse_func.c b/src/backend/parser/parse_func.c index 61af484fee..56c9a4293d 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))); +} diff --git a/src/backend/parser/parse_oper.c b/src/backend/parser/parse_oper.c index e913d05a79..aecda6d933 100644 --- a/src/backend/parser/parse_oper.c +++ b/src/backend/parser/parse_oper.c @@ -839,6 +839,10 @@ make_op(ParseState *pstate, List *opname, Node *ltree, Node *rtree, result->args = args; result->location = location; + /* if it returns a set, check that's OK */ + if (result->opretset) + check_srf_call_placement(pstate, location); + ReleaseSysCache(tup); return (Expr *) result; diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c index 7a2950e6a9..0670bc2482 100644 --- a/src/backend/parser/parse_utilcmd.c +++ b/src/backend/parser/parse_utilcmd.c @@ -294,7 +294,7 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString) * overridden if an inherited table has oids. */ stmt->options = lcons(makeDefElem("oids", - (Node *) makeInteger(cxt.hasoids), -1), + (Node *) makeInteger(cxt.hasoids), -1), stmt->options); } @@ -483,7 +483,7 @@ transformColumnDefinition(CreateStmtContext *cxt, ColumnDef *column) makeString(cxt->relation->relname), makeString(column->colname)); altseqstmt->options = list_make1(makeDefElem("owned_by", - (Node *) attnamelist, -1)); + (Node *) attnamelist, -1)); cxt->alist = lappend(cxt->alist, altseqstmt); @@ -2106,17 +2106,11 @@ transformIndexStmt(Oid relid, IndexStmt *stmt, const char *queryString) /* * transformExpr() should have already rejected subqueries, - * aggregates, and window functions, based on the EXPR_KIND_ for - * an index expression. + * aggregates, window functions, and SRFs, based on the EXPR_KIND_ + * for an index expression. * - * Also reject expressions returning sets; this is for consistency - * with what transformWhereClause() checks for the predicate. * DefineIndex() will make more checks. */ - if (expression_returns_set(ielem->expr)) - ereport(ERROR, - (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("index expression cannot return a set"))); } } @@ -2594,12 +2588,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, def->cooked_default = transformExpr(pstate, def->raw_default, EXPR_KIND_ALTER_COL_TRANSFORM); - - /* it can't return a set */ - if (expression_returns_set(def->cooked_default)) - ereport(ERROR, - (errcode(ERRCODE_DATATYPE_MISMATCH), - errmsg("transform expression must not return a set"))); } newcmds = lappend(newcmds, cmd); diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index a22a11e2c1..b828e3cb07 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -2221,7 +2221,7 @@ view_query_is_auto_updatable(Query *viewquery, bool check_cols) if (viewquery->hasWindowFuncs) return gettext_noop("Views that return window functions are not automatically updatable."); - if (expression_returns_set((Node *) viewquery->targetList)) + if (viewquery->hasTargetSRFs) return gettext_noop("Views that return set-returning functions are not automatically updatable."); /* diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index c04edadbf0..ef691c5721 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -53,6 +53,6 @@ */ /* yyyymmddN */ -#define CATALOG_VERSION_NO 201608231 +#define CATALOG_VERSION_NO 201609131 #endif diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h index 8d3dcf4d4c..6de2cab6b2 100644 --- a/src/include/nodes/parsenodes.h +++ b/src/include/nodes/parsenodes.h @@ -116,6 +116,7 @@ typedef struct Query bool hasAggs; /* has aggregates in tlist or havingQual */ bool hasWindowFuncs; /* has window functions in tlist */ + bool hasTargetSRFs; /* has set-returning functions in tlist */ bool hasSubLinks; /* has subquery SubLink */ bool hasDistinctOn; /* distinctClause is from DISTINCT ON */ bool hasRecursive; /* WITH RECURSIVE was specified */ diff --git a/src/include/parser/parse_func.h b/src/include/parser/parse_func.h index 0cefdf1b60..ed16d36982 100644 --- a/src/include/parser/parse_func.h +++ b/src/include/parser/parse_func.h @@ -67,4 +67,6 @@ extern Oid LookupFuncNameTypeNames(List *funcname, List *argtypes, extern Oid LookupAggNameTypeNames(List *aggname, List *argtypes, bool noError); +extern void check_srf_call_placement(ParseState *pstate, int location); + #endif /* PARSE_FUNC_H */ diff --git a/src/include/parser/parse_node.h b/src/include/parser/parse_node.h index e3e359c021..66335863db 100644 --- a/src/include/parser/parse_node.h +++ b/src/include/parser/parse_node.h @@ -27,7 +27,7 @@ * by extension code that might need to call transformExpr(). The core code * will not enforce any context-driven restrictions on EXPR_KIND_OTHER * expressions, so the caller would have to check for sub-selects, aggregates, - * and window functions if those need to be disallowed. + * window functions, SRFs, etc if those need to be disallowed. */ typedef enum ParseExprKind { @@ -150,6 +150,7 @@ struct ParseState Node *p_value_substitute; /* what to replace VALUE with, if any */ bool p_hasAggs; bool p_hasWindowFuncs; + bool p_hasTargetSRFs; bool p_hasSubLinks; bool p_hasModifyingCTE; bool p_is_insert; diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index 6141b7ab49..470cf935df 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -6799,6 +6799,7 @@ exec_simple_check_plan(PLpgSQL_execstate *estate, PLpgSQL_expr *expr) */ if (query->hasAggs || query->hasWindowFuncs || + query->hasTargetSRFs || query->hasSubLinks || query->hasForUpdate || query->cteList || diff --git a/src/test/regress/expected/tsrf.out b/src/test/regress/expected/tsrf.out index 805e8db9f6..622f75517a 100644 --- a/src/test/regress/expected/tsrf.out +++ b/src/test/regress/expected/tsrf.out @@ -359,15 +359,20 @@ SELECT * FROM fewmore; 5 (5 rows) --- nonsense that seems to be allowed +-- SRFs are not allowed in UPDATE (they once were, but it was nonsense) UPDATE fewmore SET data = generate_series(4,9); +ERROR: set-returning functions are not allowed in UPDATE +LINE 1: UPDATE fewmore SET data = generate_series(4,9); + ^ -- SRFs are not allowed in RETURNING INSERT INTO fewmore VALUES(1) RETURNING generate_series(1,3); -ERROR: set-valued function called in context that cannot accept a set +ERROR: set-returning functions are not allowed in RETURNING +LINE 1: INSERT INTO fewmore VALUES(1) RETURNING generate_series(1,3)... + ^ -- nor aggregate arguments SELECT count(generate_series(1,3)) FROM few; ERROR: set-valued function called in context that cannot accept a set --- nor proper VALUES +-- nor standalone VALUES (but surely this is a bug?) VALUES(1, generate_series(1,2)); ERROR: set-valued function called in context that cannot accept a set -- DISTINCT ON is evaluated before tSRF evaluation if SRF is not @@ -457,7 +462,7 @@ SELECT a, generate_series(1,2) FROM (VALUES(1),(2),(3)) r(a) LIMIT 2 OFFSET 2; -- SRFs are not allowed in LIMIT. SELECT 1 LIMIT generate_series(1,3); -ERROR: argument of LIMIT must not return a set +ERROR: set-returning functions are not allowed in LIMIT LINE 1: SELECT 1 LIMIT generate_series(1,3); ^ -- tSRF in correlated subquery, referencing table outside diff --git a/src/test/regress/sql/tsrf.sql b/src/test/regress/sql/tsrf.sql index 524779581d..c28dd017e5 100644 --- a/src/test/regress/sql/tsrf.sql +++ b/src/test/regress/sql/tsrf.sql @@ -68,14 +68,14 @@ CREATE TABLE fewmore AS SELECT generate_series(1,3) AS data; INSERT INTO fewmore VALUES(generate_series(4,5)); SELECT * FROM fewmore; --- nonsense that seems to be allowed +-- SRFs are not allowed in UPDATE (they once were, but it was nonsense) UPDATE fewmore SET data = generate_series(4,9); -- SRFs are not allowed in RETURNING INSERT INTO fewmore VALUES(1) RETURNING generate_series(1,3); -- nor aggregate arguments SELECT count(generate_series(1,3)) FROM few; --- nor proper VALUES +-- nor standalone VALUES (but surely this is a bug?) VALUES(1, generate_series(1,2)); -- DISTINCT ON is evaluated before tSRF evaluation if SRF is not -- 2.40.0