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

Hi,

On Sun, Mar 15, 2020 at 9:36 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> 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?

Looking into it, that partitioning code in ExecConstraint() would not
"normally" overwrite a slot that is not managed by partitioning.
Normally, there would be a dedicated "partition" slot that would be
used, but only if tuple conversion from root parent to leaf partition
is necessary before routing the tuple.

What is not "normal" in this case is that tuple conversion is deemed
necessary when converting from leaf partition to root parent whereas
not in the other direction. It's because one of the partition's
attributes has atthasmissing set to true, which triggers this code in
convert_tuples_by_name():

/*
* If the input column has a missing attribute, we need a
* conversion.
*/
if (inatt->atthasmissing)
{
same = false;
break;
}

This asymmetry causes the code in ExecConstraints() to try to convert
the tuple whereas no conversion would have been performed before,
meaning to dedicated partition slot would have been passed to
ExecConstraints().

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

Right.

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

Agreed.

> 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().

That is certainly bad. I think we should get that code in one place
and inside ExecBuildSlotValueDescription() seems like a good place,
because that's what needs a converted tuple to build a string from it.
I have tried that in the attached -- need different patches for
different branches as there have been a lot of small changes in this
code over releases.

--
Thank you,
Amit