Date: Sat, 14 Mar 2020 17:35:41 -0700
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Daniel WM <dwilches@gmail.com>, pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16293: postgres segfaults and returns SQLSTATE 08006
Message-ID: <20200315003541.ptc7742cbouixgcl@alap3.anarazel.de>
References: <16293-26f5777d10143a66@postgresql.org>
 <28203.1583878791@sss.pgh.pa.us>
 <CAEbtaO3wTbx-DOh+B9UfNpoS2N4nQm25Y1DLrNNN_U1-8qNnqg@mail.gmail.com>
 <CAEbtaO39qxx7GX=u8j02y5K2eyVGT8gyCOLHS2KNENQS=w_D7Q@mail.gmail.com>
 <21088.1584220864@sss.pgh.pa.us>
 <20200314212657.jnvxfocyd5redf3p@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200314212657.jnvxfocyd5redf3p@alap3.anarazel.de>
Precedence: bulk

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