From db18703b5a7970f96142ede2f166513274075f8f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Mon, 15 Sep 2003 23:33:43 +0000 Subject: [PATCH] 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?) --- src/backend/access/heap/heapam.c | 38 +++++++++++++++----- src/backend/commands/async.c | 59 +++++++++++++++++++++++++++++--- src/backend/executor/execMain.c | 8 +++-- src/include/access/heapam.h | 6 ++-- 4 files changed, 92 insertions(+), 19 deletions(-) diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 54cba25571..3bcd8da1da 100644 --- 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 8ceac36cf4..95c83c2372 100644 --- 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 a75edb370a..1f78b1265b 100644 --- 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 34f1241ed5..05801af9e1 100644 --- 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); -- GitLab