Re: BUG #14344: string_agg(DISTINCT ..) crash

On Thu, Sep 29, 2016 at 10:08 AM, Heikki Linnakangas  wrote:
> The comments in those other routines explicitly say that "If it is not set
> [*should_free], caller should not use tuple following next call here". I
> also didn't notice that tuplesort_gettupleslot doesn't contain say that,
> until now.

They say that because the patch with the bug in question made them say
that. The fact that tuplesort_gettupleslot() did not have such a
comment added at the same time (and, more importantly, verification
that that would be okay for all existing callers) was arguably the
oversight in that patch/commit. That said, it probably wouldn't have
been a good idea to make it work. There is an implicit requirement for
tuplesort_gettupleslot() alone around the ownership of memory, per Tom
-- caller wants ownership of the memory returned. This is confused by
the fact that the memory is and was always allocated in sortcontext
memory context. So, caller wants ownership of the memory, but gets it
in the sortcontext. It looks like the callers sometimes ultimately
call ExecCopySlotTuple() again, just to be safe (e.g.,
agg_retrieve_direct()), possibly because the contained-in-slot tuple
may need to outlive the tuplesort itself (which represent the entire
sort operation's lifetime).

> That's unfortunate. AFAICS, we have no choice but palloc(), when
> tuplesort_gettupleslot() is used. For version 10, maybe we should reconsider
> that API.

We have to do something to fix this bug, and performing an extra
retail palloc() looks like the easiest solution. This does not
actually defeat the purpose of batch memory, which was mostly about
cache efficiency. I tend to think that the possible uses of the slot
by tuplesort_gettupleslot() callers are too complex to tie down.

-- 
Peter Geoghegan