Re: BUG #16095: Segfault while executing trigger

PG Bug reporting form <noreply(at)postgresql(dot)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.

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?

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?

regards, tom lane