Wrong results with subquery pullup and grouping sets
While working on the expansion of virtual generated columns, Dean
encountered $subject in [1]/messages/by-id/CAEZATCWsKqCtZ=ud26-gGV3zHt-hjS4OKG43GCBhSaYUWyfKiw@mail.gmail.com, which can be reproduced using the query
below.
create table t (a int);
insert into t values (1);
# select a, b
from (select a, a as b from t) ss
group by grouping sets(a, b)
having b = 1;
a | b
---+---
1 |
(1 row)
Note that the having clause filters out the wrong row.
In 90947674f, we fixed a similar issue by wrapping subquery outputs
that are non-var expressions in PlaceHolderVars. This prevents
const-simplification from merging them into the surrounding expression
after subquery pullup and resulting in something that won't match the
grouping set expression in setrefs.c.
It seems that we still have loose ends with that fix.
This issue illustrates that if the subquery's target list contains two
or more identical Var expressions, we can also fail to match the Var
expression to the expected grouping set expression. This is not
related to const-simplification, but rather to how we match
expressions to lower target items in setrefs.c.
For sort/group expressions, we use ressortgroupref matching, which
works well. For other expressions, we primarily rely on comparing the
expressions to determine if they are the same. This is why, in the
query above, the Var 'b' in the HAVING clause matches the first target
entry of the subquery, rather than the expected second one.
You might wonder why this issue wasn't fixed by the commit that
introduced a dummy RTE representing the output of the grouping step.
I think that commit prevents one expression that matches the grouping
item from being const-folded or preprocessed, but it doesn't prevent
setrefs.c from matching the expression to some other identical ones.
It seems to me that simple Var expressions in a subquery's target list
also need to retain their separate identity in order to match grouping
set columns after subquery pullup, not just non-var expressions.
Alternatively, is there a way to teach setrefs.c to match an
expression to the expected one when there are multiple identical
expressions?
[1]: /messages/by-id/CAEZATCWsKqCtZ=ud26-gGV3zHt-hjS4OKG43GCBhSaYUWyfKiw@mail.gmail.com
Thanks
Richard
On Wed, Mar 5, 2025 at 11:02 AM Richard Guo <guofenglinux@gmail.com> wrote:
create table t (a int);
insert into t values (1);# select a, b
from (select a, a as b from t) ss
group by grouping sets(a, b)
having b = 1;
a | b
---+---
1 |
(1 row)Note that the having clause filters out the wrong row.
BTW, this issue is not limited to HAVING clause; it can also happen to
targetlist.
# select a, b+1
from (select a, a as b from t) ss
group by grouping sets(a, b);
a | ?column?
---+----------
1 | 2
|
(2 rows)
Surely this is wrong.
Thanks
Richard
On Wed, Mar 5, 2025 at 11:02 AM Richard Guo <guofenglinux@gmail.com> wrote:
It seems to me that simple Var expressions in a subquery's target list
also need to retain their separate identity in order to match grouping
set columns after subquery pullup, not just non-var expressions.
I noticed the adjacent code that sets wrap_non_vars to true if we
are considering an appendrel child subquery, and I doubt this is
necessary.
/*
* If we are dealing with an appendrel member then anything that's not a
* simple Var has to be turned into a PlaceHolderVar. We force this to
* ensure that what we pull up doesn't get merged into a surrounding
* expression during later processing and then fail to match the
* expression actually available from the appendrel.
*/
if (containing_appendrel != NULL)
rvcontext.wrap_non_vars = true;
As explained in e42e31243, the only part of the upper query that could
reference the appendrel child yet is the translated_vars list of the
associated AppendRelInfo that we just made for this child, and we do
not want to force use of PHVs in the AppendRelInfo (see the comment in
perform_pullup_replace_vars). In fact, perform_pullup_replace_vars
sets wrap_non_vars to false before performing pullup_replace_vars on
the AppendRelInfo. It seems to me that this makes the code shown
above unnecessary, and we can simply remove it.
Any thoughts?
Thanks
Richard
On Wed, 5 Mar 2025 at 09:09, Richard Guo <guofenglinux@gmail.com> wrote:
I noticed the adjacent code that sets wrap_non_vars to true if we
are considering an appendrel child subquery, and I doubt this is
necessary./*
* If we are dealing with an appendrel member then anything that's not a
* simple Var has to be turned into a PlaceHolderVar. We force this to
* ensure that what we pull up doesn't get merged into a surrounding
* expression during later processing and then fail to match the
* expression actually available from the appendrel.
*/
if (containing_appendrel != NULL)
rvcontext.wrap_non_vars = true;As explained in e42e31243, the only part of the upper query that could
reference the appendrel child yet is the translated_vars list of the
associated AppendRelInfo that we just made for this child, and we do
not want to force use of PHVs in the AppendRelInfo (see the comment in
perform_pullup_replace_vars). In fact, perform_pullup_replace_vars
sets wrap_non_vars to false before performing pullup_replace_vars on
the AppendRelInfo. It seems to me that this makes the code shown
above unnecessary, and we can simply remove it.
Yes, that makes sense, and it seems like a fairly straightforward
simplification, given that perform_pullup_replace_vars() sets it back
to false shortly afterwards.
The same thing happens in pull_up_constant_function().
Regards,
Dean
On Wed, Mar 5, 2025 at 8:01 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
Yes, that makes sense, and it seems like a fairly straightforward
simplification, given that perform_pullup_replace_vars() sets it back
to false shortly afterwards.The same thing happens in pull_up_constant_function().
Thanks for looking.
Attached are the patches. 0001 removes the code that sets
wrap_non_vars to true for UNION ALL subqueries. And that leaves us
only two cases where we need PHVs for identification purposes:
1. If the query uses grouping sets, we need a PHV for each expression
of the subquery's targetlist items.
2. In the join quals of a full join, we need PHVs for variable-free
expressions.
In 0002, I changed the boolean wrap_non_vars to an enum, which
indicates whether no expressions, all expressions, or only
variable-free expressions need to be wrapped for identification
purposes.
Another thing is that when deciding whether to wrap the newnode in
pullup_replace_vars_callback(), I initially thought that we didn't
need to handle Vars/PHVs specifically, and could instead merge them
into the branch for handling general expressions. However, doing so
causes a plan diff in the regression tests. The reason is that a
non-strict construct hidden within a lower-level PlaceHolderVar can
lead the code for handling general expressions to mistakenly think
that another PHV is needed, even when it isn't. Therefore, the
branches for handling Vars/PHVs are retained in 0002.
Thanks
Richard
Attachments:
v1-0001-Remove-code-setting-wrap_non_vars-to-true-for-UNION-ALL-subqueries.patchapplication/octet-stream; name=v1-0001-Remove-code-setting-wrap_non_vars-to-true-for-UNION-ALL-subqueries.patchDownload
From 45dbbaf320e2362cffedc462e6b152bd7187ffec Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 6 Mar 2025 18:25:11 +0900
Subject: [PATCH v1 1/2] Remove code setting wrap_non_vars to true for UNION
ALL subqueries
In pull_up_simple_subquery and pull_up_constant_function, there is
code that sets wrap_non_vars to true when dealing with an appendrel
member. The goal is to wrap subquery outputs that are not simple Vars
in PlaceHolderVars, ensuring that what we pull up doesn't get merged
into a surrounding expression during later processing, which could
cause it to fail to match the expression actually available from the
appendrel.
However, this is unnecessary. When pulling up an appendrel child
subquery, the only part of the upper query that could reference the
appendrel child yet is the translated_vars list of the associated
AppendRelInfo that we just made for this child. Furthermore, we do
not want to force use of PHVs in the AppendRelInfo, as there is no
outer join between. In fact, perform_pullup_replace_vars always sets
wrap_non_vars to false before performing pullup_replace_vars on the
AppendRelInfo.
This patch simply removes the code that sets wrap_non_vars to true for
UNION ALL subqueries.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4-VXDEi1v+hZYLxpOv0riJxHsCkCH1f46tLnhonEAyGCQ@mail.gmail.com
---
src/backend/optimizer/prep/prepjointree.c | 18 ------------------
1 file changed, 18 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index bcc40dd5a84..6a4d005e6ce 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -1434,16 +1434,6 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
rvcontext.rv_cache = palloc0((list_length(subquery->targetList) + 1) *
sizeof(Node *));
- /*
- * If we are dealing with an appendrel member then anything that's not a
- * simple Var has to be turned into a PlaceHolderVar. We force this to
- * ensure that what we pull up doesn't get merged into a surrounding
- * expression during later processing and then fail to match the
- * expression actually available from the appendrel.
- */
- if (containing_appendrel != NULL)
- rvcontext.wrap_non_vars = true;
-
/*
* If the parent query uses grouping sets, we need a PlaceHolderVar for
* anything that's not a simple Var. Again, this ensures that expressions
@@ -2153,14 +2143,6 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
rvcontext.rv_cache = palloc0((list_length(rvcontext.targetlist) + 1) *
sizeof(Node *));
- /*
- * If we are dealing with an appendrel member then anything that's not a
- * simple Var has to be turned into a PlaceHolderVar. (See comments in
- * pull_up_simple_subquery().)
- */
- if (containing_appendrel != NULL)
- rvcontext.wrap_non_vars = true;
-
/*
* If the parent query uses grouping sets, we need a PlaceHolderVar for
* anything that's not a simple Var.
--
2.43.0
v1-0002-Fix-incorrect-handling-of-subquery-pullup.patchapplication/octet-stream; name=v1-0002-Fix-incorrect-handling-of-subquery-pullup.patchDownload
From b6851d1a611f665d067b64e1d10483081dbac04e Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 10 Mar 2025 16:41:00 +0900
Subject: [PATCH v1 2/2] Fix incorrect handling of subquery pullup
When pulling up a subquery, if the subquery's target list items are
used in grouping set columns, we need to wrap them in PlaceHolderVars.
This ensures that expressions retain their separate identity so that
they will match grouping set columns when appropriate.
In 90947674f, we decided to wrap subquery outputs that are non-var
expressions in PlaceHolderVars. This prevents const-simplification
from merging them into the surrounding expressions after subquery
pullup, which could otherwise lead to failing to match those
subexpressions to grouping set columns, with the effect that they'd
not go to null when expected.
However, that left some loose ends. If the subquery's target list
contains two or more identical Var expressions, we can still fail to
match the Var expression to the expected grouping set expression.
This is not related to const-simplification, but rather to how we
match expressions to lower target items in setrefs.c.
For sort/group expressions, we use ressortgroupref matching, which
works well. For other expressions, we primarily rely on comparing the
expressions to determine if they are the same. Therefore, we need a
way to prevent setrefs.c from matching the expression to some other
identical ones.
To fix, wrap all subquery outputs in PlaceHolderVars if the parent
query uses grouping sets, ensuring that they preserve their separate
identity throughout the whole planning process.
---
src/backend/optimizer/prep/prepjointree.c | 92 ++++++++++++----------
src/backend/rewrite/rewriteManip.c | 2 +-
src/test/regress/expected/groupingsets.out | 29 +++++++
src/test/regress/sql/groupingsets.sql | 11 +++
4 files changed, 91 insertions(+), 43 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 6a4d005e6ce..d9ab418ad33 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -57,6 +57,15 @@ typedef struct nullingrel_info
int rtlength; /* used only for assertion checks */
} nullingrel_info;
+/* Options for wrapping an expression for identification purposes */
+typedef enum ReplaceWrapOption
+{
+ REPLACE_WRAP_NONE, /* no expressions need to be wrapped */
+ REPLACE_WRAP_ALL, /* all expressions need to be wrapped */
+ REPLACE_WRAP_VARFREE, /* variable-free expressions need to be
+ * wrapped */
+} ReplaceWrapOption;
+
typedef struct pullup_replace_vars_context
{
PlannerInfo *root;
@@ -70,7 +79,7 @@ typedef struct pullup_replace_vars_context
* target_rte->lateral) */
bool *outer_hasSubLinks; /* -> outer query's hasSubLinks */
int varno; /* varno of subquery */
- bool wrap_non_vars; /* do we need all non-Var outputs to be PHVs? */
+ ReplaceWrapOption wrap_option; /* do we need certain outputs to be PHVs? */
Node **rv_cache; /* cache for results with PHVs */
} pullup_replace_vars_context;
@@ -1019,23 +1028,18 @@ expand_virtual_generated_columns(PlannerInfo *root)
rvcontext.outer_hasSubLinks = NULL;
rvcontext.varno = rt_index;
/* this flag will be set below, if needed */
- rvcontext.wrap_non_vars = false;
+ rvcontext.wrap_option = REPLACE_WRAP_NONE;
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(tlist) + 1) *
sizeof(Node *));
/*
* If the query uses grouping sets, we need a PlaceHolderVar for
- * anything that's not a simple Var. Again, this ensures that
- * expressions retain their separate identity so that they will
- * match grouping set columns when appropriate. (It'd be
- * sufficient to wrap values used in grouping set columns, and do
- * so only in non-aggregated portions of the tlist and havingQual,
- * but that would require a lot of infrastructure that
- * pullup_replace_vars hasn't currently got.)
+ * each expression of the relation's targetlist items. (See
+ * comments in pull_up_simple_subquery().)
*/
if (parse->groupingSets)
- rvcontext.wrap_non_vars = true;
+ rvcontext.wrap_option = REPLACE_WRAP_ALL;
/*
* Apply pullup variable replacement throughout the query tree.
@@ -1429,22 +1433,22 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
rvcontext.varno = varno;
/* this flag will be set below, if needed */
- rvcontext.wrap_non_vars = false;
+ rvcontext.wrap_option = REPLACE_WRAP_NONE;
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(subquery->targetList) + 1) *
sizeof(Node *));
/*
* If the parent query uses grouping sets, we need a PlaceHolderVar for
- * anything that's not a simple Var. Again, this ensures that expressions
- * retain their separate identity so that they will match grouping set
- * columns when appropriate. (It'd be sufficient to wrap values used in
- * grouping set columns, and do so only in non-aggregated portions of the
- * tlist and havingQual, but that would require a lot of infrastructure
- * that pullup_replace_vars hasn't currently got.)
+ * each expression of the subquery's targetlist items. This ensures that
+ * expressions retain their separate identity so that they will match
+ * grouping set columns when appropriate. (It'd be sufficient to wrap
+ * values used in grouping set columns, and do so only in non-aggregated
+ * portions of the tlist and havingQual, but that would require a lot of
+ * infrastructure that pullup_replace_vars hasn't currently got.)
*/
if (parse->groupingSets)
- rvcontext.wrap_non_vars = true;
+ rvcontext.wrap_option = REPLACE_WRAP_ALL;
/*
* Replace all of the top query's references to the subquery's outputs
@@ -1970,7 +1974,7 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
rvcontext.nullinfo = NULL;
rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
rvcontext.varno = varno;
- rvcontext.wrap_non_vars = false;
+ rvcontext.wrap_option = REPLACE_WRAP_NONE;
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(tlist) + 1) *
sizeof(Node *));
@@ -2138,17 +2142,18 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
rvcontext.varno = ((RangeTblRef *) jtnode)->rtindex;
/* this flag will be set below, if needed */
- rvcontext.wrap_non_vars = false;
+ rvcontext.wrap_option = REPLACE_WRAP_NONE;
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(rvcontext.targetlist) + 1) *
sizeof(Node *));
/*
* If the parent query uses grouping sets, we need a PlaceHolderVar for
- * anything that's not a simple Var.
+ * each expression of the subquery's targetlist items. (See comments in
+ * pull_up_simple_subquery().)
*/
if (parse->groupingSets)
- rvcontext.wrap_non_vars = true;
+ rvcontext.wrap_option = REPLACE_WRAP_ALL;
/*
* Replace all of the top query's references to the RTE's output with
@@ -2399,13 +2404,13 @@ perform_pullup_replace_vars(PlannerInfo *root,
*/
if (containing_appendrel)
{
- bool save_wrap_non_vars = rvcontext->wrap_non_vars;
+ ReplaceWrapOption save_wrap_option = rvcontext->wrap_option;
- rvcontext->wrap_non_vars = false;
+ rvcontext->wrap_option = REPLACE_WRAP_NONE;
containing_appendrel->translated_vars = (List *)
pullup_replace_vars((Node *) containing_appendrel->translated_vars,
rvcontext);
- rvcontext->wrap_non_vars = save_wrap_non_vars;
+ rvcontext->wrap_option = save_wrap_option;
return;
}
@@ -2566,24 +2571,24 @@ replace_vars_in_jointree(Node *jtnode,
else if (IsA(jtnode, JoinExpr))
{
JoinExpr *j = (JoinExpr *) jtnode;
- bool save_wrap_non_vars = context->wrap_non_vars;
+ ReplaceWrapOption save_wrap_option = context->wrap_option;
replace_vars_in_jointree(j->larg, context);
replace_vars_in_jointree(j->rarg, context);
/*
- * Use PHVs within the join quals of a full join. Otherwise, we
- * cannot identify which side of the join a pulled-up var-free
- * expression came from, which can lead to failure to make a plan at
- * all because none of the quals appear to be mergeable or hashable
- * conditions.
+ * Use PHVs within the join quals of a full join for variable-free
+ * expressions. Otherwise, we cannot identify which side of the join
+ * a pulled-up variable-free expression came from, which can lead to
+ * failure to make a plan at all because none of the quals appear to
+ * be mergeable or hashable conditions.
*/
if (j->jointype == JOIN_FULL)
- context->wrap_non_vars = true;
+ context->wrap_option = REPLACE_WRAP_VARFREE;
j->quals = pullup_replace_vars(j->quals, context);
- context->wrap_non_vars = save_wrap_non_vars;
+ context->wrap_option = save_wrap_option;
}
else
elog(ERROR, "unrecognized node type: %d",
@@ -2623,10 +2628,11 @@ pullup_replace_vars_callback(Var *var,
* We need a PlaceHolderVar if the Var-to-be-replaced has nonempty
* varnullingrels (unless we find below that the replacement expression is
* a Var or PlaceHolderVar that we can just add the nullingrels to). We
- * also need one if the caller has instructed us that all non-Var/PHV
+ * also need one if the caller has instructed us that certain expression
* replacements need to be wrapped for identification purposes.
*/
- need_phv = (var->varnullingrels != NULL) || rcon->wrap_non_vars;
+ need_phv = (var->varnullingrels != NULL) ||
+ (rcon->wrap_option != REPLACE_WRAP_NONE);
/*
* If PlaceHolderVars are needed, we cache the modified expressions in
@@ -2666,7 +2672,12 @@ pullup_replace_vars_callback(Var *var,
{
bool wrap;
- if (varattno == InvalidAttrNumber)
+ if (rcon->wrap_option == REPLACE_WRAP_ALL)
+ {
+ /* Caller told us to wrap all expressions in a PlaceHolderVar */
+ wrap = true;
+ }
+ else if (varattno == InvalidAttrNumber)
{
/*
* Insert PlaceHolderVar for whole-tuple reference. Notice
@@ -2726,11 +2737,6 @@ pullup_replace_vars_callback(Var *var,
}
}
}
- else if (rcon->wrap_non_vars)
- {
- /* Caller told us to wrap all non-Vars in a PlaceHolderVar */
- wrap = true;
- }
else
{
/*
@@ -2762,7 +2768,9 @@ pullup_replace_vars_callback(Var *var,
* This analysis could be tighter: in particular, a non-strict
* construct hidden within a lower-level PlaceHolderVar is not
* reason to add another PHV. But for now it doesn't seem
- * worth the code to be more exact.
+ * worth the code to be more exact. This is also why we need
+ * to handle Vars and PHVs in the above branches, rather than
+ * merging them into this one.
*
* For a LATERAL subquery, we have to check the actual var
* membership of the node, but if it's non-lateral then any
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 98a265cd3d5..1c61085a0a7 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -1421,7 +1421,7 @@ remove_nulling_relids_mutator(Node *node,
* Note: it might seem desirable to remove the PHV altogether if
* phnullingrels goes to empty. Currently we dare not do that
* because we use PHVs in some cases to enforce separate identity
- * of subexpressions; see wrap_non_vars usages in prepjointree.c.
+ * of subexpressions; see wrap_option usages in prepjointree.c.
*/
/* Copy the PlaceHolderVar and mutate what's below ... */
phv = (PlaceHolderVar *)
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index d7c9b44605d..3a4afb6f730 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -434,6 +434,35 @@ select x, not x as not_x, q2 from
| | 4567890123456789
(5 rows)
+select x, y
+ from (select four as x, four as y from tenk1) as t
+ group by grouping sets (x, y)
+ having y is null
+ order by 1, 2;
+ x | y
+---+---
+ 0 |
+ 1 |
+ 2 |
+ 3 |
+(4 rows)
+
+select x, y || 'y'
+ from (select four as x, four as y from tenk1) as t
+ group by grouping sets (x, y)
+ order by 1, 2;
+ x | ?column?
+---+----------
+ 0 |
+ 1 |
+ 2 |
+ 3 |
+ | 0y
+ | 1y
+ | 2y
+ | 3y
+(8 rows)
+
-- check qual push-down rules for a subquery with grouping sets
explain (verbose, costs off)
select * from (
diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index 21cd3121940..38d3cdd0fd8 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -172,6 +172,17 @@ select x, not x as not_x, q2 from
group by grouping sets(x, q2)
order by x, q2;
+select x, y
+ from (select four as x, four as y from tenk1) as t
+ group by grouping sets (x, y)
+ having y is null
+ order by 1, 2;
+
+select x, y || 'y'
+ from (select four as x, four as y from tenk1) as t
+ group by grouping sets (x, y)
+ order by 1, 2;
+
-- check qual push-down rules for a subquery with grouping sets
explain (verbose, costs off)
select * from (
--
2.43.0
On Mon, 10 Mar 2025 at 13:05, Richard Guo <guofenglinux@gmail.com> wrote:
Attached are the patches.
This looks good to me. I did some more testing, and I wasn't able to break it.
Some minor nitpicks:
These 2 comment changes from 0002 could be made part of 0001:
1). In pull_up_simple_subquery(), removing the word "Again" from the
comment following the deleted block, since this is now the only place
that sets wrap_non_vars there.
2). In pull_up_constant_function(), moving "(See comments in
pull_up_simple_subquery().)" to the next comment.
Another thing is that when deciding whether to wrap the newnode in
pullup_replace_vars_callback(), I initially thought that we didn't
need to handle Vars/PHVs specifically, and could instead merge them
into the branch for handling general expressions. However, doing so
causes a plan diff in the regression tests. The reason is that a
non-strict construct hidden within a lower-level PlaceHolderVar can
lead the code for handling general expressions to mistakenly think
that another PHV is needed, even when it isn't. Therefore, the
branches for handling Vars/PHVs are retained in 0002.
Right. The comment addition in 0002, relating to that, confused me at first:
* This analysis could be tighter: in particular, a non-strict
* construct hidden within a lower-level PlaceHolderVar is not
* reason to add another PHV. But for now it doesn't seem
- * worth the code to be more exact.
+ * worth the code to be more exact. This is also why we need
+ * to handle Vars and PHVs in the above branches, rather than
+ * merging them into this one.AIUI, it's not really that it *needs* to handle Vars and PHVs
separately, it's just better if it does, since that avoids
unnecessarily wrapping a PHV again, if it contains non-strict
constructs. Also, AFAICS there's no technical reason why simple Vars
couldn't be handled here (all the tests pass if that branch is
removed), but as a comment higher up says, that would be more
expensive. So perhaps this new comment should say "This is why it's
preferable to handle simple PHVs in the branch above, rather than this
branch."
Finally, ReplaceWrapOption should be added to pgindent's typedefs.list.
Regards,
Dean
On Wed, Mar 12, 2025 at 1:32 AM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Mon, 10 Mar 2025 at 13:05, Richard Guo <guofenglinux@gmail.com> wrote:
Attached are the patches.
These 2 comment changes from 0002 could be made part of 0001:
1). In pull_up_simple_subquery(), removing the word "Again" from the
comment following the deleted block, since this is now the only place
that sets wrap_non_vars there.2). In pull_up_constant_function(), moving "(See comments in
pull_up_simple_subquery().)" to the next comment.
Nice catch.
Another thing is that when deciding whether to wrap the newnode in
pullup_replace_vars_callback(), I initially thought that we didn't
need to handle Vars/PHVs specifically, and could instead merge them
into the branch for handling general expressions. However, doing so
causes a plan diff in the regression tests. The reason is that a
non-strict construct hidden within a lower-level PlaceHolderVar can
lead the code for handling general expressions to mistakenly think
that another PHV is needed, even when it isn't. Therefore, the
branches for handling Vars/PHVs are retained in 0002.
Right. The comment addition in 0002, relating to that, confused me at first:
* This analysis could be tighter: in particular, a non-strict * construct hidden within a lower-level PlaceHolderVar is not * reason to add another PHV. But for now it doesn't seem - * worth the code to be more exact. + * worth the code to be more exact. This is also why we need + * to handle Vars and PHVs in the above branches, rather than + * merging them into this one.AIUI, it's not really that it *needs* to handle Vars and PHVs
separately, it's just better if it does, since that avoids
unnecessarily wrapping a PHV again, if it contains non-strict
constructs. Also, AFAICS there's no technical reason why simple Vars
couldn't be handled here (all the tests pass if that branch is
removed), but as a comment higher up says, that would be more
expensive. So perhaps this new comment should say "This is why it's
preferable to handle simple PHVs in the branch above, rather than this
branch."
Exactly. Technically, we could remove the branch for Vars. However,
I chose to keep it because: 1) it's more efficient this way, and 2)
it's better to keep the handling of Vars and PHVs aligned. I refined
the comment in v2 and ended up with:
* This analysis could be tighter: in particular, a non-strict
* construct hidden within a lower-level PlaceHolderVar is not
* reason to add another PHV. But for now it doesn't seem
- * worth the code to be more exact.
+ * worth the code to be more exact. This is also why it's
+ * preferable to handle bare PHVs in the above branch, rather
+ * than this branch. We also prefer to handle bare Vars in a
+ * separate branch, as it's cheaper this way and parallels the
+ * handling of PHVs.Finally, ReplaceWrapOption should be added to pgindent's typedefs.list.
Nice catch.
For backpatching, 0001 seems more like a cosmetic change, and I don't
think it needs to be backpatched. 0002 is a bug fix, but I'm not
confident enough to commit it to the back branches. Given that there
are other wrong result issues with grouping sets fixed in version 18
but not in earlier versions, and that there are no field reports about
this issue, perhaps it's OK to refrain from backpatching this one?
Thanks
Richard
Attachments:
v2-0001-Remove-code-setting-wrap_non_vars-to-true-for-UNION-ALL-subqueries.patchapplication/octet-stream; name=v2-0001-Remove-code-setting-wrap_non_vars-to-true-for-UNION-ALL-subqueries.patchDownload
From 3cc322d11a66c9d3891e573ba85c7a79d5b18ace Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Thu, 6 Mar 2025 18:25:11 +0900
Subject: [PATCH v2 1/2] Remove code setting wrap_non_vars to true for UNION
ALL subqueries
In pull_up_simple_subquery and pull_up_constant_function, there is
code that sets wrap_non_vars to true when dealing with an appendrel
member. The goal is to wrap subquery outputs that are not simple Vars
in PlaceHolderVars, ensuring that what we pull up doesn't get merged
into a surrounding expression during later processing, which could
cause it to fail to match the expression actually available from the
appendrel.
However, this is unnecessary. When pulling up an appendrel child
subquery, the only part of the upper query that could reference the
appendrel child yet is the translated_vars list of the associated
AppendRelInfo that we just made for this child. Furthermore, we do
not want to force use of PHVs in the AppendRelInfo, as there is no
outer join between. In fact, perform_pullup_replace_vars always sets
wrap_non_vars to false before performing pullup_replace_vars on the
AppendRelInfo.
This patch simply removes the code that sets wrap_non_vars to true for
UNION ALL subqueries.
Author: Richard Guo <guofenglinux@gmail.com>
Reviewed-by: Dean Rasheed <dean.a.rasheed@gmail.com>
Discussion: https://postgr.es/m/CAMbWs4-VXDEi1v+hZYLxpOv0riJxHsCkCH1f46tLnhonEAyGCQ@mail.gmail.com
---
src/backend/optimizer/prep/prepjointree.c | 33 ++++++-----------------
1 file changed, 8 insertions(+), 25 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index bcc40dd5a84..9ff9a4f1004 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -1434,24 +1434,14 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
rvcontext.rv_cache = palloc0((list_length(subquery->targetList) + 1) *
sizeof(Node *));
- /*
- * If we are dealing with an appendrel member then anything that's not a
- * simple Var has to be turned into a PlaceHolderVar. We force this to
- * ensure that what we pull up doesn't get merged into a surrounding
- * expression during later processing and then fail to match the
- * expression actually available from the appendrel.
- */
- if (containing_appendrel != NULL)
- rvcontext.wrap_non_vars = true;
-
/*
* If the parent query uses grouping sets, we need a PlaceHolderVar for
- * anything that's not a simple Var. Again, this ensures that expressions
- * retain their separate identity so that they will match grouping set
- * columns when appropriate. (It'd be sufficient to wrap values used in
- * grouping set columns, and do so only in non-aggregated portions of the
- * tlist and havingQual, but that would require a lot of infrastructure
- * that pullup_replace_vars hasn't currently got.)
+ * anything that's not a simple Var. This ensures that expressions retain
+ * their separate identity so that they will match grouping set columns
+ * when appropriate. (It'd be sufficient to wrap values used in grouping
+ * set columns, and do so only in non-aggregated portions of the tlist and
+ * havingQual, but that would require a lot of infrastructure that
+ * pullup_replace_vars hasn't currently got.)
*/
if (parse->groupingSets)
rvcontext.wrap_non_vars = true;
@@ -2153,17 +2143,10 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
rvcontext.rv_cache = palloc0((list_length(rvcontext.targetlist) + 1) *
sizeof(Node *));
- /*
- * If we are dealing with an appendrel member then anything that's not a
- * simple Var has to be turned into a PlaceHolderVar. (See comments in
- * pull_up_simple_subquery().)
- */
- if (containing_appendrel != NULL)
- rvcontext.wrap_non_vars = true;
-
/*
* If the parent query uses grouping sets, we need a PlaceHolderVar for
- * anything that's not a simple Var.
+ * anything that's not a simple Var. (See comments in
+ * pull_up_simple_subquery().)
*/
if (parse->groupingSets)
rvcontext.wrap_non_vars = true;
--
2.43.0
v2-0002-Fix-incorrect-handling-of-subquery-pullup.patchapplication/octet-stream; name=v2-0002-Fix-incorrect-handling-of-subquery-pullup.patchDownload
From e3d0892a87107ecd482ec534460289cdbfed4205 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Mon, 10 Mar 2025 16:41:00 +0900
Subject: [PATCH v2 2/2] Fix incorrect handling of subquery pullup
When pulling up a subquery, if the subquery's target list items are
used in grouping set columns, we need to wrap them in PlaceHolderVars.
This ensures that expressions retain their separate identity so that
they will match grouping set columns when appropriate.
In 90947674f, we decided to wrap subquery outputs that are non-var
expressions in PlaceHolderVars. This prevents const-simplification
from merging them into the surrounding expressions after subquery
pullup, which could otherwise lead to failing to match those
subexpressions to grouping set columns, with the effect that they'd
not go to null when expected.
However, that left some loose ends. If the subquery's target list
contains two or more identical Var expressions, we can still fail to
match the Var expression to the expected grouping set expression.
This is not related to const-simplification, but rather to how we
match expressions to lower target items in setrefs.c.
For sort/group expressions, we use ressortgroupref matching, which
works well. For other expressions, we primarily rely on comparing the
expressions to determine if they are the same. Therefore, we need a
way to prevent setrefs.c from matching the expression to some other
identical ones.
To fix, wrap all subquery outputs in PlaceHolderVars if the parent
query uses grouping sets, ensuring that they preserve their separate
identity 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
---
src/backend/optimizer/prep/prepjointree.c | 93 ++++++++++++----------
src/backend/rewrite/rewriteManip.c | 2 +-
src/test/regress/expected/groupingsets.out | 29 +++++++
src/test/regress/sql/groupingsets.sql | 11 +++
src/tools/pgindent/typedefs.list | 1 +
5 files changed, 93 insertions(+), 43 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 9ff9a4f1004..4af721004c7 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -57,6 +57,15 @@ typedef struct nullingrel_info
int rtlength; /* used only for assertion checks */
} nullingrel_info;
+/* Options for wrapping an expression for identification purposes */
+typedef enum ReplaceWrapOption
+{
+ REPLACE_WRAP_NONE, /* no expressions need to be wrapped */
+ REPLACE_WRAP_ALL, /* all expressions need to be wrapped */
+ REPLACE_WRAP_VARFREE, /* variable-free expressions need to be
+ * wrapped */
+} ReplaceWrapOption;
+
typedef struct pullup_replace_vars_context
{
PlannerInfo *root;
@@ -70,7 +79,7 @@ typedef struct pullup_replace_vars_context
* target_rte->lateral) */
bool *outer_hasSubLinks; /* -> outer query's hasSubLinks */
int varno; /* varno of subquery */
- bool wrap_non_vars; /* do we need all non-Var outputs to be PHVs? */
+ ReplaceWrapOption wrap_option; /* do we need certain outputs to be PHVs? */
Node **rv_cache; /* cache for results with PHVs */
} pullup_replace_vars_context;
@@ -1019,23 +1028,18 @@ expand_virtual_generated_columns(PlannerInfo *root)
rvcontext.outer_hasSubLinks = NULL;
rvcontext.varno = rt_index;
/* this flag will be set below, if needed */
- rvcontext.wrap_non_vars = false;
+ rvcontext.wrap_option = REPLACE_WRAP_NONE;
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(tlist) + 1) *
sizeof(Node *));
/*
* If the query uses grouping sets, we need a PlaceHolderVar for
- * anything that's not a simple Var. Again, this ensures that
- * expressions retain their separate identity so that they will
- * match grouping set columns when appropriate. (It'd be
- * sufficient to wrap values used in grouping set columns, and do
- * so only in non-aggregated portions of the tlist and havingQual,
- * but that would require a lot of infrastructure that
- * pullup_replace_vars hasn't currently got.)
+ * each expression of the relation's targetlist items. (See
+ * comments in pull_up_simple_subquery().)
*/
if (parse->groupingSets)
- rvcontext.wrap_non_vars = true;
+ rvcontext.wrap_option = REPLACE_WRAP_ALL;
/*
* Apply pullup variable replacement throughout the query tree.
@@ -1429,22 +1433,22 @@ pull_up_simple_subquery(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte,
rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
rvcontext.varno = varno;
/* this flag will be set below, if needed */
- rvcontext.wrap_non_vars = false;
+ rvcontext.wrap_option = REPLACE_WRAP_NONE;
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(subquery->targetList) + 1) *
sizeof(Node *));
/*
* If the parent query uses grouping sets, we need a PlaceHolderVar for
- * anything that's not a simple Var. This ensures that expressions retain
- * their separate identity so that they will match grouping set columns
- * when appropriate. (It'd be sufficient to wrap values used in grouping
- * set columns, and do so only in non-aggregated portions of the tlist and
- * havingQual, but that would require a lot of infrastructure that
- * pullup_replace_vars hasn't currently got.)
+ * each expression of the subquery's targetlist items. This ensures that
+ * expressions retain their separate identity so that they will match
+ * grouping set columns when appropriate. (It'd be sufficient to wrap
+ * values used in grouping set columns, and do so only in non-aggregated
+ * portions of the tlist and havingQual, but that would require a lot of
+ * infrastructure that pullup_replace_vars hasn't currently got.)
*/
if (parse->groupingSets)
- rvcontext.wrap_non_vars = true;
+ rvcontext.wrap_option = REPLACE_WRAP_ALL;
/*
* Replace all of the top query's references to the subquery's outputs
@@ -1970,7 +1974,7 @@ pull_up_simple_values(PlannerInfo *root, Node *jtnode, RangeTblEntry *rte)
rvcontext.nullinfo = NULL;
rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
rvcontext.varno = varno;
- rvcontext.wrap_non_vars = false;
+ rvcontext.wrap_option = REPLACE_WRAP_NONE;
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(tlist) + 1) *
sizeof(Node *));
@@ -2138,18 +2142,18 @@ pull_up_constant_function(PlannerInfo *root, Node *jtnode,
rvcontext.outer_hasSubLinks = &parse->hasSubLinks;
rvcontext.varno = ((RangeTblRef *) jtnode)->rtindex;
/* this flag will be set below, if needed */
- rvcontext.wrap_non_vars = false;
+ rvcontext.wrap_option = REPLACE_WRAP_NONE;
/* initialize cache array with indexes 0 .. length(tlist) */
rvcontext.rv_cache = palloc0((list_length(rvcontext.targetlist) + 1) *
sizeof(Node *));
/*
* If the parent query uses grouping sets, we need a PlaceHolderVar for
- * anything that's not a simple Var. (See comments in
+ * each expression of the subquery's targetlist items. (See comments in
* pull_up_simple_subquery().)
*/
if (parse->groupingSets)
- rvcontext.wrap_non_vars = true;
+ rvcontext.wrap_option = REPLACE_WRAP_ALL;
/*
* Replace all of the top query's references to the RTE's output with
@@ -2400,13 +2404,13 @@ perform_pullup_replace_vars(PlannerInfo *root,
*/
if (containing_appendrel)
{
- bool save_wrap_non_vars = rvcontext->wrap_non_vars;
+ ReplaceWrapOption save_wrap_option = rvcontext->wrap_option;
- rvcontext->wrap_non_vars = false;
+ rvcontext->wrap_option = REPLACE_WRAP_NONE;
containing_appendrel->translated_vars = (List *)
pullup_replace_vars((Node *) containing_appendrel->translated_vars,
rvcontext);
- rvcontext->wrap_non_vars = save_wrap_non_vars;
+ rvcontext->wrap_option = save_wrap_option;
return;
}
@@ -2567,24 +2571,24 @@ replace_vars_in_jointree(Node *jtnode,
else if (IsA(jtnode, JoinExpr))
{
JoinExpr *j = (JoinExpr *) jtnode;
- bool save_wrap_non_vars = context->wrap_non_vars;
+ ReplaceWrapOption save_wrap_option = context->wrap_option;
replace_vars_in_jointree(j->larg, context);
replace_vars_in_jointree(j->rarg, context);
/*
- * Use PHVs within the join quals of a full join. Otherwise, we
- * cannot identify which side of the join a pulled-up var-free
- * expression came from, which can lead to failure to make a plan at
- * all because none of the quals appear to be mergeable or hashable
- * conditions.
+ * Use PHVs within the join quals of a full join for variable-free
+ * expressions. Otherwise, we cannot identify which side of the join
+ * a pulled-up variable-free expression came from, which can lead to
+ * failure to make a plan at all because none of the quals appear to
+ * be mergeable or hashable conditions.
*/
if (j->jointype == JOIN_FULL)
- context->wrap_non_vars = true;
+ context->wrap_option = REPLACE_WRAP_VARFREE;
j->quals = pullup_replace_vars(j->quals, context);
- context->wrap_non_vars = save_wrap_non_vars;
+ context->wrap_option = save_wrap_option;
}
else
elog(ERROR, "unrecognized node type: %d",
@@ -2624,10 +2628,11 @@ pullup_replace_vars_callback(Var *var,
* We need a PlaceHolderVar if the Var-to-be-replaced has nonempty
* varnullingrels (unless we find below that the replacement expression is
* a Var or PlaceHolderVar that we can just add the nullingrels to). We
- * also need one if the caller has instructed us that all non-Var/PHV
+ * also need one if the caller has instructed us that certain expression
* replacements need to be wrapped for identification purposes.
*/
- need_phv = (var->varnullingrels != NULL) || rcon->wrap_non_vars;
+ need_phv = (var->varnullingrels != NULL) ||
+ (rcon->wrap_option != REPLACE_WRAP_NONE);
/*
* If PlaceHolderVars are needed, we cache the modified expressions in
@@ -2667,7 +2672,12 @@ pullup_replace_vars_callback(Var *var,
{
bool wrap;
- if (varattno == InvalidAttrNumber)
+ if (rcon->wrap_option == REPLACE_WRAP_ALL)
+ {
+ /* Caller told us to wrap all expressions in a PlaceHolderVar */
+ wrap = true;
+ }
+ else if (varattno == InvalidAttrNumber)
{
/*
* Insert PlaceHolderVar for whole-tuple reference. Notice
@@ -2727,11 +2737,6 @@ pullup_replace_vars_callback(Var *var,
}
}
}
- else if (rcon->wrap_non_vars)
- {
- /* Caller told us to wrap all non-Vars in a PlaceHolderVar */
- wrap = true;
- }
else
{
/*
@@ -2763,7 +2768,11 @@ pullup_replace_vars_callback(Var *var,
* This analysis could be tighter: in particular, a non-strict
* construct hidden within a lower-level PlaceHolderVar is not
* reason to add another PHV. But for now it doesn't seem
- * worth the code to be more exact.
+ * worth the code to be more exact. This is also why it's
+ * preferable to handle bare PHVs in the above branch, rather
+ * than this branch. We also prefer to handle bare Vars in a
+ * separate branch, as it's cheaper this way and parallels the
+ * handling of PHVs.
*
* For a LATERAL subquery, we have to check the actual var
* membership of the node, but if it's non-lateral then any
diff --git a/src/backend/rewrite/rewriteManip.c b/src/backend/rewrite/rewriteManip.c
index 98a265cd3d5..1c61085a0a7 100644
--- a/src/backend/rewrite/rewriteManip.c
+++ b/src/backend/rewrite/rewriteManip.c
@@ -1421,7 +1421,7 @@ remove_nulling_relids_mutator(Node *node,
* Note: it might seem desirable to remove the PHV altogether if
* phnullingrels goes to empty. Currently we dare not do that
* because we use PHVs in some cases to enforce separate identity
- * of subexpressions; see wrap_non_vars usages in prepjointree.c.
+ * of subexpressions; see wrap_option usages in prepjointree.c.
*/
/* Copy the PlaceHolderVar and mutate what's below ... */
phv = (PlaceHolderVar *)
diff --git a/src/test/regress/expected/groupingsets.out b/src/test/regress/expected/groupingsets.out
index 449f0384225..35e4cb47ebe 100644
--- a/src/test/regress/expected/groupingsets.out
+++ b/src/test/regress/expected/groupingsets.out
@@ -434,6 +434,35 @@ select x, not x as not_x, q2 from
| | 4567890123456789
(5 rows)
+select x, y
+ from (select four as x, four as y from tenk1) as t
+ group by grouping sets (x, y)
+ having y is null
+ order by 1, 2;
+ x | y
+---+---
+ 0 |
+ 1 |
+ 2 |
+ 3 |
+(4 rows)
+
+select x, y || 'y'
+ from (select four as x, four as y from tenk1) as t
+ group by grouping sets (x, y)
+ order by 1, 2;
+ x | ?column?
+---+----------
+ 0 |
+ 1 |
+ 2 |
+ 3 |
+ | 0y
+ | 1y
+ | 2y
+ | 3y
+(8 rows)
+
-- check qual push-down rules for a subquery with grouping sets
explain (verbose, costs off)
select * from (
diff --git a/src/test/regress/sql/groupingsets.sql b/src/test/regress/sql/groupingsets.sql
index 21cd3121940..38d3cdd0fd8 100644
--- a/src/test/regress/sql/groupingsets.sql
+++ b/src/test/regress/sql/groupingsets.sql
@@ -172,6 +172,17 @@ select x, not x as not_x, q2 from
group by grouping sets(x, q2)
order by x, q2;
+select x, y
+ from (select four as x, four as y from tenk1) as t
+ group by grouping sets (x, y)
+ having y is null
+ order by 1, 2;
+
+select x, y || 'y'
+ from (select four as x, four as y from tenk1) as t
+ group by grouping sets (x, y)
+ order by 1, 2;
+
-- check qual push-down rules for a subquery with grouping sets
explain (verbose, costs off)
select * from (
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index dfe2690bdd3..b9ab9e6e9c9 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2475,6 +2475,7 @@ RepOriginId
ReparameterizeForeignPathByChild_function
ReplaceVarsFromTargetList_context
ReplaceVarsNoMatchOption
+ReplaceWrapOption
ReplicaIdentityStmt
ReplicationKind
ReplicationSlot
--
2.43.0
On Wed, 12 Mar 2025 at 07:45, Richard Guo <guofenglinux@gmail.com> wrote:
I refined the comment in v2 and ended up with:
* This analysis could be tighter: in particular, a non-strict * construct hidden within a lower-level PlaceHolderVar is not * reason to add another PHV. But for now it doesn't seem - * worth the code to be more exact. + * worth the code to be more exact. This is also why it's + * preferable to handle bare PHVs in the above branch, rather + * than this branch. We also prefer to handle bare Vars in a + * separate branch, as it's cheaper this way and parallels the + * handling of PHVs.
LGTM.
For backpatching, 0001 seems more like a cosmetic change, and I don't
think it needs to be backpatched. 0002 is a bug fix, but I'm not
confident enough to commit it to the back branches. Given that there
are other wrong result issues with grouping sets fixed in version 18
but not in earlier versions, and that there are no field reports about
this issue, perhaps it's OK to refrain from backpatching this one?
Yes, I was thinking along the same lines. This particular issue feels
a bit manufactured, and unlikely to occur in practice, but I'm happy
to go with whatever you decide.
Thanks for taking care of this.
Regards,
Dean
On Wed, Mar 12, 2025 at 6:29 PM Dean Rasheed <dean.a.rasheed@gmail.com> wrote:
On Wed, 12 Mar 2025 at 07:45, Richard Guo <guofenglinux@gmail.com> wrote:
For backpatching, 0001 seems more like a cosmetic change, and I don't
think it needs to be backpatched. 0002 is a bug fix, but I'm not
confident enough to commit it to the back branches. Given that there
are other wrong result issues with grouping sets fixed in version 18
but not in earlier versions, and that there are no field reports about
this issue, perhaps it's OK to refrain from backpatching this one?
Yes, I was thinking along the same lines. This particular issue feels
a bit manufactured, and unlikely to occur in practice, but I'm happy
to go with whatever you decide.
Thank you, Dean. I've decided to refrain from backpatching and have
pushed the changes to the master branch. If we receive any complaints
from the fields in the future, we can always perform the backpatching
at that time. Hopefully, by then, this patch will have gone through a
full beta test cycle.
Thanks
Richard