In INSERT/UPDATE, use the table's real tuple descriptor as target.

This back-patches commit 20d3fe9 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

Author: tglsfdc

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?
  */

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;

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)
 				{

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);

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 d integer default 42 not null;
+select * from trigtest;
+ a | b |   c   | d  
+---+---+-------+----
+ 1 | t | xyzzy | 42
+(1 row)
+
+update trigtest set a = 2 where a = 1 returning *;
+ a | b |   c   | d  
+---+---+-------+----
+ 1 | t | xyzzy | 42
+(1 row)
+
+select * from trigtest;
+ a | b |   c   | d  
+---+---+-------+----
+ 1 | t | xyzzy | 42
 (1 row)
 
-alter table trigtest add column b integer default 42 not null;
+alter table trigtest drop column b;
 select * from trigtest;
- a | b  
----+----
- 1 | 42
+ a |   c   | d  
+---+-------+----
+ 1 | xyzzy | 42
 (1 row)
 
 update trigtest set a = 2 where a = 1 returning *;
- a | b  
----+----
- 1 | 42
+ a |   c   | d  
+---+-------+----
+ 1 | xyzzy | 42
 (1 row)
 
 select * from trigtest;
- a | b  
----+----
- 1 | 42
+ a |   c   | d  
+---+-------+----
+ 1 | xyzzy | 42
 (1 row)
 
 drop table trigtest;

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 *;