Re: tuplesort_gettuple_common() and *should_free argument

On Fri, Oct 21, 2016 at 4:45 PM, Peter Geoghegan <pg(at)heroku(dot)com> wrote:
> 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.

Attached patch 0001-* removes all should_free arguments. To reiterate,
this is purely a refactoring patch.

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

I decided that it made sense to address this at the same time after
all. So, the second patch attached here, 0002-*, goes on to do so.
This is a performance optimization that we already agreed was a good
idea for Postgres 10 when d8589946ddd5c43e1ebd01c5e92d0e177cbfc198
went in.

Note that the call sites that now opt out of receiving their own
personal copy are essentially returning to their behavior for the
entire beta phase of PostgreSQL 9.6. The problem with that accidental
state of affairs was that it wasn't always safe. But, it was still
generally safe for the majority of callers, I believe, not least
because it took months to hear any complaints at all. That majority of
callers now opt out.

The main question for reviewers of 0002-*, then, is whether or not I
have correctly determined which particular callers can safely opt out.

Finally, 0003-* is a Valgrind suppression borrowed from my parallel
CREATE INDEX patch. It's self-explanatory.

--
Peter Geoghegan