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
