Fix dangling-pointer problem in before-row update trigger processing.

ExecUpdate checked for whether ExecBRUpdateTriggers had returned a new
tuple value by seeing if the returned tuple was pointer-equal to the old
one.  But the "old one" was in estate->es_junkFilter's result slot, which
would be scribbled on if we had done an EvalPlanQual update in response to
a concurrent update of the target tuple; therefore we were comparing a
dangling pointer to a live one.  Given the right set of circumstances we
could get a false match, resulting in not forcing the tuple to be stored in
the slot we thought it was stored in.  In the case reported by Maxim Boguk
in bug #5798, this led to "cannot extract system attribute from virtual
tuple" failures when trying to do "RETURNING ctid".  I believe there is a
very-low-probability chance of more serious errors, such as generating
incorrect index entries based on the original rather than the
trigger-modified version of the row.

In HEAD, change all of ExecBRInsertTriggers, ExecIRInsertTriggers,
ExecBRUpdateTriggers, and ExecIRUpdateTriggers so that they continue to
have similar APIs.  In the back branches I just changed
ExecBRUpdateTriggers, since there is no bug in the ExecBRInsertTriggers
case.

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 251c3e81a2f41cb3e3b3b94da8c7f6aa36302cdd..d67611b2ce3d26226f8ffff52995d00752a94026 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -2172,18 +2172,19 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
                                                          GetModifiedColumns(relinfo, estate));
 }
 
-HeapTuple
+TupleTableSlot *
 ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
                                         ResultRelInfo *relinfo,
-                                        ItemPointer tupleid, HeapTuple newtuple)
+                                        ItemPointer tupleid, TupleTableSlot *slot)
 {
        TriggerDesc *trigdesc = relinfo->ri_TrigDesc;
        int                     ntrigs = trigdesc->n_before_row[TRIGGER_EVENT_UPDATE];
        int                *tgindx = trigdesc->tg_before_row[TRIGGER_EVENT_UPDATE];
+       HeapTuple       slottuple = ExecMaterializeSlot(slot);
+       HeapTuple       newtuple = slottuple;
        TriggerData LocTriggerData;
        HeapTuple       trigtuple;
        HeapTuple       oldtuple;
-       HeapTuple       intuple = newtuple;
        TupleTableSlot *newSlot;
        int                     i;
        Bitmapset  *modifiedCols;
@@ -2194,12 +2195,22 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
                return NULL;
 
        /*
-        * In READ COMMITTED isolation level it's possible that newtuple was
+        * In READ COMMITTED isolation level it's possible that target tuple was
         * changed due to concurrent update.  In that case we have a raw subplan
-        * output tuple and need to run it through the junk filter.
+        * output tuple in newSlot, and need to run it through the junk filter to
+        * produce an insertable tuple.
+        *
+        * Caution: more than likely, the passed-in slot is the same as the
+        * junkfilter's output slot, so we are clobbering the original value of
+        * slottuple by doing the filtering.  This is OK since neither we nor our
+        * caller have any more interest in the prior contents of that slot.
         */
        if (newSlot != NULL)
-               intuple = newtuple = ExecRemoveJunk(relinfo->ri_junkFilter, newSlot);
+       {
+               slot = ExecFilterJunk(relinfo->ri_junkFilter, newSlot);
+               slottuple = ExecMaterializeSlot(slot);
+               newtuple = slottuple;
+       }
 
        modifiedCols = GetModifiedColumns(relinfo, estate);
 
@@ -2226,13 +2237,33 @@ ExecBRUpdateTriggers(EState *estate, EPQState *epqstate,
                                                                           relinfo->ri_TrigFunctions,
                                                                           relinfo->ri_TrigInstrument,
                                                                           GetPerTupleMemoryContext(estate));
-               if (oldtuple != newtuple && oldtuple != intuple)
+               if (oldtuple != newtuple && oldtuple != slottuple)
                        heap_freetuple(oldtuple);
                if (newtuple == NULL)
-                       break;
+               {
+                       heap_freetuple(trigtuple);
+                       return NULL;            /* "do nothing" */
+               }
        }
        heap_freetuple(trigtuple);
-       return newtuple;
+
+       if (newtuple != slottuple)
+       {
+               /*
+                * 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);
+               slot = newslot;
+       }
+       return slot;
 }
 
 void

diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c
index f4b2b16b69e6e257f85934375a6924b5f20db0bf..ee6eef74f7a49fc1cc511bdeb02b4d1d78c9a1a4 100644 (file)
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -453,31 +453,14 @@ ExecUpdate(ItemPointer tupleid,
        if (resultRelInfo->ri_TrigDesc &&
                resultRelInfo->ri_TrigDesc->n_before_row[TRIGGER_EVENT_UPDATE] > 0)
        {
-               HeapTuple       newtuple;
-
-               newtuple = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
-                                                                               tupleid, tuple);
+               slot = ExecBRUpdateTriggers(estate, epqstate, resultRelInfo,
+                                                                       tupleid, slot);
 
-               if (newtuple == NULL)   /* "do nothing" */
+               if (slot == NULL)               /* "do nothing" */
                        return NULL;
 
-               if (newtuple != tuple)  /* modified by Trigger(s) */
-               {
-                       /*
-                        * Put the modified tuple into a slot for convenience of routines
-                        * below.  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(resultRelationDesc);
-
-                       if (newslot->tts_tupleDescriptor != tupdesc)
-                               ExecSetSlotDescriptor(newslot, tupdesc);
-                       ExecStoreTuple(newtuple, newslot, InvalidBuffer, false);
-                       slot = newslot;
-                       tuple = newtuple;
-               }
+               /* trigger might have changed tuple */
+               tuple = ExecMaterializeSlot(slot);
        }
 
        /*

diff --git a/src/include/commands/trigger.h b/src/include/commands/trigger.h
index db79eb4be732e188ea6cafced0aafd6f7a7a2cc1..07ad698e7ff1031eb0864b72305c0853b7c1027b 100644 (file)
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -149,11 +149,11 @@ extern void ExecBSUpdateTriggers(EState *estate,
                                         ResultRelInfo *relinfo);
 extern void ExecASUpdateTriggers(EState *estate,
                                         ResultRelInfo *relinfo);
-extern HeapTuple ExecBRUpdateTriggers(EState *estate,
+extern TupleTableSlot *ExecBRUpdateTriggers(EState *estate,
                                         EPQState *epqstate,
                                         ResultRelInfo *relinfo,
                                         ItemPointer tupleid,
-                                        HeapTuple newtuple);
+                                        TupleTableSlot *slot);
 extern void ExecARUpdateTriggers(EState *estate,
                                         ResultRelInfo *relinfo,
                                         ItemPointer tupleid,