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

Commitb61b28f

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 parent7c88443 commitb61b28f

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)
@@ -2170,7 +2172,9 @@ AddRelationNewConstraints(Relation rel,
21702172
List*vars;
21712173
char*colname;
21722174

2173-
vars=pull_var_clause(expr,PVC_REJECT_PLACEHOLDERS);
2175+
vars=pull_var_clause(expr,
2176+
PVC_REJECT_AGGREGATES,
2177+
PVC_REJECT_PLACEHOLDERS);
21742178

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

‎src/backend/commands/trigger.c‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -302,7 +302,9 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString,
302302
* subselects in WHEN clauses; it would fail to examine the contents
303303
* of subselects.
304304
*/
305-
varList=pull_var_clause(whenClause,PVC_REJECT_PLACEHOLDERS);
305+
varList=pull_var_clause(whenClause,
306+
PVC_REJECT_AGGREGATES,
307+
PVC_REJECT_PLACEHOLDERS);
306308
foreach(lc,varList)
307309
{
308310
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
@@ -1473,11 +1473,16 @@ grouping_planner(PlannerInfo *root, double tuple_fraction)
14731473
* step. That's handled internally by make_sort_from_pathkeys,
14741474
* but we need the copyObject steps here to ensure that each plan
14751475
* node has a separately modifiable tlist.
1476+
*
1477+
* Note: it's essential here to use PVC_INCLUDE_AGGREGATES so that
1478+
* Vars mentioned only in aggregate expressions aren't pulled out
1479+
* as separate targetlist entries. Otherwise we could be putting
1480+
* ungrouped Vars directly into an Agg node's tlist, resulting in
1481+
* undefined behavior.
14761482
*/
1477-
window_tlist=flatten_tlist(tlist);
1478-
if (parse->hasAggs)
1479-
window_tlist=add_to_flat_tlist(window_tlist,
1480-
pull_agg_clause((Node*)tlist));
1483+
window_tlist=flatten_tlist(tlist,
1484+
PVC_INCLUDE_AGGREGATES,
1485+
PVC_INCLUDE_PLACEHOLDERS);
14811486
window_tlist=add_volatile_sort_exprs(window_tlist,tlist,
14821487
activeWindows);
14831488
result_plan->targetlist= (List*)copyObject(window_tlist);
@@ -2576,14 +2581,18 @@ make_subplanTargetList(PlannerInfo *root,
25762581
}
25772582

25782583
/*
2579-
* Otherwise, start with a "flattened" tlist (having just thevars
2580-
* mentioned in the targetlist and HAVING qual --- but not upper-level
2581-
*Vars; they will be replaced by Params later on). Note this includes
2582-
*vars used in resjunk items, so we are covering the needs of ORDER BY
2583-
*and window specifications.
2584+
* Otherwise, start with a "flattened" tlist (having just theVars
2585+
* mentioned in the targetlist and HAVING qual). Note this includes Vars
2586+
*used in resjunk items, so we are covering the needs of ORDER BY and
2587+
*window specifications. Vars used within Aggrefs will be pulled out
2588+
*here, too.
25842589
*/
2585-
sub_tlist=flatten_tlist(tlist);
2586-
extravars=pull_var_clause(parse->havingQual,PVC_INCLUDE_PLACEHOLDERS);
2590+
sub_tlist=flatten_tlist(tlist,
2591+
PVC_RECURSE_AGGREGATES,
2592+
PVC_INCLUDE_PLACEHOLDERS);
2593+
extravars=pull_var_clause(parse->havingQual,
2594+
PVC_RECURSE_AGGREGATES,
2595+
PVC_INCLUDE_PLACEHOLDERS);
25872596
sub_tlist=add_to_flat_tlist(sub_tlist,extravars);
25882597
list_free(extravars);
25892598
*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