On Wed, Feb 20, 2019 at 03:14:07PM +0900, Masahiko Sawada wrote: > @@ -811,6 +811,9 @@ TwoPhaseGetGXact(TransactionId xid) > static TransactionId cached_xid = InvalidTransactionId; > static GlobalTransaction cached_gxact = NULL; > > + Assert(!lock_held || > + LWLockHeldByMeInMode(TwoPhaseStateLock, LW_EXCLUSIVE)); > + > > It seems strange to me, why do we always require an exclusive lock > here in spite of acquiring a shared lock?
Thanks for looking at the patch. LWLockHeldByMe() is fine here. It seems that I had a brain fade. > TwoPhaseGetDummyBackendId() is called by > multixact_twophase_postcommit() which is called while holding > TwoPhaseStateLock in exclusive mode in FinishPreparedTransaction(). > Since we cache the global transaction when we call > lock_twophase_commit() I couldn't produce issue but it seems to have a > potential of locking problem. You are right: this could cause a deadlock problem, but it actually cannot be triggered now because the repetitive calls of TwoPhaseGetGXact() make the resulting XID to be cached, so the lock finishes by never be taken, but that could become broken in the future. From a consistency point of view, adding the same option to TwoPhaseGetGXact() and TwoPhaseGetGXact() to control the lock acquisition is much better as well. Please note that the recovery tests stress multixact post-commit callbacks already, so you can see the caching behavior with that one: BEGIN; CREATE TABLE t_009_tbl2 (id int, msg text); SAVEPOINT s1; INSERT INTO t_009_tbl2 VALUES (27, 'issued to stuff'); PREPARE TRANSACTION 'xact_009_13'; CHECKPOINT; COMMIT PREPARED 'xact_009_13'; The assertion really needs to be before the cached XID is checked though. Attached is an updated patch. Thanks for the feedback. -- Michael
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 07ae7a31db..d3e20fe6bc 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1713,7 +1713,7 @@ PostPrepare_MultiXact(TransactionId xid)
myOldestMember = OldestMemberMXactId[MyBackendId];
if (MultiXactIdIsValid(myOldestMember))
{
- BackendId dummyBackendId = TwoPhaseGetDummyBackendId(xid);
+ BackendId dummyBackendId = TwoPhaseGetDummyBackendId(xid, false);
/*
* Even though storing MultiXactId is atomic, acquire lock to make
@@ -1755,7 +1755,7 @@ void
multixact_twophase_recover(TransactionId xid, uint16 info,
void *recdata, uint32 len)
{
- BackendId dummyBackendId = TwoPhaseGetDummyBackendId(xid);
+ BackendId dummyBackendId = TwoPhaseGetDummyBackendId(xid, true);
MultiXactId oldestMember;
/*
@@ -1776,7 +1776,7 @@ void
multixact_twophase_postcommit(TransactionId xid, uint16 info,
void *recdata, uint32 len)
{
- BackendId dummyBackendId = TwoPhaseGetDummyBackendId(xid);
+ BackendId dummyBackendId = TwoPhaseGetDummyBackendId(xid, true);
Assert(len == sizeof(MultiXactId));
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 9a8a6bb119..a44db07676 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -801,9 +801,12 @@ pg_prepared_xact(PG_FUNCTION_ARGS)
* TwoPhaseGetGXact
* Get the GlobalTransaction struct for a prepared transaction
* specified by XID
+ *
+ * If lock_held is set to true, TwoPhaseStateLock will not be taken, so the
+ * caller had better hold it.
*/
static GlobalTransaction
-TwoPhaseGetGXact(TransactionId xid)
+TwoPhaseGetGXact(TransactionId xid, bool lock_held)
{
GlobalTransaction result = NULL;
int i;
@@ -811,6 +814,8 @@ TwoPhaseGetGXact(TransactionId xid)
static TransactionId cached_xid = InvalidTransactionId;
static GlobalTransaction cached_gxact = NULL;
+ Assert(!lock_held || LWLockHeldByMe(TwoPhaseStateLock));
+
/*
* 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 +823,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 +838,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);
@@ -849,12 +856,13 @@ TwoPhaseGetGXact(TransactionId xid)
*
* Dummy backend IDs are similar to real backend IDs of real backends.
* They start at MaxBackends + 1, and are unique across all currently active
- * real backends and prepared transactions.
+ * real backends and prepared transactions. If lock_held is set to true,
+ * TwoPhaseStateLock will not be taken, so the caller had better hold it.
*/
BackendId
-TwoPhaseGetDummyBackendId(TransactionId xid)
+TwoPhaseGetDummyBackendId(TransactionId xid, bool lock_held)
{
- GlobalTransaction gxact = TwoPhaseGetGXact(xid);
+ GlobalTransaction gxact = TwoPhaseGetGXact(xid, lock_held);
return gxact->dummyBackendId;
}
@@ -862,11 +870,14 @@ TwoPhaseGetDummyBackendId(TransactionId xid)
/*
* TwoPhaseGetDummyProc
* Get the PGPROC that represents a prepared transaction specified by XID
+ *
+ * If lock_held is set to true, TwoPhaseStateLock will not be taken, so the
+ * caller had better hold it.
*/
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 +1571,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 +1589,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 +1604,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..fcd1913c43 100644
--- a/src/include/access/twophase.h
+++ b/src/include/access/twophase.h
@@ -34,8 +34,8 @@ extern void TwoPhaseShmemInit(void);
extern void AtAbort_Twophase(void);
extern void PostPrepare_Twophase(void);
-extern PGPROC *TwoPhaseGetDummyProc(TransactionId xid);
-extern BackendId TwoPhaseGetDummyBackendId(TransactionId xid);
+extern PGPROC *TwoPhaseGetDummyProc(TransactionId xid, bool lock_held);
+extern BackendId TwoPhaseGetDummyBackendId(TransactionId xid, bool lock_held);
extern GlobalTransaction MarkAsPreparing(TransactionId xid, const char *gid,
TimestampTz prepared_at,
signature.asc
Description: PGP signature
