Re: tuplesort_gettuple_common() and *should_free argument

On Wed, Jan 25, 2017 at 2:49 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I looked at the 0002 patch, and while the code is probably OK, I am
> dissatisfied with this API spec:
>
> + * 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 may just receive a pointer
> + * to a tuple held within the tuplesort. The latter is more efficient, but
> + * the slot contents may be corrupted if there is another call here before
> + * previous slot contents are used.
>
> What does "here" mean? If that means specifically "another call of
> tuplesort_gettupleslot", say so. If "here" refers to the whole module,
> it would be better to say something like "the slot contents may be
> invalidated by any subsequent manipulation of the tuplesort's state".
> In any case it'd be a good idea to delineate safe usage patterns, perhaps
> "copy=FALSE is recommended only when the next tuplesort manipulation will
> be another tuplesort_gettupleslot fetch into the same slot."

I agree with your analysis.

It means "another call to tuplesort_gettupleslot", but I believe that
it would be safer (more future-proof) to actually specify "the slot
contents may be invalidated by any subsequent manipulation of the
tuplesort's state" instead.

> There are several other uses of "call here", both in this patch and
> pre-existing in tuplesort.c, that I find equally vague and unsatisfactory.
> Let's try to improve that.

Should I write a patch along those lines?

--
Peter Geoghegan