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

Commitcc5d985

Browse files
author
Richard Guo
committed
Fix incorrect handling of subquery pullup
When pulling up a subquery, if the subquery's target list items areused in grouping set columns, we need to wrap them in PlaceHolderVars.This ensures that expressions retain their separate identity so thatthey will match grouping set columns when appropriate.In9094767, we decided to wrap subquery outputs that are non-varexpressions in PlaceHolderVars. This prevents const-simplificationfrom merging them into the surrounding expressions after subquerypullup, which could otherwise lead to failing to match thosesubexpressions to grouping set columns, with the effect that they'dnot go to null when expected.However, that left some loose ends. If the subquery's target listcontains two or more identical Var expressions, we can still fail tomatch the Var expression to the expected grouping set expression.This is not related to const-simplification, but rather to how wematch expressions to lower target items in setrefs.c.For sort/group expressions, we use ressortgroupref matching, whichworks well. For other expressions, we primarily rely on comparing theexpressions to determine if they are the same. Therefore, we need away to prevent setrefs.c from matching the expression to some otheridentical ones.To fix, wrap all subquery outputs in PlaceHolderVars if the parentquery uses grouping sets, ensuring that they preserve their separateidentity throughout the whole planning process.Reported-by: Dean Rasheed <dean.a.rasheed@gmail.com>Author: Richard Guo <guofenglinux@gmail.com>Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com>Discussion:https://postgr.es/m/CAMbWs4-meSahaanKskpBn0KKxdHAXC1_EJCVWHxEodqirrGJnw@mail.gmail.com
1 parent4c49611 commitcc5d985

File tree

5 files changed

+93
-43
lines changed

5 files changed

+93
-43
lines changed

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

Lines changed: 51 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,15 @@ typedef struct nullingrel_info
5757
intrtlength;/* used only for assertion checks */
5858
}nullingrel_info;
5959

60+
/* Options for wrapping an expression for identification purposes */
61+
typedefenumReplaceWrapOption
62+
{
63+
REPLACE_WRAP_NONE,/* no expressions need to be wrapped */
64+
REPLACE_WRAP_ALL,/* all expressions need to be wrapped */
65+
REPLACE_WRAP_VARFREE,/* variable-free expressions need to be
66+
* wrapped */
67+
}ReplaceWrapOption;
68+
6069
typedefstructpullup_replace_vars_context
6170
{
6271
PlannerInfo*root;
@@ -70,7 +79,7 @@ typedef struct pullup_replace_vars_context
7079
* target_rte->lateral) */
7180
bool*outer_hasSubLinks;/* -> outer query's hasSubLinks */
7281
intvarno;/* varno of subquery */
73-
boolwrap_non_vars;/* do we needall non-Var outputs to be PHVs? */
82+
ReplaceWrapOptionwrap_option;/* do we needcertain outputs to be PHVs? */
7483
Node**rv_cache;/* cache for results with PHVs */
7584
}pullup_replace_vars_context;
7685

@@ -1025,23 +1034,18 @@ expand_virtual_generated_columns(PlannerInfo *root)
10251034
rvcontext.outer_hasSubLinks=NULL;
10261035
rvcontext.varno=rt_index;
10271036
/* this flag will be set below, if needed */
1028-
rvcontext.wrap_non_vars=false;
1037+
rvcontext.wrap_option=REPLACE_WRAP_NONE;
10291038
/* initialize cache array with indexes 0 .. length(tlist) */
10301039
rvcontext.rv_cache=palloc0((list_length(tlist)+1)*
10311040
sizeof(Node*));
10321041

