Skip to content

Commit a4c35ea

Browse files
tglsfdctglsfdc
committed
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 eaccfde). 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 parent 445a38a commit a4c35ea

File tree

23 files changed

+225
-73
lines changed

23 files changed

+225
-73
lines changed

‎src/backend/catalog/heap.c

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2560,14 +2560,9 @@ cookDefault(ParseState *pstate,
25602560

25612561
/*
25622562
* transformExpr() should have already rejected subqueries, aggregates,
2563-
* and window functions, based on the EXPR_KIND_ for a default expression.
2564-
*
2565-
* It can't return a set either.
2563+
* window functions, and SRFs, based on the EXPR_KIND_ for a default
2564+
* expression.
25662565
*/
2567-
if (expression_returns_set(expr))
2568-
ereport(ERROR,
2569-
(errcode(ERRCODE_DATATYPE_MISMATCH),
2570-
errmsg("default expression must not return a set")));
25712566

25722567
/*
25732568
* Coerce the expression to the correct type and typmod, if given. This

‎src/backend/nodes/copyfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2731,6 +2731,7 @@ _copyQuery(const Query *from)
27312731
COPY_SCALAR_FIELD(resultRelation);
27322732
COPY_SCALAR_FIELD(hasAggs);
27332733
COPY_SCALAR_FIELD(hasWindowFuncs);
2734+
COPY_SCALAR_FIELD(hasTargetSRFs);
27342735
COPY_SCALAR_FIELD(hasSubLinks);
27352736
COPY_SCALAR_FIELD(hasDistinctOn);
27362737
COPY_SCALAR_FIELD(hasRecursive);

‎src/backend/nodes/equalfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -921,6 +921,7 @@ _equalQuery(const Query *a, const Query *b)
921921
COMPARE_SCALAR_FIELD(resultRelation);
922922
COMPARE_SCALAR_FIELD(hasAggs);
923923
COMPARE_SCALAR_FIELD(hasWindowFuncs);
924+
COMPARE_SCALAR_FIELD(hasTargetSRFs);
924925
COMPARE_SCALAR_FIELD(hasSubLinks);
925926
COMPARE_SCALAR_FIELD(hasDistinctOn);
926927
COMPARE_SCALAR_FIELD(hasRecursive);

‎src/backend/nodes/outfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2683,6 +2683,7 @@ _outQuery(StringInfo str, const Query *node)
26832683
WRITE_INT_FIELD(resultRelation);
26842684
WRITE_BOOL_FIELD(hasAggs);
26852685
WRITE_BOOL_FIELD(hasWindowFuncs);
2686+
WRITE_BOOL_FIELD(hasTargetSRFs);
26862687
WRITE_BOOL_FIELD(hasSubLinks);
26872688
WRITE_BOOL_FIELD(hasDistinctOn);
26882689
WRITE_BOOL_FIELD(hasRecursive);

‎src/backend/nodes/readfuncs.c

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,7 @@ _readQuery(void)
238238
READ_INT_FIELD(resultRelation);
239239
READ_BOOL_FIELD(hasAggs);
240240
READ_BOOL_FIELD(hasWindowFuncs);
241+
READ_BOOL_FIELD(hasTargetSRFs);
241242
READ_BOOL_FIELD(hasSubLinks);
242243
READ_BOOL_FIELD(hasDistinctOn);
243244
READ_BOOL_FIELD(hasRecursive);

‎src/backend/optimizer/path/allpaths.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2422,7 +2422,8 @@ check_output_expressions(Query *subquery, pushdown_safety_info *safetyInfo)
24222422
continue;
24232423

24242424
/* Functions returning sets are unsafe (point 1) */
2425-
if (expression_returns_set((Node *) tle->expr))
2425+
if (subquery->hasTargetSRFs &&
2426+
expression_returns_set((Node *) tle->expr))
24262427
{
24272428
safetyInfo->unsafeColumns[tle->resno] = true;
24282429
continue;
@@ -2835,7 +2836,8 @@ remove_unused_subquery_outputs(Query *subquery, RelOptInfo *rel)
28352836
* If it contains a set-returning function, we can't remove it since
28362837
* that could change the number of rows returned by the subquery.
28372838
*/
2838-
if (expression_returns_set(texpr))
2839+
if (subquery->hasTargetSRFs &&
2840+
expression_returns_set(texpr))
28392841
continue;
28402842

28412843
/*

‎src/backend/optimizer/plan/analyzejoins.c

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,11 @@ rel_is_distinct_for(PlannerInfo *root, RelOptInfo *rel, List *clause_list)
650650
bool
651651
query_supports_distinctness(Query *query)
652652
{
653+
/* we don't cope with SRFs, see comment below */
654+
if (query->hasTargetSRFs)
655+
return false;
656+
657+
/* check for features we can prove distinctness with */
653658
if (query->distinctClause != NIL ||
654659
query->groupClause != NIL ||
655660
query->groupingSets != NIL ||
@@ -695,7 +700,7 @@ query_is_distinct_for(Query *query, List *colnos, List *opids)
695700
* specified columns, since those must be evaluated before de-duplication;
696701
* but it doesn't presently seem worth the complication to check that.)
697702
*/
698-
if (expression_returns_set((Node *) query->targetList))
703+
if (query->hasTargetSRFs)
699704
return false;
700705

701706
/*

‎src/backend/optimizer/plan/planner.c

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -604,6 +604,10 @@ subquery_planner(PlannerGlobal *glob, Query *parse,
604604
preprocess_expression(root, (Node *) parse->targetList,
605605
EXPRKIND_TARGET);
606606

607+
/* Constant-folding might have removed all set-returning functions */
608+
if (parse->hasTargetSRFs)
609+
parse->hasTargetSRFs = expression_returns_set((Node *) parse->targetList);
610+
607611
newWithCheckOptions = NIL;
608612
foreach(l, parse->withCheckOptions)
609613
{
@@ -1702,16 +1706,14 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
17021706
* Figure out whether there's a hard limit on the number of rows that
17031707
* query_planner's result subplan needs to return. Even if we know a
17041708
* hard limit overall, it doesn't apply if the query has any
1705-
* grouping/aggregation operations. (XXX it also doesn't apply if the
1706-
* tlist contains any SRFs; but checking for that here seems more
1707-
* costly than it's worth, since root->limit_tuples is only used for
1708-
* cost estimates, and only in a small number of cases.)
1709+
* grouping/aggregation operations, or SRFs in the tlist.
17091710
*/
17101711
if (parse->groupClause ||
17111712
parse->groupingSets ||
17121713
parse->distinctClause ||
17131714
parse->hasAggs ||
17141715
parse->hasWindowFuncs ||
1716+
parse->hasTargetSRFs ||
17151717
root->hasHavingQual)
17161718
root->limit_tuples = -1.0;
17171719
else
@@ -1928,7 +1930,11 @@ grouping_planner(PlannerInfo *root, bool inheritance_update,
19281930
* weird usage that it doesn't seem worth greatly complicating matters to
19291931
* account for it.
19301932
*/
1931-
tlist_rows = tlist_returns_set_rows(tlist);
1933+
if (parse->hasTargetSRFs)
1934+
tlist_rows = tlist_returns_set_rows(tlist);
1935+
else
1936+
tlist_rows = 1;
1937+
19321938
if (tlist_rows > 1)
19331939
{
19341940
foreach(lc, current_rel->pathlist)
@@ -4995,7 +5001,8 @@ make_sort_input_target(PlannerInfo *root,
49955001
* Check for SRF or volatile functions. Check the SRF case first
49965002
* because we must know whether we have any postponed SRFs.
49975003
*/
4998-
if (expression_returns_set((Node *) expr))
5004+
if (parse->hasTargetSRFs &&
5005+
expression_returns_set((Node *) expr))
49995006
{
50005007
/* We'll decide below whether these are postponable */
50015008
col_is_srf[i] = true;
@@ -5034,6 +5041,7 @@ make_sort_input_target(PlannerInfo *root,
50345041
{
50355042
/* For sortgroupref cols, just check if any contain SRFs */
50365043
if (!have_srf_sortcols &&
5044+
parse->hasTargetSRFs &&
50375045
expression_returns_set((Node *) expr))
50385046
have_srf_sortcols = true;
50395047
}

‎src/backend/optimizer/plan/subselect.c

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1562,7 +1562,7 @@ simplify_EXISTS_query(PlannerInfo *root, Query *query)
15621562
{
15631563
/*
15641564
* We don't try to simplify at all if the query uses set operations,
1565-
* aggregates, grouping sets, modifying CTEs, HAVING, OFFSET, or FOR
1565+
* aggregates, grouping sets, SRFs, modifying CTEs, HAVING, OFFSET, or FOR
15661566
* UPDATE/SHARE; none of these seem likely in normal usage and their
15671567
* possible effects are complex. (Note: we could ignore an "OFFSET 0"
15681568
* clause, but that traditionally is used as an optimization fence, so we
@@ -1573,6 +1573,7 @@ simplify_EXISTS_query(PlannerInfo *root, Query *query)
15731573
query->hasAggs ||
15741574
query->groupingSets ||
15751575
query->hasWindowFuncs ||
1576+
query->hasTargetSRFs ||
15761577
query->hasModifyingCTE ||
15771578
query->havingQual ||
15781579
query->limitOffset ||
@@ -1613,13 +1614,6 @@ simplify_EXISTS_query(PlannerInfo *root, Query *query)
16131614
query->limitCount = NULL;
16141615
}
16151616

1616-
/*
1617-
* Mustn't throw away the targetlist if it contains set-returning
1618-
* functions; those could affect whether zero rows are returned!
1619-
*/
1620-
if (expression_returns_set((Node *) query->targetList))
1621-
return false;
1622-
16231617
/*
16241618
* Otherwise, we can throw away the targetlist, as well as any GROUP,
16251619
* WINDOW, DISTINCT, and ORDER BY clauses; none of those clauses will

‎src/backend/optimizer/prep/prepjointree.c

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1188,8 +1188,8 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
11881188
parse->hasSubLinks |= subquery->hasSubLinks;
11891189

11901190
/*
1191-
* subquery won't be pulled up if it hasAggs or hasWindowFuncs, so no work
1192-
* needed on those flags
1191+
* subquery won't be pulled up if it hasAggs, hasWindowFuncs, or
1192+
* hasTargetSRFs, so no work needed on those flags
11931193
*/
11941194

11951195
/*
@@ -1419,8 +1419,8 @@ is_simple_subquery(Query *subquery, RangeTblEntry *rte,
14191419
return false;
14201420

14211421
/*
1422-
* Can't pull up a subquery involving grouping, aggregation, sorting,
1423-
* limiting, or WITH. (XXX WITH could possibly be allowed later)
1422+
* Can't pull up a subquery involving grouping, aggregation, SRFs,
1423+
* sorting, limiting, or WITH. (XXX WITH could possibly be allowed later)
14241424
*
14251425
* We also don't pull up a subquery that has explicit FOR UPDATE/SHARE
14261426
* clauses, because pullup would cause the locking to occur semantically
@@ -1430,6 +1430,7 @@ is_simple_subquery(Query *subquery, RangeTblEntry *rte,
14301430
*/
14311431
if (subquery->hasAggs ||
14321432
subquery->hasWindowFuncs ||
1433+
subquery->hasTargetSRFs ||
14331434
subquery->groupClause ||
14341435
subquery->groupingSets ||
14351436
subquery->havingQual ||
@@ -1542,15 +1543,6 @@ is_simple_subquery(Query *subquery, RangeTblEntry *rte,
15421543
}
15431544
}
15441545

1545-
/*
1546-
* Don't pull up a subquery that has any set-returning functions in its
1547-
* targetlist. Otherwise we might well wind up inserting set-returning
1548-
* functions into places where they mustn't go, such as quals of higher
1549-
* queries. This also ensures deletion of an empty jointree is valid.
1550-
*/
1551-
if (expression_returns_set((Node *) subquery->targetList))
1552-
return false;
1553-
15541546
/*
15551547
* Don't pull up a subquery that has any volatile functions in its
15561548
* targetlist. Otherwise we might introduce multiple evaluations of these

0 commit comments

Comments
 (0)