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 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_wh2...@mail.gmail.com [2] https://www.postgresql.org/message-id/17071-24dc13fbfa296...@postgresql.org Thanks Richard
v1-0001-Mark-expressions-nullable-by-grouping-sets.patch
Description: Binary data