On 2017-04-04 13:49:11 -0700, Peter Geoghegan wrote: > On Tue, Apr 4, 2017 at 1:32 PM, Andres Freund <and...@anarazel.de> wrote: > >> static bool > >> tuplesort_gettuple_common(Tuplesortstate *state, bool forward, > >> @@ -2091,12 +2092,15 @@ tuplesort_gettuple_common(Tuplesortstate *state, > >> bool forward, > >> * NULL value in leading attribute will set abbreviated value to zeroed > >> * representation, which caller may rely on in abbreviated inequality > >> check. > >> * > >> - * The slot receives a copied tuple (sometimes allocated in caller memory > >> - * context) that will stay valid regardless of future manipulations of the > >> - * tuplesort's state. > >> + * If copy is TRUE, the slot receives a copied tuple that will stay valid > >> + * regardless of future manipulations of the tuplesort's state. Memory is > >> + * owned by the caller. If copy is FALSE, the slot will just receive a > >> + * pointer to a tuple held within the tuplesort, which is more efficient, > >> + * but only safe for callers that are prepared to have any subsequent > >> + * manipulation of the tuplesort's state invalidate slot contents. > >> */ > > > > Hm. Do we need to note anything about how long caller memory needs to > > live? I think we'd get into trouble if the caller does a memory context > > reset, without also clearing the slot? > > Since we arrange to have caller explicitly own memory (it's always in > their own memory context (current context), which is not the case with > other similar routines), it's 100% the caller's problem. As I assume > you know, convention in executor around resource management of memory > like this is pretty centralized within ExecStoreTuple(), and nearby > routines. In general, the executor is more or less used to having this > be its problem alone, and is pessimistic about memory lifetime unless > otherwise noted.
I'm not sure you entirely got my point here. If tuplesort_gettupleslot is called with copy = true, it'll store that tuple w/ ExecStoreMinimalTuple passing shouldFree = copy = true. If the caller is in a short-lived context, e.g. some expression context, and resets that context before the slot is cleared (either by storing another tuple or ExecClearTuple()) you'll get a double free, because the context has freed the tuple in bulk, and then if (slot->tts_shouldFree) heap_freetuple(slot->tts_tuple); will do its work. Now, that's obviously not a problem for existing code, because it worked that way for a long time - I just was wondering whether that needs to be called out. - Andres -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers