pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feed[BUG]: segfault during update
6+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2020-11-08 8b39345a9d 94ec005f33 7aeb6404f0 In INSERT/UPDATE, use the table's real tuple descriptor as target.
* [BUG]: segfault during update
@ 2020-11-08 16:30 Drouvot, Bertrand <bdrouvot@amazon.com>
0 siblings, 1 reply; 6+ messages in thread
From: Drouvot, Bertrand @ 2020-11-08 16:30 UTC (permalink / raw)
To: pgsql-hackers
Hi hackers,
Here is a scenario that produces segfault during update (on version 12
and 13):
create table bdttrig (created_at timestamp without time zone not null
default now(),col1 bool not null default false, col2 text not null
default 'def', col3 char(6) not null default 'bdtbdt');
CREATE TABLE
CREATE or replace FUNCTION trigger_function()
RETURNS TRIGGER
LANGUAGE PLPGSQL
AS $$
BEGIN
IF (NEW != OLD) THEN
NEW.created_at = OLD.created_at;
RETURN NEW;
END IF;
RETURN OLD;
END;
$$
;
CREATE FUNCTION
create trigger bdt_trigger before update on bdttrig for each row EXECUTE
function trigger_function();
CREATE TRIGGER
insert into bdttrig(col1) values (true);
INSERT 0 1
alter table bdttrig add column col4 text not null default 'default';
ALTER TABLE
alter table bdttrig drop column col2;
ALTER TABLE
update bdttrig set col1 = true;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
I did some digging and found out and tested that the issue is fixed by
this commit:
https://github.com/postgres/postgres/commit/20d3fe9009ddbbbb3da3a2da298f922054b43f8c
So, we would need to back port this commit on 12 and 13.
Thanks
Bertrand
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [BUG]: segfault during update
@ 2020-11-08 17:18 Tom Lane <tgl@sss.pgh.pa.us>
parent: Drouvot, Bertrand <bdrouvot@amazon.com>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2020-11-08 17:18 UTC (permalink / raw)
To: Drouvot, Bertrand <bdrouvot@amazon.com>; +Cc: pgsql-hackers
"Drouvot, Bertrand" <bdrouvot@amazon.com> writes:
> Here is a scenario that produces segfault during update (on version 12
> and 13):
Hm. So the point about failing to reproduce dropped columns is more
critical than I thought. I wonder how come we get away with that before
v12?
> So, we would need to back port this commit on 12 and 13.
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.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [BUG]: segfault during update
@ 2020-11-08 17:46 Tom Lane <tgl@sss.pgh.pa.us>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 2 replies; 6+ messages in thread
From: Tom Lane @ 2020-11-08 17:46 UTC (permalink / raw)
To: Drouvot, Bertrand <bdrouvot@amazon.com>; +Cc: pgsql-hackers; Andres Freund <andres@anarazel.de>
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.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [BUG]: segfault during update
@ 2020-11-09 05:41 Drouvot, Bertrand <bdrouvot@amazon.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 0 replies; 6+ messages in thread
From: Drouvot, Bertrand @ 2020-11-09 05:41 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [BUG]: segfault during update
@ 2020-11-10 05:10 Andres Freund <andres@anarazel.de>
parent: Tom Lane <tgl@sss.pgh.pa.us>
1 sibling, 1 reply; 6+ messages in thread
From: Andres Freund @ 2020-11-10 05:10 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Drouvot, Bertrand <bdrouvot@amazon.com>; pgsql-hackers
Hi,
On 2020-11-08 12:46:44 -0500, Tom Lane wrote:
> This logic is entirely gone in v12, which confirms my instinct that
> there was something about Andres' slot-manipulation changes that
> broke this scenario.
Entirely possible :(. In my defense, it wasn't exactly obvious or
documented that the fast default code relied on this
ExecSetSlotDescriptor()...
> 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.
I really wonder if we ought to redesign the column default logic to
really be a property of the Attr, instead of the constraint stuff it is
now.
> So I'll go see about back-patching 20d3fe900.
Thanks.
Is it worth adding Bertrand's testcase to the regression suite in some
form?
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [BUG]: segfault during update
@ 2020-11-10 05:24 Tom Lane <tgl@sss.pgh.pa.us>
parent: Andres Freund <andres@anarazel.de>
0 siblings, 0 replies; 6+ messages in thread
From: Tom Lane @ 2020-11-10 05:24 UTC (permalink / raw)
To: Andres Freund <andres@anarazel.de>; +Cc: Drouvot, Bertrand <bdrouvot@amazon.com>; pgsql-hackers
Andres Freund <andres@anarazel.de> writes:
> On 2020-11-08 12:46:44 -0500, Tom Lane wrote:
>> 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.
> I really wonder if we ought to redesign the column default logic to
> really be a property of the Attr, instead of the constraint stuff it is
> now.
Yeah, I don't much like treating it as a constraint either. Not quite
sure that it's worth the work to move it somewhere else, though.
> Is it worth adding Bertrand's testcase to the regression suite in some
> form?
I did add an equivalent test case.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2020-11-10 05:24 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-08 16:30 [BUG]: segfault during update Drouvot, Bertrand <bdrouvot@amazon.com>
2020-11-08 17:18 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-11-08 17:46 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-11-09 05:41 ` Drouvot, Bertrand <bdrouvot@amazon.com>
2020-11-10 05:10 ` Andres Freund <andres@anarazel.de>
2020-11-10 05:24 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox