Fix LISTEN/NOTIFY race condition reported by Gavin Sherry. While a

really general fix might be difficult, I believe the only case where
AtCommit_Notify could see an uncommitted tuple is where the other guy
has just unlistened and not yet committed.  The best solution seems to
be to just skip updating that tuple, on the assumption that the other
guy does not want to hear about the notification anyway.  This is not
perfect --- if the other guy rolls back his unlisten instead of committing,
then he really should have gotten this notify.  But to do that, we'd have
to wait to see if he commits or not, or make UNLISTEN hold exclusive lock
on pg_listener until commit.  Either of these answers is deadlock-prone,
not to mention horrible for interactive performance.  Do it this way
for now.  (What happened to that project to do LISTEN/NOTIFY in memory
with no table, anyway?)

diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 54cba25..3bcd8da 100644 (file)
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -8,7 +8,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/access/heap/heapam.c,v 1.154 2003/08/04 02:39:57 momjian Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/access/heap/heapam.c,v 1.155 2003/09/15 23:33:38 tgl Exp $
  *
  *
  * INTERFACE ROUTINES
@@ -1206,10 +1206,15 @@ simple_heap_insert(Relation relation, HeapTuple tup)
  *
  * NB: do not call this directly unless you are prepared to deal with
  * concurrent-update conditions.  Use simple_heap_delete instead.
+ *
+ * Normal, successful return value is HeapTupleMayBeUpdated, which
+ * actually means we did delete it.  Failure return codes are
+ * HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated
+ * (the last only possible if wait == false).
  */
 int
 heap_delete(Relation relation, ItemPointer tid,
-                       ItemPointer ctid, CommandId cid)
+                       ItemPointer ctid, CommandId cid, bool wait)
 {
        ItemId          lp;
        HeapTupleData tp;
@@ -1243,7 +1248,7 @@ l1:
                ReleaseBuffer(buffer);
                elog(ERROR, "attempted to delete invisible tuple");
        }
-       else if (result == HeapTupleBeingUpdated)
+       else if (result == HeapTupleBeingUpdated && wait)
        {
                TransactionId xwait = HeapTupleHeaderGetXmax(tp.t_data);
 
@@ -1275,7 +1280,9 @@ l1:
        }
        if (result != HeapTupleMayBeUpdated)
        {
-               Assert(result == HeapTupleSelfUpdated || result == HeapTupleUpdated);
+               Assert(result == HeapTupleSelfUpdated ||
+                          result == HeapTupleUpdated ||
+                          result == HeapTupleBeingUpdated);
                *ctid = tp.t_data->t_ctid;
                LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
                ReleaseBuffer(buffer);
@@ -1369,7 +1376,10 @@ simple_heap_delete(Relation relation, ItemPointer tid)
        ItemPointerData ctid;
        int                     result;
 
-       result = heap_delete(relation, tid, &ctid, GetCurrentCommandId());
+       result = heap_delete(relation, tid,
+                                                &ctid,
+                                                GetCurrentCommandId(),
+                                                true /* wait for commit */);
        switch (result)
        {
                case HeapTupleSelfUpdated:
@@ -1396,10 +1406,15 @@ simple_heap_delete(Relation relation, ItemPointer tid)
  *
  * NB: do not call this directly unless you are prepared to deal with
  * concurrent-update conditions.  Use simple_heap_update instead.
+ *
+ * Normal, successful return value is HeapTupleMayBeUpdated, which
+ * actually means we *did* update it.  Failure return codes are
+ * HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated
+ * (the last only possible if wait == false).
  */
 int
 heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
-                       ItemPointer ctid, CommandId cid)
+                       ItemPointer ctid, CommandId cid, bool wait)
 {
        ItemId          lp;
        HeapTupleData oldtup;
@@ -1443,7 +1458,7 @@ l2:
                ReleaseBuffer(buffer);
                elog(ERROR, "attempted to update invisible tuple");
        }
-       else if (result == HeapTupleBeingUpdated)
+       else if (result == HeapTupleBeingUpdated && wait)
        {
                TransactionId xwait = HeapTupleHeaderGetXmax(oldtup.t_data);
 
@@ -1475,7 +1490,9 @@ l2:
        }
        if (result != HeapTupleMayBeUpdated)
        {
-               Assert(result == HeapTupleSelfUpdated || result == HeapTupleUpdated);
+               Assert(result == HeapTupleSelfUpdated ||
+                          result == HeapTupleUpdated ||
+                          result == HeapTupleBeingUpdated);
                *ctid = oldtup.t_data->t_ctid;
                LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
                ReleaseBuffer(buffer);
@@ -1699,7 +1716,10 @@ simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup)
        ItemPointerData ctid;
        int                     result;
 
