On Wed, Feb 7, 2018 at 4:41 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter Geoghegan <pg@bowt.ie> writes:
>> It would be nice to get an opinion on this mode_final() + tuplesort
>> memory lifetime business from you, Tom.
>
> I'm fairly sure that that bit in mode_final() was just a hack to make
> it work. If there's a better, more principled way, let's go for it.
This is the more principled way. It is much easier to make every
single tuplesort caller on every release branch follow this rule (or
those on 9.5+, at least).
My big concern with making tuplesort_getdatum() deliberately copy into
caller's context is that that could introduce memory leaks in caller
code that evolved in a world where tuplesort_end() frees
pass-by-reference datum memory. Seems like the only risky case is
certain ordered set aggregate code, though. And, it's probably just a
matter of fixing the comments. I'll read through the bug report thread
that led up to October's commit be0ebb65 [1] tomorrow, to make sure of
this.
I just noticed that process_ordered_aggregate_single() says that the
behavior I propose for tuplesort_getdatum() is what it *already*
expects:
/*
* Note: if input type is pass-by-ref, the datums returned by the sort are
* freshly palloc'd in the per-query context, so we must be careful to
* pfree them when they are no longer needed.
*/
This supports the idea that ordered set aggregate code is the odd one
out when it comes to beliefs about tuplesort memory contexts. Even
just among tuplesort_getdatum() callers, though even more so among
tuplesort callers in general. One simple rule for all tuplesort fetch
routines is the high-level goal here.
>> Note that you removed the quoted comment within be0ebb65, back in October.
>
> There were multiple instances of basically that same comment before.
> AFAICS I just consolidated them into one place, in the header comment for
> ordered_set_shutdown.
I see. So, to restate my earlier remarks in terms of the newer
organization: The last line in that header comment will need to be
removed, since it will become false under my scheme. The line is:
"Note that many of the finalfns could *not* free the tuplesort object,
at least not without extra data copying, because what they return is a
pointer to a datum inside the tuplesort object".
[1]
https://www.postgresql.org/message-id/flat/CAB4ELO5RZhOamuT9Xsf72ozbenDLLXZKSk07FiSVsuJNZB861A%40mail.gmail.com#CAB4ELO5RZhOamuT9Xsf72ozbenDLLXZKSk07FiSVsuJNZB861A@mail.gmail.com
--
Peter Geoghegan
