On Mon, Feb 18, 2019 at 05:05:13PM +0100, Oleksii Kliukin wrote: > That looks like a race condition to me. What happens is that another > transaction with the name identical to the running one can start and proceed > to the prepare phase while the original one commits, failing at last instead > of waiting for the original one to finish.
It took me 50 clients and a bit more than 20 seconds, but I have been able to reproduce the problem with one error. Thanks for the reproducer. This is indeed a race condition with 2PC. > By looking at the source code of FinishPreparedTransaction() I can see the > RemoveGXact() call, which removes the prepared transaction from > TwoPhaseState->prepXacts. It is placed at the very end of the function, > after the post-commit callbacks that clear out the locks held by the > transaction. Those callbacks are not guarded by the TwoPhaseStateLock, > resulting in a possibility for a concurrent session to proceed will > MarkAsPreparing after acquiring the locks released by them. Hm, I see. Taking a breakpoint just after ProcessRecords() or putting a sleep there makes the issue plain. The same issue can happen with predicate locks. > I couldn’t find any documentation on the expected outcome in cases like > this, so I assume it might not be a bug, but an undocumented behavior. If you run two transactions in parallel using your script, the second transaction would wait at LOCK time until the first transaction releases its locks with the COMMIT PREPARED. > Should I go about and produce a patch to put a note in the description of > commit prepared, or is there any interest in changing the behavior to avoid > such conflicts altogether (perhaps by holding the lock throughout the > cleanup phase)? That's a bug, let's fix it. I agree with your suggestion that we had better not release the 2PC lock using the callbacks for COMMIT PREPARED or ROLLBACK PREPARED until the shared memory state is cleared. At the same time, I think that we should be smarter in the ordering of the actions: we care about predicate locks here, but not about the on-disk file removal and the stat counters. One trick is that the post-commit callback calls TwoPhaseGetDummyProc() which would try to take TwoPhaseStateLock which needs to be applied so we need some extra logic to not take a lock in this case. From what I can see this is older than 9.4 as the removal of the GXACT entry in shared memory and the post-commit hooks are out of sync for a long time :( Attached is a patch that fixes the problem for me. Please note the safety net in TwoPhaseGetGXact(). -- Michael
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 9a8a6bb119..5916697214 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -803,7 +803,7 @@ pg_prepared_xact(PG_FUNCTION_ARGS)
* specified by XID
*/
static GlobalTransaction
-TwoPhaseGetGXact(TransactionId xid)
+TwoPhaseGetGXact(TransactionId xid, bool lock_held)
{
GlobalTransaction result = NULL;
int i;
@@ -811,6 +811,9 @@ TwoPhaseGetGXact(TransactionId xid)
static TransactionId cached_xid = InvalidTransactionId;
static GlobalTransaction cached_gxact = NULL;
+ Assert(!lock_held ||
+ LWLockHeldByMeInMode(TwoPhaseStateLock, LW_EXCLUSIVE));
+
/*
* During a recovery, COMMIT PREPARED, or ABORT PREPARED, we'll be called
* repeatedly for the same XID. We can save work with a simple cache.
@@ -818,7 +821,8 @@ TwoPhaseGetGXact(TransactionId xid)
if (xid == cached_xid)
return cached_gxact;
- LWLockAcquire(TwoPhaseStateLock, LW_SHARED);
+ if (!lock_held)
+ LWLockAcquire(TwoPhaseStateLock, LW_SHARED);
for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
{
@@ -832,7 +836,8 @@ TwoPhaseGetGXact(TransactionId xid)
}
}
- LWLockRelease(TwoPhaseStateLock);
+ if (!lock_held)
+ LWLockRelease(TwoPhaseStateLock);
if (result == NULL) /* should not happen */
elog(ERROR, "failed to find GlobalTransaction for xid %u", xid);
@@ -854,7 +859,7 @@ TwoPhaseGetGXact(TransactionId xid)
BackendId
TwoPhaseGetDummyBackendId(TransactionId xid)
{
- GlobalTransaction gxact = TwoPhaseGetGXact(xid);
+ GlobalTransaction gxact = TwoPhaseGetGXact(xid, false);
return gxact->dummyBackendId;
}
@@ -864,9 +869,9 @@ TwoPhaseGetDummyBackendId(TransactionId xid)
* Get the PGPROC that represents a prepared transaction specified by XID
*/
PGPROC *
-TwoPhaseGetDummyProc(TransactionId xid)
+TwoPhaseGetDummyProc(TransactionId xid, bool lock_held)
{
- GlobalTransaction gxact = TwoPhaseGetGXact(xid);
+ GlobalTransaction gxact = TwoPhaseGetGXact(xid, lock_held);
return &ProcGlobal->allProcs[gxact->pgprocno];
}
@@ -1560,6 +1565,16 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
if (hdr->initfileinval)
RelationCacheInitFilePostInvalidate();
+ /* Count the prepared xact as committed or aborted */
+ AtEOXact_PgStat(isCommit);
+
+ /*
+ * Acquire the two-phase lock, we want to work on the two-phase
+ * callbacks while holding it to avoid potential conflicts with
+ * other transactions, until the shared memory state is cleared.
+ */
+ LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
+
/* And now do the callbacks */
if (isCommit)
ProcessRecords(bufptr, xid, twophase_postcommit_callbacks);
@@ -1568,8 +1583,14 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
PredicateLockTwoPhaseFinish(xid, isCommit);
- /* Count the prepared xact as committed or aborted */
- AtEOXact_PgStat(isCommit);
+ /* Clear shared memory state */
+ RemoveGXact(gxact);
+
+ /*
+ * Release the lock as all callbacks are called and shared memory
+ * cleanup is done.
+ */
+ LWLockRelease(TwoPhaseStateLock);
/*
* And now we can clean up any files we may have left.
@@ -1577,9 +1598,6 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
if (gxact->ondisk)
RemoveTwoPhaseFile(xid, true);
- LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
- RemoveGXact(gxact);
- LWLockRelease(TwoPhaseStateLock);
MyLockedGxact = NULL;
RESUME_INTERRUPTS();
diff --git a/src/backend/storage/lmgr/lock.c b/src/backend/storage/lmgr/lock.c
index 3bb5ce350a..78fdbd6ff8 100644
--- a/src/backend/storage/lmgr/lock.c
+++ b/src/backend/storage/lmgr/lock.c
@@ -3243,7 +3243,7 @@ AtPrepare_Locks(void)
void
PostPrepare_Locks(TransactionId xid)
{
- PGPROC *newproc = TwoPhaseGetDummyProc(xid);
+ PGPROC *newproc = TwoPhaseGetDummyProc(xid, false);
HASH_SEQ_STATUS status;
LOCALLOCK *locallock;
LOCK *lock;
@@ -4034,7 +4034,7 @@ lock_twophase_recover(TransactionId xid, uint16 info,
void *recdata, uint32 len)
{
TwoPhaseLockRecord *rec = (TwoPhaseLockRecord *) recdata;
- PGPROC *proc = TwoPhaseGetDummyProc(xid);
+ PGPROC *proc = TwoPhaseGetDummyProc(xid, false);
LOCKTAG *locktag;
LOCKMODE lockmode;
LOCKMETHODID lockmethodid;
@@ -4247,7 +4247,7 @@ lock_twophase_postcommit(TransactionId xid, uint16 info,
void *recdata, uint32 len)
{
TwoPhaseLockRecord *rec = (TwoPhaseLockRecord *) recdata;
- PGPROC *proc = TwoPhaseGetDummyProc(xid);
+ PGPROC *proc = TwoPhaseGetDummyProc(xid, true);
LOCKTAG *locktag;
LOCKMETHODID lockmethodid;
LockMethod lockMethodTable;
diff --git a/src/include/access/twophase.h b/src/include/access/twophase.h
index 6228b091d8..2dcd08e9fa 100644
--- a/src/include/access/twophase.h
+++ b/src/include/access/twophase.h
@@ -34,7 +34,7 @@ extern void TwoPhaseShmemInit(void);
extern void AtAbort_Twophase(void);
extern void PostPrepare_Twophase(void);
-extern PGPROC *TwoPhaseGetDummyProc(TransactionId xid);
+extern PGPROC *TwoPhaseGetDummyProc(TransactionId xid, bool lock_held);
extern BackendId TwoPhaseGetDummyBackendId(TransactionId xid);
extern GlobalTransaction MarkAsPreparing(TransactionId xid, const char *gid,
signature.asc
Description: PGP signature
