Re: [HACKERS] tuplesort_gettuple_common() and *should_free argument

On 2017-04-04 13:49:11 -0700, Peter Geoghegan wrote:
> On Tue, Apr 4, 2017 at 1:32 PM, Andres Freund <andres@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 thenif (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