Subject: Re: [BUG]: segfault during update
To: <pgsql-hackers@lists.postgresql.org>
References: <db5d97c8-f48a-51e2-7b08-b73d5434d425@amazon.com>
 <1831655.1604855899@sss.pgh.pa.us> <1832746.1604857604@sss.pgh.pa.us>
From: "Drouvot, Bertrand" <bdrouvot@amazon.com>
Message-ID: <3f67f014-743d-583b-8516-6286ec17ad05@amazon.com>
Date: Mon, 9 Nov 2020 06:41:20 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:78.0)
 Gecko/20100101 Thunderbird/78.4.1
MIME-Version: 1.0
In-Reply-To: <1832746.1604857604@sss.pgh.pa.us>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

Hi,

On 11/8/20 6:46 PM, Tom Lane wrote:
> I wrote:
>> Yeah, this is sufficient reason why we must use the more invasive
>> patch on those branches.  What I'm wondering now is if there's a
>> way to break even-older branches based on failure to handle dropped
>> columns here.
> After tracing through the example in v11, I see why those branches
> are not broken: when ExecBRUpdateTriggers decides to return the
> trigger-returned tuple, it sticks it into a target slot like this:
>
>          /*
>           * Return the modified tuple using the es_trig_tuple_slot.  We assume
>           * the tuple was allocated in per-tuple memory context, and therefore
>           * will go away by itself. The tuple table slot should not try to
>           * clear it.
>           */
>          TupleTableSlot *newslot = estate->es_trig_tuple_slot;
>          TupleDesc    tupdesc = RelationGetDescr(relinfo->ri_RelationDesc);
>
>          if (newslot->tts_tupleDescriptor != tupdesc)
>              ExecSetSlotDescriptor(newslot, tupdesc);
>          ExecStoreTuple(newtuple, newslot, InvalidBuffer, false);
>
> So the slot that ExecConstraints et al will be working with contains
> the relation's actual tuple descriptor, not the approximate descr
> obtained by looking at the plan tlist.
>
> This logic is entirely gone in v12, which confirms my instinct that
> there was something about Andres' slot-manipulation changes that
> broke this scenario.  In v12 we end up using the junkfilter's output
> slot, which does not have a sufficiently accurate tupdesc to deal with
> an on-disk tuple rather than one constructed by the executor.
>
> So I'll go see about back-patching 20d3fe900.

Thanks for the back-patching!

Bertrand