Re: Wrong results with grouping sets

Hi! Thank you for your work on the subject.

I think I've come across a wrong result issue with grouping sets, as
shown by the query below.

-- result is correct with only grouping sets
select a, b
from (values (1, 1), (2, 2)) as t (a, b) where a = b
group by grouping sets((a, b), (a));
 a | b
---+---
 1 | 1
 1 |
 2 | 2
 2 |
(4 rows)

-- result is NOT correct with grouping sets and distinct on
select distinct on (a, b) a, b
from (values (1, 1), (2, 2)) as t (a, b) where a = b
group by grouping sets((a, b), (a));
 a | b
---+---
 1 | 1
 2 | 2
(2 rows)

The distinct on expressions include both 'a' and 'b', so rows (1, 1) and
(1, NULL) should not be considered equal.  (The same for rows (2, 2) and
(2, NULL).)

I noticed that this query worked correctly in the main branch with the inequality operator:

postgres=# select distinct on (a, b) a, b from (values (3, 1), (2, 2)) as t (a, b) where a > b group by grouping sets((a, b), (a)); a | b ---+--- 3 | 1 3 | (2 rows)

So, I think you are right)

I think the root cause is that when we generate distinct_pathkeys, we
failed to realize that Var 'b' might be nullable by the grouping sets,
so it's no longer always equal to Var 'a'.  It's not correct to deem
that the PathKey for 'b' is redundant and thus remove it from the
pathkeys list.

We have the same issue when generating sort_pathkeys.  As a result, we
may have the final output in the wrong order.  There were several
reports about this issue before, such as [1][2].

To fix this issue, I'm thinking that we mark the grouping expressions
nullable by grouping sets with a dummy RTE for grouping sets, something
like attached.  In practice we do not need to create a real RTE for
that, what we need is just a RT index.  In the patch I use 0, because
it's not a valid outer join relid, so it would not conflict with the
outer-join-aware-Var infrastructure.

If the grouping expression is a Var or PHV, we can just set its
nullingrels, very straightforward.   For an expression that is neither a
Var nor a PHV, I'm not quite sure how to set the nullingrels.  I tried
the idea of wrapping it in a new PHV to carry the nullingrels, but that
may cause some unnecessary plan diffs.  In the patch for such an
expression I just set the nullingrels of Vars or PHVs that are contained
in it.  This is not really 'correct' in theory, because it is the whole
expression that can be nullable by grouping sets, not its individual
vars.  But it works in practice, because what we need is that the
expression can be somehow distinguished from the same expression in ECs,
and marking its vars is sufficient for this purpose.  But what if the
expression is variable-free?  This is the point that needs more work.
Fow now the patch just handles variable-free expressions of type Const,
by wrapping it in a new PHV.

There are some places where we need to artificially remove the RT index
of grouping sets from the nullingrels, such as when we generate
PathTarget for initial input to grouping nodes, or when we generate
PathKeys for the grouping clauses, because the expressions there are
logically below the grouping sets.  We also need to do that in
set_upper_references when we update the targetlist of an Agg node, so
that we can perform exact match for nullingrels, rather than superset
match.

Since the fix depends on the nullingrels stuff, it seems not easy for
back-patching.  I'm not sure what we should do in back branches.

Any thoughts?

[1] https://www.postgresql.org/message-id/CAMbWs48AtQTQGk37MSyDk_EAgDO3Y0iA_LzvuvGQ2uO_Wh2muw@mail.gmail.com
[2] https://www.postgresql.org/message-id/17071-24dc13fbfa29672d@postgresql.org

I looked at your patch and noticed a few things:

1. I think you should add a test with the cube operator, because I noticed that the order of the query in the result has also changed:

master:
postgres=# select a,b from (values(1,1),(2,2)) as t (a,b) where a=b group by cube (a, (a,b)) order by b,a; a | b ---+--- 1 | 1 1 | 1 1 | 2 | 2 2 | 2 2 | | (7 rows)

with patch:

postgres=# select a, b from (values (1, 1), (2, 2)) as t (a, b) where a = b group by cube(a, (a, b)) order by b,a; a | b ---+--- 1 | 1 1 | 1 2 | 2 2 | 2 1 | 2 | | (7 rows)

-- 
Regards,
Alena Rybakina