10331042
/*
10341043
* If the query uses grouping sets, we need a PlaceHolderVar for
1035-
* anything that's not a simple Var. Again, this ensures that
1036-
* expressions retain their separate identity so that they will
1037-
* match grouping set columns when appropriate. (It'd be
1038-
* sufficient to wrap values used in grouping set columns, and do
1039-
* so only in non-aggregated portions of the tlist and havingQual,
1040-
* but that would require a lot of infrastructure that
1041-
* pullup_replace_vars hasn't currently got.)
1044+
* each expression of the relation's targetlist items. (See
1045+
* comments in pull_up_simple_subquery().)
10421046
*/
10431047
if (parse->groupingSets)
1044-
rvcontext.wrap_non_vars=true;
1048+
rvcontext.wrap_option=REPLACE_WRAP_ALL;
10451049

10461050
/*
10471051
* Apply pullup variable replacement throughout the query tree.
@@ -1435,22 +1439,22 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
14351439
rvcontext.outer_hasSubLinks=&parse->hasSubLinks;
14361440
rvcontext.varno=varno;
14371441
/* this flag will be set below, if needed */
1438-
rvcontext.wrap_non_vars=false;
1442+
rvcontext.wrap_option=REPLACE_WRAP_NONE;
14391443
/* initialize cache array with indexes 0 .. length(tlist) */
14401444
rvcontext.rv_cache=palloc0((list_length(subquery->targetList)+1)*
14411445
sizeof(Node*));
14421446

14431447
/*
14441448
* If the parent query uses grouping sets, we need a PlaceHolderVar for
1445-
*anything that's not a simple Var. This ensures that expressions retain
1446-
* their separate identity so that they will match grouping set columns
1447-
* when appropriate. (It'd be sufficient to wrap values used in grouping
1448-
* set columns, and do so only in non-aggregated portions of the tlist and
1449-
* havingQual, but that would require a lot of infrastructure that
1450-
* pullup_replace_vars hasn't currently got.)
1449+
*each expression of the subquery's targetlist items. This ensures that
1450+
*expressions retaintheir separate identity so that they will match
1451+
*grouping set columnswhen appropriate. (It'd be sufficient to wrap
1452+
*values used in groupingset columns, and do so only in non-aggregated
1453+
*portions of the tlist andhavingQual, but that would require a lot of
1454+
*infrastructure thatpullup_replace_vars hasn't currently got.)
14511455
*/
14521456
if (parse->groupingSets)
1453-
rvcontext.wrap_non_vars=true;
1457+
rvcontext.wrap_option=REPLACE_WRAP_ALL;
14541458

14551459
/*
14561460
* Replace all of the top query's references to the subquery's outputs
@@ -1976,7 +1980,7 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
19761980
rvcontext.nullinfo=NULL;
19771981
rvcontext.outer_hasSubLinks=&parse->hasSubLinks;
19781982
rvcontext.varno=varno;
1979-
rvcontext.wrap_non_vars=false;
1983+
rvcontext.wrap_option=REPLACE_WRAP_NONE;
19801984
/* initialize cache array with indexes 0 .. length(tlist) */
19811985
rvcontext.rv_cache=palloc0((list_length(tlist)+1)*
19821986
sizeof(Node*));
@@ -2144,18 +2148,18 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
21442148
rvcontext.outer_hasSubLinks=&parse->hasSubLinks;
21452149
rvcontext.varno= ((RangeTblRef*)jtnode)->rtindex;
21462150
/* this flag will be set below, if needed */
2147-
rvcontext.wrap_non_vars=false;
2151+
rvcontext.wrap_option=REPLACE_WRAP_NONE;
21482152
/* initialize cache array with indexes 0 .. length(tlist) */
21492153
rvcontext.rv_cache=palloc0((list_length(rvcontext.targetlist)+1)*
21502154
sizeof(Node*));
21512155

21522156
/*
21532157
* If the parent query uses grouping sets, we need a PlaceHolderVar for
2154-
*anything that's not a simple Var. (See comments in
2158+
*each expression of the subquery's targetlist items. (See comments in
21552159
* pull_up_simple_subquery().)
21562160
*/
21572161
if (parse->groupingSets)
2158-
rvcontext.wrap_non_vars=true;
2162+
rvcontext.wrap_option=REPLACE_WRAP_ALL;
21592163

