On Wed, Jan 17, 2018 at 1:00 PM, Tom Lane <tgl@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