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. 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). -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