21602164
/*
21612165
* Replace all of the top query's references to the RTE's output with
@@ -2406,13 +2410,13 @@ perform_pullup_replace_vars(PlannerInfo *root,
24062410
*/
24072411
if (containing_appendrel)
24082412
{
2409-
boolsave_wrap_non_vars=rvcontext->wrap_non_vars;
2413+
ReplaceWrapOptionsave_wrap_option=rvcontext->wrap_option;
24102414

2411-
rvcontext->wrap_non_vars=false;
2415+
rvcontext->wrap_option=REPLACE_WRAP_NONE;
24122416
containing_appendrel->translated_vars= (List*)
24132417
pullup_replace_vars((Node*)containing_appendrel->translated_vars,
24142418
rvcontext);
2415-
rvcontext->wrap_non_vars=save_wrap_non_vars;
2419+
rvcontext->wrap_option=save_wrap_option;
24162420
return;
24172421
}
24182422

@@ -2573,24 +2577,24 @@ replace_vars_in_jointree(Node *jtnode,
25732577
elseif (IsA(jtnode,JoinExpr))
25742578
{
25752579
JoinExpr*j= (JoinExpr*)jtnode;
2576-
boolsave_wrap_non_vars=context->wrap_non_vars;
2580+
ReplaceWrapOptionsave_wrap_option=context->wrap_option;
25772581

25782582
replace_vars_in_jointree(j->larg,context);
25792583
replace_vars_in_jointree(j->rarg,context);
25802584

25812585
/*
2582-
* Use PHVs within the join quals of a full join. Otherwise, we
2583-
* cannot identify which side of the join a pulled-up var-free
2584-
* expression came from, which can lead to failure to make a plan at
2585-
* all because none of the quals appear to be mergeable or hashable
2586-
* conditions.
2586+
* Use PHVs within the join quals of a full join for variable-free
2587+
*expressions. Otherwise, wecannot identify which side of the join
2588+
*a pulled-up variable-freeexpression came from, which can lead to
2589+
*failure to make a plan atall because none of the quals appear to
2590+
*be mergeable or hashableconditions.
25872591
*/
25882592
if (j->jointype==JOIN_FULL)
2589-
context->wrap_non_vars=true;
2593+
context->wrap_option=REPLACE_WRAP_VARFREE;
25902594

25912595
j->quals=pullup_replace_vars(j->quals,context);
25922596

2593-
context->wrap_non_vars=save_wrap_non_vars;
2597+
context->wrap_option=save_wrap_option;
25942598
}
25952599
else
25962600
elog(ERROR,"unrecognized node type: %d",
@@ -2630,10 +2634,11 @@ pullup_replace_vars_callback(Var *var,
26302634
* We need a PlaceHolderVar if the Var-to-be-replaced has nonempty
26312635
* varnullingrels (unless we find below that the replacement expression is
26322636
* a Var or PlaceHolderVar that we can just add the nullingrels to). We
2633-
* also need one if the caller has instructed us thatall non-Var/PHV
2637+
* also need one if the caller has instructed us thatcertain expression
26342638
* replacements need to be wrapped for identification purposes.
26352639
*/
2636-
need_phv= (var->varnullingrels!=NULL)||rcon->wrap_non_vars;
2640+
need_phv= (var->varnullingrels!=NULL)||
2641+
(rcon->wrap_option!=REPLACE_WRAP_NONE);
26372642

26382643
/*
26392644
* If PlaceHolderVars are needed, we cache the modified expressions in
@@ -2673,7 +2678,12 @@ pullup_replace_vars_callback(Var *var,
26732678
{
26742679
boolwrap;
26752680

2676-
if (varattno==InvalidAttrNumber)
2681+
if (rcon->wrap_option==REPLACE_WRAP_ALL)
2682+
{
2683+
/* Caller told us to wrap all expressions in a PlaceHolderVar */
2684+
wrap= true;
2685+
}
2686+
elseif (varattno==InvalidAttrNumber)
26772687
{
26782688
/*
26792689
* Insert PlaceHolderVar for whole-tuple reference. Notice
@@ -2733,11 +2743,6 @@ pullup_replace_vars_callback(Var *var,
27332743
}
27342744
}
27352745
}
2736-
elseif (rcon->wrap_non_vars)
2737-
{
2738-
/* Caller told us to wrap all non-Vars in a PlaceHolderVar */
2739-
wrap= true;
2740-
}
27412746
else
27422747
{
27432748
/*
@@ -2769,7 +2774,11 @@ pullup_replace_vars_callback(Var *var,
27692774
* This analysis could be tighter: in particular, a non-strict
27702775
* construct hidden within a lower-level PlaceHolderVar is not
27712776
* reason to add another PHV. But for now it doesn't seem
2772-
* worth the code to be more exact.
2777+
* worth the code to be more exact. This is also why it's
2778+
* preferable to handle bare PHVs in the above branch, rather
2779+
* than this branch. We also prefer to handle bare Vars in a
2780+
* separate branch, as it's cheaper this way and parallels the
2781+
* handling of PHVs.
27732782
*
27742783
* For a LATERAL subquery, we have to check the actual var
27752784
* membership of the node, but if it's non-lateral then any

‎src/backend/rewrite/rewriteManip.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1421,7 +1421,7 @@ remove_nulling_relids_mutator(Node *node,
14211421
* Note: it might seem desirable to remove the PHV altogether if
14221422
* phnullingrels goes to empty. Currently we dare not do that
14231423
* because we use PHVs in some cases to enforce separate identity
1424-
* of subexpressions; seewrap_non_vars usages in prepjointree.c.
1424+
* of subexpressions; seewrap_option usages in prepjointree.c.
14251425
*/
14261426
/* Copy the PlaceHolderVar and mutate what's below ... */
14271427
phv= (PlaceHolderVar*)

‎src/test/regress/expected/groupingsets.out

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,35 @@ select x, not x as not_x, q2 from
434434
| | 4567890123456789
435435
(5 rows)
436436

437+
select x, y
438+
from (select four as x, four as y from tenk1) as t
439+
group by grouping sets (x, y)
440+
having y is null
441+
order by 1, 2;
442+
x | y
443+
---+---
444+
0 |
445+
1 |
446+
2 |
447+
3 |
448+
(4 rows)
449+
450+
select x, y || 'y'
451+
from (select four as x, four as y from tenk1) as t
452+
group by grouping sets (x, y)
453+
order by 1, 2;
454+
x | ?column?
455+
---+----------
456+
0 |
457+
1 |
458+
2 |
459+
3 |
460+
| 0y
461+
| 1y
462+
| 2y
463+
| 3y
464+
(8 rows)
465+
437466
-- check qual push-down rules for a subquery with grouping sets
438467
explain (verbose, costs off)
439468
select * from (

‎src/test/regress/sql/groupingsets.sql

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,17 @@ select x, not x as not_x, q2 from
172172
group by grouping sets(x, q2)
173173
order by x, q2;
174174

175+
select x, y
176+
from (select fouras x, fouras yfrom tenk1)as t
177+
group by grouping sets (x, y)
178+
having y isnull
179+
order by1,2;
180+
181+
select x, y||'y'
182+
from (select fouras x, fouras yfrom tenk1)as t
183+
group by grouping sets (x, y)
184+
order by1,2;
185+
175186
-- check qual push-down rules for a subquery with grouping sets
176187
explain (verbose, costs off)
177188
select*from (

‎src/tools/pgindent/typedefs.list

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2477,6 +2477,7 @@ RepOriginId
24772477
ReparameterizeForeignPathByChild_function
24782478
ReplaceVarsFromTargetList_context
24792479
ReplaceVarsNoMatchOption
2480+
ReplaceWrapOption
24802481
ReplicaIdentityStmt
24812482
ReplicationKind
24822483
ReplicationSlot

0 commit comments

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