From 94ec005f334e1ae740a04f4f8a14e3f7f934a346 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Sun, 8 Nov 2020 13:08:36 -0500 Subject: [PATCH] In INSERT/UPDATE, use the table's real tuple descriptor as target. This back-patches commit 20d3fe900 into the v12 and v13 branches. At the time I thought that commit was not fixing any observable bug, but Bertrand Drouvot showed otherwise: adding a dropped column to the previously-considered scenario crashes v12 and v13, unless the dropped column happens to be an integer. That is, of course, because the tupdesc we derive from the plan output tlist fails to describe the dropped column accurately, so that we'll do the wrong thing with a tuple in which that column isn't NULL. There is no bug in pre-v12 branches because they already did use the table's real tuple descriptor for any trigger-returned tuple. It seems that this set of bugs can be blamed on the changes that removed es_trig_tuple_slot, though I've not attempted to pin that down precisely. Although there's no code change needed in HEAD, update the test case to include a dropped column there too. Discussion: https://postgr.es/m/db5d97c8-f48a-51e2-7b08-b73d5434d425@amazon.com Discussion: https://postgr.es/m/16644-5da7ef98a7ac4545@postgresql.org --- src/backend/commands/trigger.c | 38 +------------------- src/backend/executor/execJunk.c | 42 +++++++++++++++++----- src/backend/executor/nodeModifyTable.c | 22 +++++++++--- src/include/executor/executor.h | 3 ++ src/test/regress/expected/triggers.out | 50 ++++++++++++++++++-------- src/test/regress/sql/triggers.sql | 13 +++++-- 6 files changed, 102 insertions(+), 66 deletions(-) diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 390933b036..d85c053a14 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -89,8 +89,6 @@ static bool GetTupleForTrigger(EState *estate, LockTupleMode lockmode, TupleTableSlot *oldslot, TupleTableSlot **newSlot); -static HeapTuple MaterializeTupleForTrigger(TupleTableSlot *slot, - bool *shouldFree); static bool TriggerEnabled(EState *estate, ResultRelInfo *relinfo, Trigger *trigger, TriggerEvent event, Bitmapset *modifiedCols, @@ -3036,7 +3034,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate, ExecCopySlot(newslot, epqslot_clean); } - trigtuple = MaterializeTupleForTrigger(oldslot, &should_free_trig); + trigtuple = ExecFetchSlotHeapTuple(oldslot, true, &should_free_trig); } else { @@ -3404,40 +3402,6 @@ GetTupleForTrigger(EState *estate, return true; } -/* - * Extract a HeapTuple that we can pass off to trigger functions. - * - * We must materialize the tuple and make sure it is not dependent on any - * attrmissing data. This is needed for the old row in BEFORE UPDATE - * triggers, since they can choose to pass back this exact tuple as the update - * result, causing the tuple to be inserted into an executor slot that lacks - * the attrmissing data. - * - * Currently we don't seem to need to remove the attrmissing dependency in any - * other cases, but keep this as a separate function to simplify fixing things - * if that changes. - */ -static HeapTuple -MaterializeTupleForTrigger(TupleTableSlot *slot, bool *shouldFree) -{ - HeapTuple tup; - TupleDesc tupdesc = slot->tts_tupleDescriptor; - - tup = ExecFetchSlotHeapTuple(slot, true, shouldFree); - if (HeapTupleHeaderGetNatts(tup->t_data) < tupdesc->natts && - tupdesc->constr && tupdesc->constr->missing) - { - HeapTuple newtup; - - newtup = heap_expand_tuple(tup, tupdesc); - if (*shouldFree) - heap_freetuple(tup); - *shouldFree = true; - tup = newtup; - } - return tup; -} - /* * Is trigger enabled to fire? */ diff --git a/src/backend/executor/execJunk.c b/src/backend/executor/execJunk.c index 79fcc8dee2..a1465b0bd2 100644 --- a/src/backend/executor/execJunk.c +++ b/src/backend/executor/execJunk.c @@ -54,23 +54,48 @@ * * The source targetlist is passed in. The output tuple descriptor is * built from the non-junk tlist entries. - * An optional resultSlot can be passed as well. + * An optional resultSlot can be passed as well; otherwise, we create one. */ JunkFilter * ExecInitJunkFilter(List *targetList, TupleTableSlot *slot) { - JunkFilter *junkfilter; TupleDesc cleanTupType; - int cleanLength; - AttrNumber *cleanMap; - ListCell *t; - AttrNumber cleanResno; /* * Compute the tuple descriptor for the cleaned tuple. */ cleanTupType = ExecCleanTypeFromTL(targetList); + /* + * The rest is the same as ExecInitJunkFilterInsertion, ie, we want to map + * every non-junk targetlist column into the output tuple. + */ + return ExecInitJunkFilterInsertion(targetList, cleanTupType, slot); +} + +/* + * ExecInitJunkFilterInsertion + * + * Initialize a JunkFilter for insertions into a table. + * + * Here, we are given the target "clean" tuple descriptor rather than + * inferring it from the targetlist. Although the target descriptor can + * contain deleted columns, that is not of concern here, since the targetlist + * should contain corresponding NULL constants (cf. ExecCheckPlanOutput). + * It is assumed that the caller has checked that the table's columns match up + * with the non-junk columns of the targetlist. + */ +JunkFilter * +ExecInitJunkFilterInsertion(List *targetList, + TupleDesc cleanTupType, + TupleTableSlot *slot) +{ + JunkFilter *junkfilter; + int cleanLength; + AttrNumber *cleanMap; + ListCell *t; + AttrNumber cleanResno; + /* * Use the given slot, or make a new slot if we weren't given one. */ @@ -93,17 +118,18 @@ ExecInitJunkFilter(List *targetList, TupleTableSlot *slot) if (cleanLength > 0) { cleanMap = (AttrNumber *) palloc(cleanLength * sizeof(AttrNumber)); - cleanResno = 1; + cleanResno = 0; foreach(t, targetList) { TargetEntry *tle = lfirst(t); if (!tle->resjunk) { - cleanMap[cleanResno - 1] = tle->resno; + cleanMap[cleanResno] = tle->resno; cleanResno++; } } + Assert(cleanResno == cleanLength); } else cleanMap = NULL; diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 08609f23dd..cbf7abafc5 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -2660,15 +2660,27 @@ ExecInitModifyTable(ModifyTable *node, EState *estate, int eflags) TupleTableSlot *junkresslot; subplan = mtstate->mt_plans[i]->plan; - if (operation == CMD_INSERT || operation == CMD_UPDATE) - ExecCheckPlanOutput(resultRelInfo->ri_RelationDesc, - subplan->targetlist); junkresslot = ExecInitExtraTupleSlot(estate, NULL, table_slot_callbacks(resultRelInfo->ri_RelationDesc)); - j = ExecInitJunkFilter(subplan->targetlist, - junkresslot); + + /* + * For an INSERT or UPDATE, the result tuple must always match + * the target table's descriptor. For a DELETE, it won't + * (indeed, there's probably no non-junk output columns). + */ + if (operation == CMD_INSERT || operation == CMD_UPDATE) + { + ExecCheckPlanOutput(resultRelInfo->ri_RelationDesc, + subplan->targetlist); + j = ExecInitJunkFilterInsertion(subplan->targetlist, + RelationGetDescr(resultRelInfo->ri_RelationDesc), + junkresslot); + } + else + j = ExecInitJunkFilter(subplan->targetlist, + junkresslot); if (operation == CMD_UPDATE || operation == CMD_DELETE) { diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h index 9be0b38830..bb9d9b2ca1 100644 --- a/src/include/executor/executor.h +++ b/src/include/executor/executor.h @@ -150,6 +150,9 @@ extern void ResetTupleHashTable(TupleHashTable hashtable); */ extern JunkFilter *ExecInitJunkFilter(List *targetList, TupleTableSlot *slot); +extern JunkFilter *ExecInitJunkFilterInsertion(List *targetList, + TupleDesc cleanTupType, + TupleTableSlot *slot); extern JunkFilter *ExecInitJunkFilterConversion(List *targetList, TupleDesc cleanTupType, TupleTableSlot *slot); diff --git a/src/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out index b2f6d9e27e..316eefdb6b 100644 --- a/src/test/regress/expected/triggers.out +++ b/src/test/regress/expected/triggers.out @@ -216,34 +216,56 @@ select * from trigtest; drop table trigtest; -- Check behavior with an implicit column default, too (bug #16644) -create table trigtest (a integer); +create table trigtest ( + a integer, + b bool default true not null, + c text default 'xyzzy' not null); create trigger trigger_return_old before insert or delete or update on trigtest for each row execute procedure trigger_return_old(); insert into trigtest values(1); select * from trigtest; - a ---- - 1 + a | b | c +---+---+------- + 1 | t | xyzzy (1 row) -alter table trigtest add column b integer default 42 not null; +alter table trigtest add column d integer default 42 not null; select * from trigtest; - a | b ----+---- - 1 | 42 + a | b | c | d +---+---+-------+---- + 1 | t | xyzzy | 42 (1 row) update trigtest set a = 2 where a = 1 returning *; - a | b ----+---- - 1 | 42 + a | b | c | d +---+---+-------+---- + 1 | t | xyzzy | 42 (1 row) select * from trigtest; - a | b ----+---- - 1 | 42 + a | b | c | d +---+---+-------+---- + 1 | t | xyzzy | 42 +(1 row) + +alter table trigtest drop column b; +select * from trigtest; + a | c | d +---+-------+---- + 1 | xyzzy | 42 +(1 row) + +update trigtest set a = 2 where a = 1 returning *; + a | c | d +---+-------+---- + 1 | xyzzy | 42 +(1 row) + +select * from trigtest; + a | c | d +---+-------+---- + 1 | xyzzy | 42 (1 row) drop table trigtest; diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql index abf91f23f5..4caa11d63b 100644 --- a/src/test/regress/sql/triggers.sql +++ b/src/test/regress/sql/triggers.sql @@ -155,7 +155,10 @@ select * from trigtest; drop table trigtest; -- Check behavior with an implicit column default, too (bug #16644) -create table trigtest (a integer); +create table trigtest ( + a integer, + b bool default true not null, + c text default 'xyzzy' not null); create trigger trigger_return_old before insert or delete or update on trigtest @@ -164,7 +167,13 @@ create trigger trigger_return_old insert into trigtest values(1); select * from trigtest; -alter table trigtest add column b integer default 42 not null; +alter table trigtest add column d integer default 42 not null; + +select * from trigtest; +update trigtest set a = 2 where a = 1 returning *; +select * from trigtest; + +alter table trigtest drop column b; select * from trigtest; update trigtest set a = 2 where a = 1 returning *;