projects / postgresql.git / commitdiff
summary | shortlog | log | commit | commitdiff | tree
raw | patch | inline | side by side (parent: 69bef7f)
Fix dangling-pointer problem in before-row update trigger processing.
authorTom Lane <tgl@sss.pgh.pa.us>
Tue, 22 Feb 2011 02:18:30 +0000 (21:18 -0500)
committerTom Lane <tgl@sss.pgh.pa.us>
Tue, 22 Feb 2011 02:18:30 +0000 (21:18 -0500)
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.

src/backend/commands/trigger.c patch | blob | blame | history
src/backend/executor/execMain.c patch | blob | blame | history
src/include/commands/trigger.h patch | blob | blame | history

diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c
index 6b360afd19e08e56f7769dd60751670b8879123f..e05917e278ce3097e140023cecde3dae96d7df3b 100644 (file)
--- a/src/backend/commands/trigger.c
+++ b/src/backend/commands/trigger.c
@@ -1620,18 +1620,19 @@ ExecASUpdateTriggers(EState *estate, ResultRelInfo *relinfo)
                              false, NULL, NULL);
 }
 
-HeapTuple
+TupleTableSlot *
 ExecBRUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
-                    ItemPointer tupleid, HeapTuple newtuple,
+                    ItemPointer tupleid, TupleTableSlot *slot,
                     CommandId cid)
 {
    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;
 
@@ -1640,11 +1641,22 @@ ExecBRUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
        return NULL;
 
    /*
-    * In READ COMMITTED isolation level it's possible that newtuple was
-    * changed due to concurrent update.
+    * 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 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(estate->es_junkFilter, newSlot);
+   {
+       slot = ExecFilterJunk(estate->es_junkFilter, newSlot);
+       slottuple = ExecMaterializeSlot(slot);
+       newtuple = slottuple;
+   }
 
    LocTriggerData.type = T_TriggerData;
    LocTriggerData.tg_event = TRIGGER_EVENT_UPDATE |
@@ -1667,13 +1679,33 @@ ExecBRUpdateTriggers(EState *estate, ResultRelInfo *relinfo,
                                       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/execMain.c b/src/backend/executor/execMain.c
index 7908458c5e370f683e465b1c8cc74733a7898e49..859367703cb75b93f7c51ce5fdb1de08690af9bd 100644 (file)
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1678,31 +1678,15 @@ ExecUpdate(TupleTableSlot *slot,
    if (resultRelInfo->ri_TrigDesc &&
        resultRelInfo->ri_TrigDesc->n_before_row[TRIGGER_EVENT_UPDATE] > 0)
    {
-       HeapTuple   newtuple;
-
-       newtuple = ExecBRUpdateTriggers(estate, resultRelInfo,
-                                       tupleid, tuple,
-                                       estate->es_snapshot->curcid);
+       slot = ExecBRUpdateTriggers(estate, resultRelInfo,
+                                   tupleid, slot,
+                                   estate->es_snapshot->curcid);
 
-       if (newtuple == NULL)   /* "do nothing" */
+       if (slot == NULL)       /* "do nothing" */
            return;
 
-       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;
-
-           if (newslot->tts_tupleDescriptor != slot->tts_tupleDescriptor)
-               ExecSetSlotDescriptor(newslot, slot->tts_tupleDescriptor);
-           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 d88f584885410cd28c129d38ebf6ac1500118cae..12f494726f54a679706389ed896f60a77c7b49f3 100644 (file)
--- a/src/include/commands/trigger.h
+++ b/src/include/commands/trigger.h
@@ -147,10 +147,10 @@ extern void ExecBSUpdateTriggers(EState *estate,
                     ResultRelInfo *relinfo);
 extern void ExecASUpdateTriggers(EState *estate,
                     ResultRelInfo *relinfo);
-extern HeapTuple ExecBRUpdateTriggers(EState *estate,
+extern TupleTableSlot *ExecBRUpdateTriggers(EState *estate,
                     ResultRelInfo *relinfo,
                     ItemPointer tupleid,
-                    HeapTuple newtuple,
+                    TupleTableSlot *slot,
                     CommandId cid);
 extern void ExecARUpdateTriggers(EState *estate,
                     ResultRelInfo *relinfo,
This is the main PostgreSQL git repository.