While hacking on the CSN patch, I spotted a race condition between
GetOldestActiveTransactionId() and GetNewTransactionId().
GetOldestActiveTransactionId() calculates the oldest XID that's still
running, by doing:
1. Read nextXid, without a lock. This is the upper bound, if there are
no active XIDs in the proc array.
2. Loop through the proc array, making note of the oldest XID.
While GetNewTransactionId() does:
1. Read and Increment nextXid
2. Set MyPgXact->xid.
It seems possible that if you call GetNewTransactionId() concurrently
with GetOldestActiveTransactionId(), GetOldestActiveTransactionId() sees
the new nextXid value that the concurrent GetNewTransactionId() set, but
doesn't see the old XID in the proc array. It will return a value that
doesn't cover the old XID, i.e. it won't consider the just-assigned XID
as in-progress.
Am I missing something? Commit c6d76d7c added a comment to
GetOldestActiveTransactionId() explaining why it's not necessary to
acquire XidGenLock there, but I think it missed the above race condition.
GetOldestActiveTransactionId() is not performance-critical, it's only
called when performing a checkpoint, so I think we should just bite the
bullet and grab the lock. Per attached patch.
- Heikki
diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c
index e5d487d..de45c16 100644
--- a/src/backend/storage/ipc/procarray.c
+++ b/src/backend/storage/ipc/procarray.c
@@ -2097,13 +2097,13 @@ GetOldestActiveTransactionId(void)
LWLockAcquire(ProcArrayLock, LW_SHARED);
/*
- * It's okay to read nextXid without acquiring XidGenLock because (1) we
- * assume TransactionIds can be read atomically and (2) we don't care if
- * we get a slightly stale value. It can't be very stale anyway, because
- * the LWLockAcquire above will have done any necessary memory
- * interlocking.
+ * Acquire XidGenLock while we read nextXid, to make sure that all
+ * XIDs < nextXid are already present in the proc array (or have
+ * already completed), when we spin over it.
*/
+ LWLockAcquire(XidGenLock, LW_SHARED);
oldestRunningXid = ShmemVariableCache->nextXid;
+ LWLockRelease(XidGenLock);
/*
* Spin over procArray collecting all xids and subxids.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
