Re: Wrong results with grouping sets

On Thu, 7 Dec 2023 at 13:52, Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>
>
> On Mon, Sep 25, 2023 at 3:11 PM Richard Guo <guofenglinux(at)gmail(dot)com> wrote:
>>
>> 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.
>
>
> For a variable-free expression, if it contains volatile functions, SRFs,
> aggregates, or window functions, it would not be treated as a member of
> EC that is redundant (see get_eclass_for_sort_expr()). That means it
> would not be removed from the pathkeys list, so we do not need to set
> the nullingrels for it. Otherwise we can just wrap the expression in a
> PlaceHolderVar. Attached is an updated patch to do that.
>
> BTW, this wrong results issue has existed ever since grouping sets was
> introduced in v9.5, and there were field reports complaining about this
> issue. I think it would be great if we can get rid of it. I'm still
> not sure what we should do in back branches. But let's fix it at least
> on v16 and later.

I have changed the status of the patch to "Waiting on Author" as Tom
Lane's comments have not yet been addressed, feel free to address them
and update the commitfest entry accordingly.

Regards,
Vignesh