On Thu, Jul 15, 2021 at 11:45 PM David Rowley <dgrowle...@gmail.com> wrote: > > On Fri, 16 Jul 2021 at 02:53, Ronan Dunklau <ronan.dunk...@aiven.io> wrote: > > Please find attached a v9 just moving the flag setting to ExecInitSort, and > > my > > apologies if I misunderstood your point. > > I took this and adjusted a few things and ended up with the attached patch. > > The changes are fairly minor. I made the bracing consistent between > both tuplesort_begin calls. I rewrote the comment at the top of > ExecSort() to make it more clear about each method used. > > I also adjusted the comment down at the end of ExecSort that was > mentioning something about tuplesort_gettupleslot returning NULL. > Your patch didn't touch this, but to me, the comment just looked wrong > both before and after the changes. tuplesort_gettupleslot returns > false and sets the slot to empty when it runs out of tuples. Anyway, > I wrote something there that I think improves that. > > I feel like this patch is commit-worthy now. However, I'll leave it > for a few days, maybe until after the weekend as there's been a fair > bit of interest and I imagine someone will have comments to make.
The only remaining question I have is whether or not costing needs to change, given the very significant speedup for datum sort. James
