Thread: Wrong results with subquery pullup and grouping sets

While working on the expansion of virtual generated columns, Dean
encountered $subject in [1], 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] https://postgr.es/m/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