Fix corner case for a BEFORE ROW UPDATE trigger returning OLD.

If the old row has any "missing" attributes that are supposed to
be retrieved from an associated tuple descriptor, the wrong things
happened because the trigger result is shoved directly into an
executor slot that lacks the missing-attribute data.  Notably,
CHECK-constraint verification would incorrectly see those columns
as NULL, and so would RETURNING-list evaluation.

Band-aid around this by forcibly expanding the tuple before passing
it to the trigger function.  (IMO it was a fundamental misdesign to
put the missing-attribute data into tuple constraints, which so
much of the system considers to be optional.  But we're probably
stuck with that now, and will have to continue to apply band-aids
as we find other places with similar issues.)

Back-patch to v12.  v11 would also have the issue, except that
commit 920311ab1 already applied a similar band-aid.  That forced
expansion in more cases than seem really necessary, though, so
this isn't a directly equivalent fix.

Amit Langote, with some cosmetic changes by me

Discussion: https://postgr.es/m/16644-5da7ef98a7ac4545@postgresql.org

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 5d8eb29f24796e334c3b1ff89c5056fcf4b70944..390933b036654e577f9598dcc2cb7272a444278d 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -89,6 +89,8 @@ 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,
@@ -3034,7 +3036,7 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
                ExecCopySlot(newslot, epqslot_clean);
        }
 
-       trigtuple = ExecFetchSlotHeapTuple(oldslot, true, &should_free_trig);
+       trigtuple = MaterializeTupleForTrigger(oldslot, &should_free_trig);
    }
    else
    {
@@ -3286,6 +3288,9 @@ ExecASTruncateTriggers(EState *estate, ResultRelInfo *relinfo)
 }
 
 
+/*
+ * Fetch tuple into "oldslot", dealing with locking and EPQ if necessary
+ */
 static bool
 GetTupleForTrigger(EState *estate,
                   EPQState *epqstate,
@@ -3399,6 +3404,40 @@ 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/test/regress/expected/triggers.out b/src/test/regress/expected/triggers.out
index 440e8e10784cb40fd36f480e7d7db174c3f44f78..b2f6d9e27e6c34722909d4fec519e40fef4c416c 100644 (file)
--- a/src/test/regress/expected/triggers.out
+++ b/src/test/regress/expected/triggers.out
@@ -214,6 +214,38 @@ select * from trigtest;
 ----+----
 (0 rows)
 
+drop table trigtest;
+-- Check behavior with an implicit column default, too (bug #16644)
+create table trigtest (a integer);
+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
+(1 row)
+
+alter table trigtest add column b integer default 42 not null;
+select * from trigtest;
+ a | b  
+---+----
+ 1 | 42
+(1 row)
+
+update trigtest set a = 2 where a = 1 returning *;
+ a | b  
+---+----
+ 1 | 42
+(1 row)
+
+select * from trigtest;
+ a | b  
+---+----
+ 1 | 42
+(1 row)
+
 drop table trigtest;
 create sequence ttdummy_seq increment 10 start 0 minvalue 0;
 create table tttest (

diff --git a/src/test/regress/sql/triggers.sql b/src/test/regress/sql/triggers.sql
index 67eb8ad0416ecd2e554b9984eff9df9c98788e9a..abf91f23f5e3a238c5afd763a84c68a4630f37ba 100644 (file)
--- a/src/test/regress/sql/triggers.sql
+++ b/src/test/regress/sql/triggers.sql
@@ -154,6 +154,24 @@ select * from trigtest;
 
 drop table trigtest;
 
+-- Check behavior with an implicit column default, too (bug #16644)
+create table trigtest (a integer);
+
+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;
+
+alter table trigtest add column b integer default 42 not null;
+
+select * from trigtest;
+update trigtest set a = 2 where a = 1 returning *;
+select * from trigtest;
+
+drop table trigtest;
+
 create sequence ttdummy_seq increment 10 start 0 minvalue 0;
 
 create table tttest (