On Wed, Jan 17, 2018 at 1:00 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: >> You could make the same objection to changing tuplesort_getdatum() >> outside of the master branch, though. I think that going back further >> than that for the (arguably independent) tuplesort_getdatum() subset >> fix might still be a good idea. I wonder where you stand on this. > > I haven't been following the thread very closely, so I don't have an > opinion on that.
A complicating factor for this fix of mine is that mode_final() seems to have its own ideas about tuple memory lifetime, over and above what tuplesort_getdatum() explicitly promises, as can be seen here: /* * Note: we *cannot* clean up the tuplesort object here, because the value * to be returned is allocated inside its sortcontext. We could use * datumCopy to copy it out of there, but it doesn't seem worth the * trouble, since the cleanup callback will clear the tuplesort later. */ My WIP-tuplesort-memcontext-fix.patch fix is premised on the idea that nodeAgg.c/grouping sets got it right: nodeAgg.c should be able to continue to assume that in "owning" the memory used for a tuple (in a table slot), it has it in its own memory context -- otherwise, the whole tts_shouldFree tuple slot mechanism is prone to double-frees. This comment directly contradicts/undermines that premise. ISTM that either grouping sets or mode_final() must necessarily be wrong, because each oversteps, and infers a different contract from tuplesort tuple fetching routines (different assumptions about memory contexts are made in each case). Only one can be right, unless it's okay to have one rule for tuplesort_getdatum() and another for tuplesort_gettupleslot() (which seems questionable to me). I still think that grouping sets is right (and that mode_final() is wrong). Do you? -- Peter Geoghegan
