Re: tuplesort_gettuple_common() and *should_free argument

On Tue, Apr 4, 2017 at 1:32 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> s/Avoid/Allow to avoid/

WFM.

>> + *
>> + * Callers cannot rely on memory for tuple in returned slot remaining valid
>> + * past any subsequent manipulation of the sorter, such as another fetch of
>> + * input from sorter. (The sorter may recycle memory.)
>> */
>
> It's not just the sorter, right? I'd just rephrase that the caller
> cannot rely on tuple to stay valid beyond a single call.

It started that way, but changed on the basis of Tom's feedback. I
would be equally happy with either wording.

>> 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.

> Other than these minimal adjustments, this looks good to go to me.

Cool.

I'll try to get out a revision soon, maybe later today, including an
updated 0002-* (Valgrind suppression), which I have not forgotten
about.

--
Peter Geoghegan