On Thu, Apr 6, 2017 at 2:05 PM, Andres Freund <andres@anarazel.de> wrote:
> I'm not sure you entirely got my point here. If tuplesort_gettupleslot
> is called with copy = true, it'll store that tuple w/
> ExecStoreMinimalTuple passing shouldFree = copy = true. If the caller
> is in a short-lived context, e.g. some expression context, and resets
> that context before the slot is cleared (either by storing another tuple
> or ExecClearTuple()) you'll get a double free, because the context has
> freed the tuple in bulk, and then
> if (slot->tts_shouldFree)
> heap_freetuple(slot->tts_tuple);
> will do its work.
Calling ExecClearTuple() will mark the slot "tts_shouldFree = false"
-- you only have a problem when a memory context is reset, which
obviously cannot be accounted for by ExecClearTuple(). ISTM that
ResetExprContext() is kind of called to "make sure" that memory is
freed in a short-lived expression context, at a level up from any
ExecStoreMinimalTuple()/ExecClearTuple() style memory management. The
conventions are not centrally documented, AFAIK, but this still seems
natural enough to me. Per-tuple contexts tend to be associated with
expression contexts.
In any case, I'm not sure where you'd centrally document the
conventions. Although, it seems clear that it wouldn't be anywhere
this patch currently touches. The executor README, perhaps?
--
Peter Geoghegan
VMware vCenter Server
https://www.vmware.com/
