Re: PostgreSQL crashes with SIGSEGV

On Wed, Feb 7, 2018 at 7:02 PM, Peter Geoghegan <pg(at)bowt(dot)ie> wrote:
> On Wed, Feb 7, 2018 at 4:41 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Peter Geoghegan <pg(at)bowt(dot)ie> writes:
>>> It would be nice to get an opinion on this mode_final() + tuplesort
>>> memory lifetime business from you, Tom.
>>
>> I'm fairly sure that that bit in mode_final() was just a hack to make
>> it work. If there's a better, more principled way, let's go for it.
>
> This is the more principled way. It is much easier to make every
> single tuplesort caller on every release branch follow this rule (or
> those on 9.5+, at least).

I now think that my approach to fixing 9.6 in
WIP-tuplesort-memcontext-fix.patch is too complicated to justify
backpatching. I had the right idea there, and have no reason to think
it won't work, but it now seems like the complexity simply isn't worth
it. The advantage of WIP-tuplesort-memcontext-fix.patch was that it
avoided an extra copy within tuplesort_gettupleslot() on the earlier
Postgres versions it targeted (the versions where that function does
not have a "copy" argument), by arranging to make sure that low-level
routines have tuplesort caller context passed all the way down.
However, now that I consider the frequency that
WIP-tuplesort-memcontext-fix.patch would avoid such copying given real
9.6 workloads, its approach looks rather unappealing -- we should
instead just do a copy in all cases.

Another way of putting it is that it now seems like the approach taken
in bugfix commit d8589946d should be taken even further for 9.6, so
that we *always* copy for the tuplesort_gettupleslot() caller, rather
than just copying in the most common cases. We'd also sometimes have
to free the redundant memory allocated by tuplesort_gettuple_common()
within tuplesort_gettupleslot() if we went this way -- the should_free
= true case would have tuplesort_gettuple_common() do a pfree() after
copying. Needing a pfree() is a consequence of allocating memory for
caller, and then copying it for caller when we know that we're using
caller's memory context. A bit weird, but certainly very simple.

New plan:

* For 9.5 and 9.6, the approach taken in bugfix commit d8589946d
should be taken even further -- we should always copy. Moreover, we
should always copy within tuplesort_getdatum(), for the same reasons.

* For 9.5, 9.6, 10, and master, we should make sure that
tuplesort_getdatum() uses the caller's memory context. The fact that
it doesn't already do so seems like a simple oversight. We should do
this to be consistent with tuplesort_gettupleslot(). (This isn't
critical, but seems like a good idea.)

* For 9.5, 9.6, 10, and master, we should adjust some comments from
tuplesort_getdatum() callers, so that they no longer say that
tuplesort datum tuple memory lives in tuplesort context. That won't be
true anymore.

Anyone have an opinion on this?

The advantages of this approach are:

- It's far simpler than WIP-tuplesort-memcontext-fix.patch, and can be
applied to 9.5 and 9.6 with only small adjustments.

- It leaves all branches essentially consistent with v10+. v10+ gets
everything right already (except for that one minor
tuplesort_getdatum() + caller context issue), and it seems sensible to
treat v10 as a kind of model to follow here.

There are also some disadvantages for this new plan, though:

- There is a slightly awkward question for tuplesort_getdatum() in
9.6: Is tuplesort_getdatum() *always* explicitly copying an acceptable
overhead, given that tuplesort_getdatum() is not known to cause a
crash? I doubt so myself, since tuplesort_getdatum() *always* copies
on Postgres v10+ anyway, and even on 9.6 copying is already the common
case.

- There is a new overhead in 9.5. As I said, 9.6 mostly already copies
anyway, since it already has d8589946d -- 9.5 never got that commit.
This is very similar to the situation we faced about a year ago with
d8589946d on 9.6, since there isn't going to be much extra copying
than the copying that d8589946d already implies. ISTM that d8589946d
set a precedent that makes the situation that this creates for 9.5
okay today.

--
Peter Geoghegan