On Wed, Feb 7, 2018 at 4:41 PM, Tom Lane <t...@sss.pgh.pa.us> wrote: > Peter Geoghegan <p...@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#cab4elo5rzhoamut9xsf72ozbendllxzksk07fisvsujnzb8...@mail.gmail.com -- Peter Geoghegan
