Re: BUG #16095: Segfault while executing trigger

On 2019-11-05 12:38:32 -0500, Tom Lane wrote:
> PG Bug reporting form <noreply@postgresql.org> writes:
> > I'm trying to setup Nominatim and the import seems to consistently fail
> > while updating a table because a postgres process terminates with a
> > segfault.
> 
> Hmm ... can you reproduce this with a smaller test case, by any chance?
> I'm not eager to figure out what Nominatim is, let alone install it.
> 
> It is evidently happening within a BEFORE UPDATE trigger, which is
> interesting but not necessarily relevant.
> 
> > (gdb) bt
> > #0  0x0000560fcc7e0013 in GetMemoryChunkContext (pointer=0x0) at
> > ./build/../src/include/utils/memutils.h:127
> > #1  pfree (pointer=0x0) at ./build/../src/backend/utils/mmgr/mcxt.c:1033
> > #2  0x0000560fcc381a35 in heap_freetuple (htup=<optimized out>) at
> > ./build/../src/backend/access/common/heaptuple.c:1340
> > #3  0x0000560fcc52fe69 in tts_buffer_heap_clear (slot=0x560fce8d8ff0) at
> > ./build/../src/backend/executor/execTuples.c:652
> > #4  0x0000560fcc53022e in ExecClearTuple (slot=0x560fce8d8ff0) at
> > ./build/../src/include/executor/tuptable.h:428
> > #5  ExecResetTupleTable (tupleTable=0x560fcdd30d28, shouldFree=false) at
> > ./build/../src/backend/executor/execTuples.c:1165
> > #6  0x0000560fcc526171 in ExecEndPlan (estate=0x560fcdd2efd0,
> > planstate=<optimized out>) at
> > ./build/../src/backend/executor/execMain.c:1560
> 
> So pretty clearly, this slot has a null bslot->base.tuple pointer and
> yet its TTS_FLAG_SHOULDFREE flag is set.

Hm. It seems quite likely that this is yet another report of:

commit d986d4e87f61c68f52c68ebc274960dc664b7b4e
Author: Andres Freund <andres@anarazel.de>
Date:   2019-10-04 11:59:34 -0700

    Fix crash caused by EPQ happening with a before update trigger present.

The copying of a slot into itself yielded precisely this symptom, a slot
with shouldfree set, but without a tuple associated.


> Wondering about how that could be, I notice that execTuples.c seems
> to have a bad coding pattern of setting TTS_FLAG_SHOULDFREE *before*
> the pointer is valid.  Eg, in tts_buffer_heap_materialize, a failure
> in heap_form_tuple would leave the slot in an inconsistent state.

> I'm not sure that that explains this report, because we typically
> would not run ExecutorEnd on a plan tree that had failed, but I'm
> still strongly inclined to run around and move those flag-setting
> steps down a bit.  Andres, any objection?

No objection here. I don't think it ought to cause an actual problem for
the reason you mention (we'd otherwise run into tons of problems with
slots trying to free tuples that already have been released via memory
contexts), but if the code looks otherwise ok, I see no reason to not
move the flag setting bag.


> I'm also noticing some places that seem pretty much like sloppy
> copy and paste jobs, eg a couple lines further down yet, we have
> 
>         Assert(BufferIsValid(bslot->buffer));
>         if (likely(BufferIsValid(bslot->buffer)))
>             ReleaseBuffer(bslot->buffer);
>         bslot->buffer = InvalidBuffer;
> 
> What's the point of having both an assertion and a run-time test?

Fair question. I think I might just have wanted to make sure we don't
leak a pin, but that's unconvincing, given that we actually have a
mechanism to do so reliably at eoxact.

Greetings,

Andres Freund