From: | Andres Freund <andres(at)anarazel(dot)de> |
---|---|
To: | Peter Geoghegan <pg(at)bowt(dot)ie> |
Cc: | Peter Geoghegan <pg(at)heroku(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Robert Haas <robertmhaas(at)gmail(dot)com>, hlinnaka <hlinnaka(at)iki(dot)fi>, Pg Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: tuplesort_gettuple_common() and *should_free argument |
Date: | 2017-04-06 21:05:30 |
Message-ID: | 20170406210530.pstrgm6bnqeirp7p@alap3.anarazel.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 2017-04-04 13:49:11 -0700, Peter Geoghegan wrote:
> On Tue, Apr 4, 2017 at 1:32 PM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> >> static bool
> >> tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
> >> @@ -2091,12 +2092,15 @@ tuplesort_gettuple_common(Tuplesortstate *state, bool forward,
> >> * NULL value in leading attribute will set abbreviated value to zeroed
> >> * representation, which caller may rely on in abbreviated inequality check.
> >> *
> >> - * The slot receives a copied tuple (sometimes allocated in caller memory
> >> - * context) that will stay valid regardless of future manipulations of the
> >> - * tuplesort's state.
> >> + * If copy is TRUE, the slot receives a copied tuple that will stay valid
> >> + * regardless of future manipulations of the tuplesort's state. Memory is
> >> + * owned by the caller. If copy is FALSE, the slot will just receive a
> >> + * pointer to a tuple held within the tuplesort, which is more efficient,
> >> + * but only safe for callers that are prepared to have any subsequent
> >> + * manipulation of the tuplesort's state invalidate slot contents.
> >> */
> >
> > Hm. Do we need to note anything about how long caller memory needs to
> > live? I think we'd get into trouble if the caller does a memory context
> > reset, without also clearing the slot?
>
> Since we arrange to have caller explicitly own memory (it's always in
> their own memory context (current context), which is not the case with
> other similar routines), it's 100% the caller's problem. As I assume
> you know, convention in executor around resource management of memory
> like this is pretty centralized within ExecStoreTuple(), and nearby
> routines. In general, the executor is more or less used to having this
> be its problem alone, and is pessimistic about memory lifetime unless
> otherwise noted.
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.
Now, that's obviously not a problem for existing code, because it worked
that way for a long time - I just was wondering whether that needs to be
called out.
- Andres
From | Date | Subject | |
---|---|---|---|
Next Message | Claudio Freire | 2017-04-06 21:05:41 | Re: Making clausesel.c Smarter |
Previous Message | Tomas Vondra | 2017-04-06 21:05:21 | Re: increasing the default WAL segment size |