Skip to content

Commit db18703

Browse files
tglsfdctglsfdc
committed
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?)
1 parent d73e1b3 commit db18703

File tree

4 files changed

+92
-19
lines changed

4 files changed

+92
-19
lines changed

‎src/backend/access/heap/heapam.c

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
*
99
*
1010
* IDENTIFICATION
11-
* $Header: /cvsroot/pgsql/src/backend/access/heap/heapam.c,v 1.154 2003/08/04 02:39:57 momjian Exp $
11+
* $Header: /cvsroot/pgsql/src/backend/access/heap/heapam.c,v 1.155 2003/09/15 23:33:38 tgl Exp $
1212
*
1313
*
1414
* INTERFACE ROUTINES
@@ -1206,10 +1206,15 @@ simple_heap_insert(Relation relation, HeapTuple tup)
12061206
*
12071207
* NB: do not call this directly unless you are prepared to deal with
12081208
* concurrent-update conditions. Use simple_heap_delete instead.
1209+
*
1210+
* Normal, successful return value is HeapTupleMayBeUpdated, which
1211+
* actually means we did delete it. Failure return codes are
1212+
* HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated
1213+
* (the last only possible if wait == false).
12091214
*/
12101215
int
12111216
heap_delete(Relation relation, ItemPointer tid,
1212-
ItemPointer ctid, CommandId cid)
1217+
ItemPointer ctid, CommandId cid, bool wait)
12131218
{
12141219
ItemId lp;
12151220
HeapTupleData tp;
@@ -1243,7 +1248,7 @@ heap_delete(Relation relation, ItemPointer tid,
12431248
ReleaseBuffer(buffer);
12441249
elog(ERROR, "attempted to delete invisible tuple");
12451250
}
1246-
else if (result == HeapTupleBeingUpdated)
1251+
else if (result == HeapTupleBeingUpdated && wait)
12471252
{
12481253
TransactionId xwait = HeapTupleHeaderGetXmax(tp.t_data);
12491254

@@ -1275,7 +1280,9 @@ heap_delete(Relation relation, ItemPointer tid,
12751280
}
12761281
if (result != HeapTupleMayBeUpdated)
12771282
{
1278-
Assert(result == HeapTupleSelfUpdated || result == HeapTupleUpdated);
1283+
Assert(result == HeapTupleSelfUpdated ||
1284+
result == HeapTupleUpdated ||
1285+
result == HeapTupleBeingUpdated);
12791286
*ctid = tp.t_data->t_ctid;
12801287
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
12811288
ReleaseBuffer(buffer);
@@ -1369,7 +1376,10 @@ simple_heap_delete(Relation relation, ItemPointer tid)
13691376
ItemPointerData ctid;
13701377
int result;
13711378

1372-
result = heap_delete(relation, tid, &ctid, GetCurrentCommandId());
1379+
result = heap_delete(relation, tid,
1380+
&ctid,
1381+
GetCurrentCommandId(),
1382+
true /* wait for commit */);
13731383
switch (result)
13741384
{
13751385
case HeapTupleSelfUpdated:
@@ -1396,10 +1406,15 @@ simple_heap_delete(Relation relation, ItemPointer tid)
13961406
*
13971407
* NB: do not call this directly unless you are prepared to deal with
13981408
* concurrent-update conditions. Use simple_heap_update instead.
1409+
*
1410+
* Normal, successful return value is HeapTupleMayBeUpdated, which
1411+
* actually means we *did* update it. Failure return codes are
1412+
* HeapTupleSelfUpdated, HeapTupleUpdated, or HeapTupleBeingUpdated
1413+
* (the last only possible if wait == false).
13991414
*/
14001415
int
14011416
heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
1402-
ItemPointer ctid, CommandId cid)
1417+
ItemPointer ctid, CommandId cid, bool wait)
14031418
{
14041419
ItemId lp;
14051420
HeapTupleData oldtup;
@@ -1443,7 +1458,7 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
14431458
ReleaseBuffer(buffer);
14441459
elog(ERROR, "attempted to update invisible tuple");
14451460
}
1446-
else if (result == HeapTupleBeingUpdated)
1461+
else if (result == HeapTupleBeingUpdated && wait)
14471462
{
14481463
TransactionId xwait = HeapTupleHeaderGetXmax(oldtup.t_data);
14491464

@@ -1475,7 +1490,9 @@ heap_update(Relation relation, ItemPointer otid, HeapTuple newtup,
14751490
}
14761491
if (result != HeapTupleMayBeUpdated)
14771492
{
1478-
Assert(result == HeapTupleSelfUpdated || result == HeapTupleUpdated);
1493+
Assert(result == HeapTupleSelfUpdated ||
1494+
result == HeapTupleUpdated ||
1495+
result == HeapTupleBeingUpdated);
14791496
*ctid = oldtup.t_data->t_ctid;
14801497
LockBuffer(buffer, BUFFER_LOCK_UNLOCK);
14811498
ReleaseBuffer(buffer);
@@ -1699,7 +1716,10 @@ simple_heap_update(Relation relation, ItemPointer otid, HeapTuple tup)
16991716
ItemPointerData ctid;
17001717
int result;
17011718

1702-
result = heap_update(relation, otid, tup, &ctid, GetCurrentCommandId());
1719+
result = heap_update(relation, otid, tup,
1720+
&ctid,
1721+
GetCurrentCommandId(),
1722+
true /* wait for commit */);
17031723
switch (result)
17041724
{
17051725
case HeapTupleSelfUpdated:

‎src/backend/commands/async.c

Lines changed: 55 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1994, Regents of the University of California
88
*
99
* IDENTIFICATION
10-
* $Header: /cvsroot/pgsql/src/backend/commands/async.c,v 1.99 2003/09/15 00:26:31 petere Exp $
10+
* $Header: /cvsroot/pgsql/src/backend/commands/async.c,v 1.100 2003/09/15 23:33:39 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -515,13 +515,58 @@ AtCommit_Notify(void)
515515
}
516516
else if (listener->notification == 0)
517517
{
518+
ItemPointerData ctid;
519+
int result;
520+
518521
rTuple = heap_modifytuple(lTuple, lRel,
519522
value, nulls, repl);
520-
simple_heap_update(lRel, &lTuple->t_self, rTuple);
523+
/*
524+
* We cannot use simple_heap_update here because the tuple
525+
* could have been modified by an uncommitted transaction;
526+
* specifically, since UNLISTEN releases exclusive lock on
527+
* the table before commit, the other guy could already have
528+
* tried to unlisten. There are no other cases where we
529+
* should be able to see an uncommitted update or delete.
530+
* Therefore, our response to a HeapTupleBeingUpdated result
531+
* is just to ignore it. We do *not* wait for the other
532+
* guy to commit --- that would risk deadlock, and we don't
533+
* want to block while holding the table lock anyway for
534+
* performance reasons. We also ignore HeapTupleUpdated,
535+
* which could occur if the other guy commits between our
536+
* heap_getnext and heap_update calls.
537+
*/
538+
result = heap_update(lRel, &lTuple->t_self, rTuple,
539+
&ctid,
540+
GetCurrentCommandId(),
541+
false /* no wait for commit */);
542+
switch (result)
543+
{
544+
case HeapTupleSelfUpdated:
545+
/* Tuple was already updated in current command? */
546+
elog(ERROR, "tuple already updated by self");
547+
break;
548+
549+
case HeapTupleMayBeUpdated:
550+
/* done successfully */
521551

522552
#ifdef NOT_USED /* currently there are no indexes */
523-
CatalogUpdateIndexes(lRel, rTuple);
553+
CatalogUpdateIndexes(lRel, rTuple);
524554
#endif
555+
break;
556+
557+
case HeapTupleBeingUpdated:
558+
/* ignore uncommitted tuples */
559+
break;
560+
561+
case HeapTupleUpdated:
562+
/* ignore just-committed tuples */
563+
break;
564+
565+
default:
566+
elog(ERROR, "unrecognized heap_update status: %u",
567+
result);
568+
break;
569+
}
525570
}
526571
}
527572
}
@@ -803,7 +848,13 @@ ProcessIncomingNotify(void)
803848
relname, (int) sourcePID);
804849

805850
NotifyMyFrontEnd(relname, sourcePID);
806-
/* Rewrite the tuple with 0 in notification column */
851+
/*
852+
* Rewrite the tuple with 0 in notification column.
853+
*
854+
* simple_heap_update is safe here because no one else would
855+
* have tried to UNLISTEN us, so there can be no uncommitted
856+
* changes.
857+
*/
807858
rTuple = heap_modifytuple(lTuple, lRel, value, nulls, repl);
808859
simple_heap_update(lRel, &lTuple->t_self, rTuple);
809860

‎src/backend/executor/execMain.c

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@
2626
*
2727
*
2828
* IDENTIFICATION
29-
* $Header: /cvsroot/pgsql/src/backend/executor/execMain.c,v 1.216 2003/08/08 21:41:34 momjian Exp $
29+
* $Header: /cvsroot/pgsql/src/backend/executor/execMain.c,v 1.217 2003/09/15 23:33:43 tgl Exp $
3030
*
3131
*-------------------------------------------------------------------------
3232
*/
@@ -1406,7 +1406,8 @@ ExecDelete(TupleTableSlot *slot,
14061406
ldelete:;
14071407
result = heap_delete(resultRelationDesc, tupleid,
14081408
&ctid,
1409-
estate->es_snapshot->curcid);
1409+
estate->es_snapshot->curcid,
1410+
true /* wait for commit */);
14101411
switch (result)
14111412
{
14121413
case HeapTupleSelfUpdated:
@@ -1540,7 +1541,8 @@ lreplace:;
15401541
*/
15411542
result = heap_update(resultRelationDesc, tupleid, tuple,
15421543
&ctid,
1543-
estate->es_snapshot->curcid);
1544+
estate->es_snapshot->curcid,
1545+
true /* wait for commit */);
15441546
switch (result)
15451547
{
15461548
case HeapTupleSelfUpdated:

‎src/include/access/heapam.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* Portions Copyright (c) 1996-2003, PostgreSQL Global Development Group
88
* Portions Copyright (c) 1994, Regents of the University of California
99
*
10-
* $Id: heapam.h,v 1.83 2003/08/04 02:40:10 momjian Exp $
10+
* $Id: heapam.h,v 1.84 2003/09/15 23:33:43 tgl Exp $
1111
*
1212
*-------------------------------------------------------------------------
1313
*/
@@ -155,9 +155,9 @@ extern void setLastTid(const ItemPointer tid);
155155

156156
extern Oid heap_insert(Relation relation, HeapTuple tup, CommandId cid);
157157
extern int heap_delete(Relation relation, ItemPointer tid, ItemPointer ctid,
158-
CommandId cid);
158+
CommandId cid, bool wait);
159159
extern int heap_update(Relation relation, ItemPointer otid, HeapTuple tup,
160-
ItemPointer ctid, CommandId cid);
160+
ItemPointer ctid, CommandId cid, bool wait);
161161
extern int heap_mark4update(Relation relation, HeapTuple tup,
162162
Buffer *userbuf, CommandId cid);
163163

0 commit comments

Comments
 (0)