Re: tuplesort_gettuple_common() and *should_free argument

On 10/22/2016 02:45 AM, Peter Geoghegan wrote:
> I noticed that the routine tuplesort_gettuple_common() fails to set
> *should_free in all paths in master branch (no bug in 9.6). Consider
> the TSS_SORTEDONTAPE case, where we can return false without also
> setting "*should_free = false" to instruct caller not to pfree() tuple
> memory that tuplesort still owns. I suppose that this is okay because
> caller should never pfree() a tuple when NULL pointer was returned at
> higher level (as "pointer-to-tuple"). Even still, it seems like a bad
> idea to rely on caller testing things such that caller doesn't go on
> to pfree() a NULL pointer when seemingly instructed to do so by
> tuplesort "get tuple" interface routine (that is, some higher level
> routine that calls tuplesort_gettuple_common()).
>
> More importantly, there are no remaining cases where
> tuplesort_gettuple_common() sets "*should_free = true", because there
> is no remaining need for caller to *ever* pfree() tuple. Moreover, I
> don't anticipate any future need for this -- the scheme recently
> established around per-tape "slab slots" makes it seem unlikely to me
> that the need to "*should_free = true" will ever arise again. So, for
> Postgres 10, why not rip out the "*should_free" arguments that appear
> in a bunch of places? This lets us slightly simplify most tuplesort.c
> callers.

Yeah, I agree we should just remove the *should_free arguments.

Should we be worried about breaking the API of tuplesort_get* functions? 
They might be used by extensions. I think that's OK, but wanted to bring 
it up. This would be only for master, of course, and any breakage would 
be straightforward to fix.

> Now, it is still true that at least some callers to
> tuplesort_gettupleslot() (but not any other "get tuple" interface
> routine) are going to *require* ownership of memory for returned
> tuples, which means a call to heap_copy_minimal_tuple() remains
> necessary there (see recent commit
> d8589946ddd5c43e1ebd01c5e92d0e177cbfc198 for details). But, that's
> beside the point. In the master branch only, the
> tuplesort_gettupleslot() test "if (!should_free)" seems tautological,
> just as similar tests are for every other tuplesort_gettuple_common()
> caller. So, tuplesort_gettupleslot() can safely assume it should
> *always* heap_copy_minimal_tuple() in the master branch. (BTW, I'm
> going to teach tuplesort_gettuple_common() to avoid this
> heap_copy_minimal_tuple() call for callers that don't actually need
> it, but that's a discussion for another thread).

Yep.

- Heikki