Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitc1d9579

Browse files
committed
Avoid listing ungrouped Vars in the targetlist of Agg-underneath-Window.
Regular aggregate functions in combination with, or within the argumentsof, window functions are OK per spec; they have the semantics that theaggregate output rows are computed and then we run the window functionsover that row set. (Thus, this combination is not really useful unlessthere'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 extrareferences to possibly-ungrouped Vars appearing outside the aggregatefunction calls themselves. See the added regression test case for anexample.Fixing this requires modifying the API of flatten_tlist and its underlyingfunction pull_var_clause. I chose to make pull_var_clause's API foraggregates identical to what it was already doing for placeholders, sincethe useful behaviors turn out to be the same (error, report node as-is, orrecurse 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 seeingthat it only occurs in a basically-useless corner case, it doesn't seemworth the risks of changing a function API in a minor release. There mightbe third-party code using pull_var_clause.
1 parent846af54 commitc1d9579

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 thevars
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 theVars
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
staticboolcontain_agg_clause_walker(Node*node,void*context);
87-
staticboolpull_agg_clause_walker(Node*node,List**context);
8887
staticboolcount_agg_clauses_walker(Node*node,
8988
count_agg_clauses_context*context);
9089
staticboolfind_window_functions_walker(Node*node,WindowFuncLists*lists);
@@ -418,41 +417,6 @@ contain_agg_clause_walker(Node *node, void *context)
418417
returnexpression_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-
returnresult;
438-
}
439-
440-
staticbool
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-
returnexpression_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);

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp