Skip to content

Commit c1d9579

Browse files
tglsfdctglsfdc
committed
Avoid listing ungrouped Vars in the targetlist of Agg-underneath-Window.
Regular aggregate functions in combination with, or within the arguments of, window functions are OK per spec; they have the semantics that the aggregate output rows are computed and then we run the window functions over that row set. (Thus, this combination is not really useful unless there's a GROUP BY so that more than one aggregate output row is possible.) The case without GROUP BY could fail, as recently reported by Jeff Davis, because sloppy construction of the Agg node's targetlist resulted in extra references to possibly-ungrouped Vars appearing outside the aggregate function calls themselves. See the added regression test case for an example. Fixing this requires modifying the API of flatten_tlist and its underlying function pull_var_clause. I chose to make pull_var_clause's API for aggregates identical to what it was already doing for placeholders, since the useful behaviors turn out to be the same (error, report node as-is, or recurse into it). I also tightened the error checking in this area a bit: if it was ever valid to see an uplevel Var, Aggref, or PlaceHolderVar here, that was a long time ago, so complain instead of ignoring them. Backpatch into 9.1. The failure exists in 8.4 and 9.0 as well, but seeing that it only occurs in a basically-useless corner case, it doesn't seem worth the risks of changing a function API in a minor release. There might be third-party code using pull_var_clause.
1 parent 846af54 commit c1d9579

File tree

18 files changed

+119
-78
lines changed

18 files changed

+119
-78
lines changed

‎src/backend/catalog/heap.c

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1874,7 +1874,9 @@ StoreRelCheck(Relation rel, char *ccname, Node *expr,
18741874
* in check constraints; it would fail to examine the contents of
18751875
* subselects.
18761876
*/
1877-
varList = pull_var_clause(expr, PVC_REJECT_PLACEHOLDERS);
1877+
varList = pull_var_clause(expr,
1878+
PVC_REJECT_AGGREGATES,
1879+
PVC_REJECT_PLACEHOLDERS);
18781880
keycount = list_length(varList);
18791881

18801882
if (keycount > 0)
@@ -2171,7 +2173,9 @@ AddRelationNewConstraints(Relation rel,
21712173
List *vars;
21722174
char *colname;
21732175

2174-
vars = pull_var_clause(expr, PVC_REJECT_PLACEHOLDERS);
2176+
vars = pull_var_clause(expr,
2177+
PVC_REJECT_AGGREGATES,
2178+
PVC_REJECT_PLACEHOLDERS);
21752179

21762180
/* eliminate duplicates */
21772181
vars = list_union(NIL, vars);

‎src/backend/commands/trigger.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -313,7 +313,9 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
313313
* subselects in WHEN clauses; it would fail to examine the contents
314314
* of subselects.
315315
*/
316-
varList = pull_var_clause(whenClause, PVC_REJECT_PLACEHOLDERS);
316+
varList = pull_var_clause(whenClause,
317+
PVC_REJECT_AGGREGATES,
318+
PVC_REJECT_PLACEHOLDERS);
317319
foreach(lc, varList)
318320
{
319321
Var *var = (Var *) lfirst(lc);

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1304,15 +1304,18 @@ qual_is_pushdown_safe(Query *subquery, Index rti, Node *qual,
13041304

13051305
/*
13061306
* It would be unsafe to push down window function calls, but at least for
1307-
* the moment we could never see any in a qual anyhow.
1307+
* the moment we could never see any in a qual anyhow. (The same applies
1308+
* to aggregates, which we check for in pull_var_clause below.)
13081309
*/
13091310
Assert(!contain_window_function(qual));
13101311

13111312
/*
13121313
* Examine all Vars used in clause; since it's a restriction clause, all
13131314
* such Vars must refer to subselect output columns.
13141315
*/
1315-
vars = pull_var_clause(qual, PVC_INCLUDE_PLACEHOLDERS);
1316+
vars = pull_var_clause(qual,
1317+
PVC_REJECT_AGGREGATES,
1318+
PVC_INCLUDE_PLACEHOLDERS);
13161319
foreach(vl, vars)
13171320
{
13181321
Var *var = (Var *) lfirst(vl);

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -847,6 +847,7 @@ generate_base_implied_equalities_no_const(PlannerInfo *root,
847847
{
848848
EquivalenceMember *cur_em = (EquivalenceMember *) lfirst(lc);
849849
List *vars = pull_var_clause((Node *) cur_em->em_expr,
850+
PVC_RECURSE_AGGREGATES,
850851
PVC_INCLUDE_PLACEHOLDERS);
851852

852853
add_vars_to_targetlist(root, vars, ec->ec_relids);

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3552,6 +3552,7 @@ prepare_sort_from_pathkeys(PlannerInfo *root, Plan *lefttree, List *pathkeys,
35523552
continue;
35533553
sortexpr = em->em_expr;
35543554
exprvars = pull_var_clause((Node *) sortexpr,
3555+
PVC_RECURSE_AGGREGATES,
35553556
PVC_INCLUDE_PLACEHOLDERS);
35563557
foreach(k, exprvars)
35573558
{

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,6 +132,7 @@ void
132132
build_base_rel_tlists(PlannerInfo *root, List *final_tlist)
133133
{
134134
List *tlist_vars = pull_var_clause((Node *) final_tlist,
135+
PVC_RECURSE_AGGREGATES,
135136
PVC_INCLUDE_PLACEHOLDERS);
136137

137138
if (tlist_vars != NIL)
@@ -1030,7 +1031,9 @@ distribute_qual_to_rels(PlannerInfo *root, Node *clause,
10301031
*/
10311032
if (bms_membership(relids) == BMS_MULTIPLE)
10321033
{
1033-
List *vars = pull_var_clause(clause, PVC_INCLUDE_PLACEHOLDERS);
1034+
List *vars = pull_var_clause(clause,
1035+
PVC_RECURSE_AGGREGATES,
1036+
PVC_INCLUDE_PLACEHOLDERS);
10341037

10351038
add_vars_to_targetlist(root, vars, relids);
10361039
list_free(vars);

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

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1474,11 +1474,16 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
14741474
* step. That's handled internally by make_sort_from_pathkeys,
14751475
* but we need the copyObject steps here to ensure that each plan
14761476
* node has a separately modifiable tlist.
1477+
*
1478+
* Note: it's essential here to use PVC_INCLUDE_AGGREGATES so that
1479+
* Vars mentioned only in aggregate expressions aren't pulled out
1480+
* as separate targetlist entries. Otherwise we could be putting
1481+
* ungrouped Vars directly into an Agg node's tlist, resulting in
1482+
* undefined behavior.
14771483
*/
1478-
window_tlist = flatten_tlist(tlist);
1479-
if (parse->hasAggs)
1480-
window_tlist = add_to_flat_tlist(window_tlist,
1481-
pull_agg_clause((Node *) tlist));
1484+
window_tlist = flatten_tlist(tlist,
1485+
PVC_INCLUDE_AGGREGATES,
1486+
PVC_INCLUDE_PLACEHOLDERS);
14821487
window_tlist = add_volatile_sort_exprs(window_tlist, tlist,
14831488
activeWindows);
14841489
result_plan->targetlist = (List *) copyObject(window_tlist);
@@ -2577,14 +2582,18 @@ make_subplanTargetList(PlannerInfo *root,
25772582
}
25782583

25792584
/*
2580-
* Otherwise, start with a "flattened" tlist (having just the vars
2581-
* mentioned in the targetlist and HAVING qual --- but not upper-level
2582-
* Vars; they will be replaced by Params later on). Note this includes
2583-
* vars used in resjunk items, so we are covering the needs of ORDER BY
2584-
* and window specifications.
2585+
* Otherwise, start with a "flattened" tlist (having just the Vars
2586+
* mentioned in the targetlist and HAVING qual). Note this includes Vars
2587+
* used in resjunk items, so we are covering the needs of ORDER BY and
2588+
* window specifications. Vars used within Aggrefs will be pulled out
2589+
* here, too.
25852590
*/
2586-
sub_tlist = flatten_tlist(tlist);
2587-
extravars = pull_var_clause(parse->havingQual, PVC_INCLUDE_PLACEHOLDERS);
2591+
sub_tlist = flatten_tlist(tlist,
2592+
PVC_RECURSE_AGGREGATES,
2593+
PVC_INCLUDE_PLACEHOLDERS);
2594+
extravars = pull_var_clause(parse->havingQual,
2595+
PVC_RECURSE_AGGREGATES,
2596+
PVC_INCLUDE_PLACEHOLDERS);
25882597
sub_tlist = add_to_flat_tlist(sub_tlist, extravars);
25892598
list_free(extravars);
25902599
*need_tlist_eval = false; /* only eval if not flat tlist */

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@ preprocess_targetlist(PlannerInfo *root, List *tlist)
154154
ListCell *l;
155155

156156
vars = pull_var_clause((Node *) parse->returningList,
157+
PVC_RECURSE_AGGREGATES,
157158
PVC_INCLUDE_PLACEHOLDERS);
158159
foreach(l, vars)
159160
{

‎src/backend/optimizer/util/clauses.c

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,6 @@ typedef struct
8484
} inline_error_callback_arg;
8585

8686
static bool contain_agg_clause_walker(Node *node, void *context);
87-
static bool pull_agg_clause_walker(Node *node, List **context);
8887
static bool count_agg_clauses_walker(Node *node,
8988
count_agg_clauses_context *context);
9089
static bool find_window_functions_walker(Node *node, WindowFuncLists *lists);
@@ -418,41 +417,6 @@ contain_agg_clause_walker(Node *node, void *context)
418417
return expression_tree_walker(node, contain_agg_clause_walker, context);
419418
}
420419

421-
/*
422-
* pull_agg_clause
423-
* Recursively search for Aggref nodes within a clause.
424-
*
425-
* Returns a List of all Aggrefs found.
426-
*
427-
* This does not descend into subqueries, and so should be used only after
428-
* reduction of sublinks to subplans, or in contexts where it's known there
429-
* are no subqueries. There mustn't be outer-aggregate references either.
430-
*/
431-
List *
432-
pull_agg_clause(Node *clause)
433-
{
434-
List *result = NIL;
435-
436-
(void) pull_agg_clause_walker(clause, &result);
437-
return result;
438-
}
439-
440-
static bool
441-
pull_agg_clause_walker(Node *node, List **context)
442-
{
443-
if (node == NULL)
444-
return false;
445-
if (IsA(node, Aggref))
446-
{
447-
Assert(((Aggref *) node)->agglevelsup == 0);
448-
*context = lappend(*context, node);
449-
return false; /* no need to descend into arguments */
450-
}
451-
Assert(!IsA(node, SubLink));
452-
return expression_tree_walker(node, pull_agg_clause_walker,
453-
(void *) context);
454-
}
455-
456420
/*
457421
* count_agg_clauses
458422
* Recursively count the Aggref nodes in an expression tree, and

‎src/backend/optimizer/util/placeholder.c

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,9 @@ find_placeholders_in_qual(PlannerInfo *root, Node *qual, Relids relids)
201201
* pull_var_clause does more than we need here, but it'll do and it's
202202
* convenient to use.
203203
*/
204-
vars = pull_var_clause(qual, PVC_INCLUDE_PLACEHOLDERS);
204+
vars = pull_var_clause(qual,
205+
PVC_RECURSE_AGGREGATES,
206+
PVC_INCLUDE_PLACEHOLDERS);
205207
foreach(vl, vars)
206208
{
207209
PlaceHolderVar *phv = (PlaceHolderVar *) lfirst(vl);
@@ -348,6 +350,7 @@ fix_placeholder_input_needed_levels(PlannerInfo *root)
348350
if (bms_membership(eval_at) == BMS_MULTIPLE)
349351
{
350352
List *vars = pull_var_clause((Node *) phinfo->ph_var->phexpr,
353+
PVC_RECURSE_AGGREGATES,
351354
PVC_INCLUDE_PLACEHOLDERS);
352355

353356
add_vars_to_targetlist(root, vars, eval_at);

‎src/backend/optimizer/util/tlist.c

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -76,9 +76,8 @@ tlist_member_ignore_relabel(Node *node, List *targetlist)
7676
* flatten_tlist
7777
* Create a target list that only contains unique variables.
7878
*
79-
* Note that Vars with varlevelsup > 0 are not included in the output
80-
* tlist. We expect that those will eventually be replaced with Params,
81-
* but that probably has not happened at the time this routine is called.
79+
* Aggrefs and PlaceHolderVars in the input are treated according to
80+
* aggbehavior and phbehavior, for which see pull_var_clause().
8281
*
8382
* 'tlist' is the current target list
8483
*
@@ -88,10 +87,12 @@ tlist_member_ignore_relabel(Node *node, List *targetlist)
8887
* Copying the Var nodes is probably overkill, but be safe for now.
8988
*/
9089
List *
91-
flatten_tlist(List *tlist)
90+
flatten_tlist(List *tlist, PVCAggregateBehavior aggbehavior,
91+
PVCPlaceHolderBehavior phbehavior)
9292
{
9393
List *vlist = pull_var_clause((Node *) tlist,
94-
PVC_INCLUDE_PLACEHOLDERS);
94+
aggbehavior,
95+
phbehavior);
9596
List *new_tlist;
9697

9798
new_tlist = add_to_flat_tlist(NIL, vlist);

‎src/backend/optimizer/util/var.c

Lines changed: 42 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,8 @@ typedef struct
5656
typedef struct
5757
{
5858
List *varlist;
59-
PVCPlaceHolderBehavior behavior;
59+
PVCAggregateBehavior aggbehavior;
60+
PVCPlaceHolderBehavior phbehavior;
6061
} pull_var_clause_context;
6162

6263
typedef struct
@@ -616,16 +617,22 @@ find_minimum_var_level_walker(Node *node,
616617
* pull_var_clause
617618
* Recursively pulls all Var nodes from an expression clause.
618619
*
619-
* PlaceHolderVars are handled according to 'behavior':
620+
* Aggrefs are handled according to 'aggbehavior':
621+
* PVC_REJECT_AGGREGATES throw error if Aggref found
622+
* PVC_INCLUDE_AGGREGATES include Aggrefs in output list
623+
* PVC_RECURSE_AGGREGATES recurse into Aggref arguments
624+
* Vars within an Aggref's expression are included only in the last case.
625+
*
626+
* PlaceHolderVars are handled according to 'phbehavior':
620627
* PVC_REJECT_PLACEHOLDERS throw error if PlaceHolderVar found
621628
* PVC_INCLUDE_PLACEHOLDERS include PlaceHolderVars in output list
622-
* PVC_RECURSE_PLACEHOLDERS recurse into PlaceHolderVar argument
629+
* PVC_RECURSE_PLACEHOLDERS recurse into PlaceHolderVar arguments
623630
* Vars within a PHV's expression are included only in the last case.
624631
*
625632
* CurrentOfExpr nodes are ignored in all cases.
626633
*
627-
* Upper-level vars (with varlevelsup > 0) are not included.
628-
* (These probably represent errors too, but we don't complain.)
634+
* Upper-level vars (with varlevelsup > 0) should not be seen here,
635+
* likewise for upper-level Aggrefs and PlaceHolderVars.
629636
*
630637
* Returns list of nodes found. Note the nodes themselves are not
631638
* copied, only referenced.
@@ -634,12 +641,14 @@ find_minimum_var_level_walker(Node *node,
634641
* of sublinks to subplans!
635642
*/
636643
List *
637-
pull_var_clause(Node *node, PVCPlaceHolderBehavior behavior)
644+
pull_var_clause(Node *node, PVCAggregateBehavior aggbehavior,
645+
PVCPlaceHolderBehavior phbehavior)
638646
{
639647
pull_var_clause_context context;
640648

641649
context.varlist = NIL;
642-
context.behavior = behavior;
650+
context.aggbehavior = aggbehavior;
651+
context.phbehavior = phbehavior;
643652

644653
pull_var_clause_walker(node, &context);
645654
return context.varlist;
@@ -652,20 +661,40 @@ pull_var_clause_walker(Node *node, pull_var_clause_context *context)
652661
return false;
653662
if (IsA(node, Var))
654663
{
655-
if (((Var *) node)->varlevelsup == 0)
656-
context->varlist = lappend(context->varlist, node);
664+
if (((Var *) node)->varlevelsup != 0)
665+
elog(ERROR, "Upper-level Var found where not expected");
666+
context->varlist = lappend(context->varlist, node);
657667
return false;
658668
}
659-
if (IsA(node, PlaceHolderVar))
669+
else if (IsA(node, Aggref))
670+
{
671+
if (((Aggref *) node)->agglevelsup != 0)
672+
elog(ERROR, "Upper-level Aggref found where not expected");
673+
switch (context->aggbehavior)
674+
{
675+
case PVC_REJECT_AGGREGATES:
676+
elog(ERROR, "Aggref found where not expected");
677+
break;
678+
case PVC_INCLUDE_AGGREGATES:
679+
context->varlist = lappend(context->varlist, node);
680+
/* we do NOT descend into the contained expression */
681+
return false;
682+
case PVC_RECURSE_AGGREGATES:
683+
/* ignore the aggregate, look at its argument instead */
684+
break;
685+
}
686+
}
687+
else if (IsA(node, PlaceHolderVar))
660688
{
661-
switch (context->behavior)
689+
if (((PlaceHolderVar *) node)->phlevelsup != 0)
690+
elog(ERROR, "Upper-level PlaceHolderVar found where not expected");
691+
switch (context->phbehavior)
662692
{
663693
case PVC_REJECT_PLACEHOLDERS:
664694
elog(ERROR, "PlaceHolderVar found where not expected");
665695
break;
666696
case PVC_INCLUDE_PLACEHOLDERS:
667-
if (((PlaceHolderVar *) node)->phlevelsup == 0)
668-
context->varlist = lappend(context->varlist, node);
697+
context->varlist = lappend(context->varlist, node);
669698
/* we do NOT descend into the contained expression */
670699
return false;
671700
case PVC_RECURSE_PLACEHOLDERS:

‎src/backend/utils/adt/selfuncs.c

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3090,7 +3090,9 @@ estimate_num_groups(PlannerInfo *root, List *groupExprs, double input_rows)
30903090
* PlaceHolderVar doesn't change the number of groups, which boils
30913091
* down to ignoring the possible addition of nulls to the result set).
30923092
*/
3093-
varshere = pull_var_clause(groupexpr, PVC_RECURSE_PLACEHOLDERS);
3093+
varshere = pull_var_clause(groupexpr,
3094+
PVC_RECURSE_AGGREGATES,
3095+
PVC_RECURSE_PLACEHOLDERS);
30943096

30953097
/*
30963098
* If we find any variable-free GROUP BY item, then either it is a

‎src/include/optimizer/clauses.h

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,6 @@ extern Expr *make_ands_explicit(List *andclauses);
4848
extern List *make_ands_implicit(Expr *clause);
4949

5050
extern bool contain_agg_clause(Node *clause);
51-
extern List *pull_agg_clause(Node *clause);
5251
extern void count_agg_clauses(PlannerInfo *root, Node *clause,
5352
AggClauseCosts *costs);
5453

‎src/include/optimizer/tlist.h

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,14 @@
1414
#ifndef TLIST_H
1515
#define TLIST_H
1616

17-
#include "nodes/relation.h"
17+
#include "optimizer/var.h"
1818

1919

2020
extern TargetEntry *tlist_member(Node *node, List *targetlist);
2121
extern TargetEntry *tlist_member_ignore_relabel(Node *node, List *targetlist);
2222

23-
extern List *flatten_tlist(List *tlist);
23+
extern List *flatten_tlist(List *tlist, PVCAggregateBehavior aggbehavior,
24+
PVCPlaceHolderBehavior phbehavior);
2425
extern List *add_to_flat_tlist(List *tlist, List *exprs);
2526

2627
extern List *get_tlist_exprs(List *tlist, bool includeJunk);

0 commit comments

Comments
 (0)