Re: BUG #16293: postgres segfaults and returns SQLSTATE 08006

Hi,

On 2020-03-14 14:26:57 -0700, Andres Freund wrote:
> On 2020-03-14 17:21:04 -0400, Tom Lane wrote:
> > Interesting. I do *not* see a crash in v12 or HEAD, but v11 gives
> > me an assertion failure:
> >
> > TRAP: FailedAssertion("!(!slot->tts_fixedTupleDescriptor)", File: "execTuples.c", Line: 284)
> > 2020-03-14 17:16:06.626 EDT [1101] LOG: server process (PID 20822) was terminated by signal 6: Aborted
> > 2020-03-14 17:16:06.626 EDT [1101] DETAIL: Failed process was running: INSERT INTO parent_table VALUES ('2020-02-02 01:00:00+00:00', 'f');
> >
> > here:
> >
> > #3 0x000000000063ef45 in ExecSetSlotDescriptor (slot=0x1cf4060,
> > tupdesc=0x7f903a2dd0f8) at execTuples.c:284
> > #4 0x00000000006360eb in ExecConstraints (resultRelInfo=0x1cf47b8,
> > slot=0x1cf4060, estate=0x1cf3778) at execMain.c:2062
> > #5 0x000000000065a44d in ExecInsert (mtstate=0x1cf3ae0, slot=0x1cf4060,
> > planSlot=0x1cf4060, estate=0x1cf3778, canSetTag=true)
> > at nodeModifyTable.c:398
> > #6 0x000000000065b80b in ExecModifyTable (pstate=<value optimized out>)
> > at nodeModifyTable.c:2159
> > #7 0x0000000000636c27 in ExecProcNode (queryDesc=0x1ce51e8,
> > direction=<value optimized out>, count=0, execute_once=224)
> > at ../../../src/include/executor/executor.h:247
> > #8 ExecutePlan (queryDesc=0x1ce51e8, direction=<value optimized out>,
> > count=0, execute_once=224) at execMain.c:1723
> > #9 standard_ExecutorRun (queryDesc=0x1ce51e8,
> > direction=<value optimized out>, count=0, execute_once=224)
> >
> > So this looks like something to do with Andres' tupletableslot work.
>
> Interesting. There wasn't that much in v11. It's not the general rework,
> but just the allocation of the descriptor together slot when the
> descriptor is known at creation time - or a conflict with concurrent
> partitioning work. It's plausible that enough changed in master for
> that to not be a problem anymore.

Hm. So the relevant slot is created at (huray for rr making it trivial
to determine that):

#0 0x000055d6c1b127c6 in MakeTupleTableSlot (tupleDesc=0x55d6c3dd64f8) at /home/andres/src/postgresql-11/src/backend/executor/execTuples.c:135
#1 0x000055d6c1b12895 in ExecAllocTableSlot (tupleTable=0x55d6c3dd5dd8, desc=0x55d6c3dd64f8)
at /home/andres/src/postgresql-11/src/backend/executor/execTuples.c:169
#2 0x000055d6c1b1389a in ExecInitResultTupleSlotTL (estate=0x55d6c3dd5d28, planstate=0x55d6c3dd62e8)
at /home/andres/src/postgresql-11/src/backend/executor/execTuples.c:907
#3 0x000055d6c1b3e98b in ExecInitResult (node=0x55d6c3de2638, estate=0x55d6c3dd5d28, eflags=0)
at /home/andres/src/postgresql-11/src/backend/executor/nodeResult.c:220
#4 0x000055d6c1b0e690 in ExecInitNode (node=0x55d6c3de2638, estate=0x55d6c3dd5d28, eflags=0)
at /home/andres/src/postgresql-11/src/backend/executor/execProcnode.c:164
#5 0x000055d6c1b3beec in ExecInitModifyTable (node=0x55d6c3d0cdb0, estate=0x55d6c3dd5d28, eflags=0)
at /home/andres/src/postgresql-11/src/backend/executor/nodeModifyTable.c:2304
#6 0x000055d6c1b0e6ce in ExecInitNode (node=0x55d6c3d0cdb0, estate=0x55d6c3dd5d28, eflags=0)
at /home/andres/src/postgresql-11/src/backend/executor/execProcnode.c:174
#7 0x000055d6c1b04c21 in InitPlan (queryDesc=0x55d6c3dec888, eflags=0) at /home/andres/src/postgresql-11/src/backend/executor/execMain.c:1046

I don't think it's ok for ExecConstraint() to overwrite the tuple
descriptor of a slot "owned" by nodeResult.c. Am I missing something, or
is that broken?

In v12+ that problem doesn't exist, because we simply allocate a new
slot (this is just for printing an error message).

There's other places using ExecSetSlotDescriptor(), e.g.
else if (resultRelInfo->ri_FdwRoutine)
{
HeapTuple tuple;

/*
* delete from foreign table: let the FDW do it
*
* We offer the trigger tuple slot as a place to store RETURNING data,
* although the FDW can return some other slot if it wants. Set up
* the slot's tupdesc so the FDW doesn't need to do that for itself.
*/
slot = estate->es_trig_tuple_slot;
if (slot->tts_tupleDescriptor != RelationGetDescr(resultRelationDesc))
ExecSetSlotDescriptor(slot, RelationGetDescr(resultRelationDesc));

slot = resultRelInfo->ri_FdwRoutine->ExecForeignDelete(estate,
resultRelInfo,
slot,
planSlot);

but that's different, because it's using a slot (kind of) owned by the
modify node, rather just a random subsidiary node's slot.

I think the only reason this issue doesn't have actually bad
consequences is that we throw an error afterwards - which'll result in
the executor tree not being used any further.

I'm inclined to simply copy v12's approach of just ad-hoc creating a
slot in those routines?

It seems fairly insane how many places have this approximate code,
btw. In 11 we have a copy of nearly the same ~40 lines each in at least
ExecPartitionCheckEmitError(), ExecConstraints (twice) and
ExecWitchCheckOption().

Greetings,

Andres Freund