On Mon, Jul 5, 2021 at 8:08 AM Ronan Dunklau <ronan.dunk...@aiven.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