-       result = heap_update(relation, otid, tup, &ctid, GetCurrentCommandId());
+       result = heap_update(relation, otid, tup,
+                                                &ctid,
+                                                GetCurrentCommandId(),
+                                                true /* wait for commit */);
        switch (result)
        {
                case HeapTupleSelfUpdated:

diff --git a/src/backend/commands/async.c b/src/backend/commands/async.c
index 8ceac36..95c83c2 100644 (file)
--- a/src/backend/commands/async.c
+++ b/src/backend/commands/async.c
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1994, Regents of the University of California
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/commands/async.c,v 1.99 2003/09/15 00:26:31 petere Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/commands/async.c,v 1.100 2003/09/15 23:33:39 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -515,13 +515,58 @@ AtCommit_Notify(void)
                        }
                        else if (listener->notification == 0)
                        {
+                               ItemPointerData ctid;
+                               int                     result;
+
                                rTuple = heap_modifytuple(lTuple, lRel,
                                                                                  value, nulls, repl);
-                               simple_heap_update(lRel, &lTuple->t_self, rTuple);
+                               /*
+                                * We cannot use simple_heap_update here because the tuple
+                                * could have been modified by an uncommitted transaction;
+                                * specifically, since UNLISTEN releases exclusive lock on
+                                * the table before commit, the other guy could already have
+                                * tried to unlisten.  There are no other cases where we
+                                * should be able to see an uncommitted update or delete.
+                                * Therefore, our response to a HeapTupleBeingUpdated result
+                                * is just to ignore it.  We do *not* wait for the other
+                                * guy to commit --- that would risk deadlock, and we don't
+                                * want to block while holding the table lock anyway for
+                                * performance reasons.  We also ignore HeapTupleUpdated,
+                                * which could occur if the other guy commits between our
+                                * heap_getnext and heap_update calls.
+                                */
+                               result = heap_update(lRel, &lTuple->t_self, rTuple,
+                                                                        &ctid,
+                                                                        GetCurrentCommandId(),
+                                                                        false /* no wait for commit */);
+                               switch (result)
+                               {
+                                       case HeapTupleSelfUpdated:
+                                               /* Tuple was already updated in current command? */
+                                               elog(ERROR, "tuple already updated by self");
+                                               break;
+
+                                       case HeapTupleMayBeUpdated:
+                                               /* done successfully */
 
 #ifdef NOT_USED                                        /* currently there are no indexes */
-                               CatalogUpdateIndexes(lRel, rTuple);
+                                               CatalogUpdateIndexes(lRel, rTuple);
 #endif
+                                               break;
+
+                                       case HeapTupleBeingUpdated:
+                                               /* ignore uncommitted tuples */
+                                               break;
+
+                                       case HeapTupleUpdated:
+                                               /* ignore just-committed tuples */
+                                               break;
+
+                                       default:
+                                               elog(ERROR, "unrecognized heap_update status: %u",
+                                                        result);
+                                               break;
+                               }
                        }
                }
        }
@@ -803,7 +848,13 @@ ProcessIncomingNotify(void)
                                         relname, (int) sourcePID);
 
                        NotifyMyFrontEnd(relname, sourcePID);
-                       /* Rewrite the tuple with 0 in notification column */
+                       /*
+                        * Rewrite the tuple with 0 in notification column.
+                        *
+                        * simple_heap_update is safe here because no one else would
+                        * have tried to UNLISTEN us, so there can be no uncommitted
+                        * changes.
+                        */
                        rTuple = heap_modifytuple(lTuple, lRel, value, nulls, repl);
                        simple_heap_update(lRel, &lTuple->t_self, rTuple);
 

diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index a75edb3..1f78b12 100644 (file)
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -26,7 +26,7 @@
  *
  *
  * IDENTIFICATION
- *       $Header: /cvsroot/pgsql/src/backend/executor/execMain.c,v 1.216 2003/08/08 21:41:34 momjian Exp $
+ *       $Header: /cvsroot/pgsql/src/backend/executor/execMain.c,v 1.217 2003/09/15 23:33:43 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -1406,7 +1406,8 @@ ExecDelete(TupleTableSlot *slot,
 ldelete:;
        result = heap_delete(resultRelationDesc, tupleid,
                                                 &ctid,
-                                                estate->es_snapshot->curcid);
+                                                estate->es_snapshot->curcid,
+                                                true /* wait for commit */);
        switch (result)
        {
                case HeapTupleSelfUpdated:
@@ -1540,7 +1541,8 @@ lreplace:;
         */
        result = heap_update(resultRelationDesc, tupleid, tuple,
                                                 &ctid,
-                                                estate->es_snapshot->curcid);
+                                                estate->es_snapshot->curcid,
+                                                true /* wait for commit */);
        switch (result)
        {
                case HeapTupleSelfUpdated:

diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 34f1241..05801af 100644 (file)
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -7,7 +7,7 @@
  * Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group
  * Portions Copyright (c) 1994, Regents of the University of California
  *
- * $Id: heapam.h,v 1.83 2003/08/04 02:40:10 momjian Exp $
+ * $Id: heapam.h,v 1.84 2003/09/15 23:33:43 tgl Exp $
  *
  *-------------------------------------------------------------------------
  */
@@ -155,9 +155,9 @@ extern void setLastTid(const ItemPointer tid);
 
 extern Oid     heap_insert(Relation relation, HeapTuple tup, CommandId cid);
 extern int heap_delete(Relation relation, ItemPointer tid, ItemPointer ctid,
-                       CommandId cid);
+                       CommandId cid, bool wait);
 extern int heap_update(Relation relation, ItemPointer otid, HeapTuple tup,
-                       ItemPointer ctid, CommandId cid);
+                       ItemPointer ctid, CommandId cid, bool wait);
 extern int heap_mark4update(Relation relation, HeapTuple tup,
                                 Buffer *userbuf, CommandId cid);