Re: POC: GROUP BY optimization

On Sun, Jan 14, 2024 at 2:14 PM Andrei Lepikhov
<a(dot)lepikhov(at)postgrespro(dot)ru> wrote:
> On 13/1/2024 22:00, Alexander Korotkov wrote:
> > On Sat, Jan 13, 2024 at 11:09 AM Andrei Lepikhov
> > <a(dot)lepikhov(at)postgrespro(dot)ru> wrote:
> >> On 11/1/2024 18:30, Alexander Korotkov wrote:
> >>> On Tue, Jan 9, 2024 at 1:14 PM Pavel Borisov <pashkin(dot)elfe(at)gmail(dot)com> wrote:
> >>>>> Hmm, I don't see this old code in these patches. Resend 0002-* because
> >>>>> of trailing spaces.
> >>>>
> >>>>
> >>>> AFAIK, cfbot does not seek old versions of patchset parts in previous messages. So for it to run correctly, a new version of the whole patchset should be sent even if most patches are unchanged.
> >>>
> >>> Please, find the revised patchset with some refactoring and comments
> >>> improvement from me. I'll continue looking into this.
> >> The patch looks better, thanks to your refactoring.
> >> I propose additional comments and tests to make the code more
> >> understandable (see attachment).
> >> I intended to look into this part of the code more, but the tests show a
> >> difference between PostgreSQL v.15 and v.16, which causes changes in the
> >> code of this feature.
> >
> > Makes sense. I've incorporated your changes into the patchset.
> One more improvement. To underpin code change:
>
> - return cur_ec; /* Match! */
> + {
> + /*
> + * Match!
> + *
> + * Copy the sortref if it wasn't set yet. That may happen if
> + * the ec was constructed from WHERE clause, i.e. it doesn't
> + * have a target reference at all.
> + */
> + if (cur_ec->ec_sortref == 0 && sortref > 0)
> + cur_ec->ec_sortref = sortref;
> + return cur_ec;
> + }
>
> I propose the test (see attachment). It shows why we introduce this
> change: GROUP-BY should juggle not only pathkeys generated by explicit
> sort operators but also planner-made, likewise in this example, by
> MergeJoin.

Thank you for providing the test case relevant for this code change.
The revised patch incorporating this change is attached. Now the
patchset looks good to me. I'm going to push it if there are no
objections.

------
Regards,
Alexander Korotkov