Re: Add proper planner support for ORDER BY / DISTINCT aggregates

On Mon, Jul 5, 2021 at 8:08 AM Ronan Dunklau <ronan(dot)dunklau(at)aiven(dot)io> wrote:
>
> > Ok, I reproduced that case, just not using a group by: by adding the group
> > by a sort node is added in both cases (master and your patch), except that
> > with your patch we sort on both keys and that doesn't really incur a
> > performance penalty.
> >
> > I think the overhead occurs because in the ExecAgg case, we use the
> > tuplesort_*_datum API as an optimization when we have a single column as an
> > input, which the ExecSort code doesn't. Maybe it would be worth it to try to
> > use that API in sort nodes too, when it can be done.
>
> Please find attached a POC patch to do just that.
>
> The switch to the single-datum tuplesort is done when there is only one
> attribute, it is byval (to avoid having to deal with copy of the references
> everywhere) and we are not in bound mode (to also avoid having to move things
> around).
>
> A naive run on make check pass on this, but I may have overlooked things.
>
> Should I add this separately to the commitfest ?

It seems like a pretty obvious win on its own, and, I'd expect, will
need less discussion than David's patch, so my vote is to make it a
separate thread. The patch tester wants the full series attached each
time, and even without that it's difficult to discuss multiple patches
on a single thread.

If you make a separate thread and CF entry, please CC me and add me as
a reviewer on the CF entry.

One thing from a quick read through of the patch: your changes near
the end of ExecSort, in ExecInitSort, and in execnodes.h need
indentation fixes.

Thanks,
James