postgrespro.git / summary / log / commit / refs
commit effe7d9552dd41fd6119404e39eecd2653cd966c
Author: Michael Paquier <michael@paquier.xyz>
Date: Mon Feb 25 05:19:34 2019 +0000
Make release of 2PC identifier and locks consistent in COMMIT PREPARED
When preparing a transaction in two-phase commit, a dummy PGPROC entry
holding the GID used for the transaction is registered, which gets
released once COMMIT PREPARED is run. Prior releasing its shared memory
state, all the locks taken in the prepared transaction are released
using a dedicated set of callbacks (pgstat and multixact having similar
callbacks), which may cause the locks to be released before the GID is
set free.
Hence, there is a small window where lock conflicts could happen, for
example:
- Transaction A releases its locks, still holding its GID in shared
memory.
- Transaction B held a lock which conflicted with locks of transaction
A.
- Transaction B continues its processing, reusing the same GID as
transaction A.
- Transaction B fails because of a conflicting GID, already in use by
transaction A.
This commit changes the shared memory state release so as post-commit
callbacks and predicate lock cleanup happen consistently with the shared
memory state cleanup for the dummy PGPROC entry. The race window is
small and 2PC had this issue from the start, so no backpatch is done.
On top if that fixes discussed involved ABI breakages, which are not
welcome in stable branches.
Reported-by: Oleksii Kliukin, Ildar Musin
Diagnosed-by: Oleksii Kliukin, Ildar Musin
Author: Michael Paquier
Reviewed-by: Masahiko Sawada, Oleksii Kliukin
Discussion: https://postgr.es/m/BF9B38A4-2BFF-46E8-BA87-A2D00A8047A6@hintbits.com
src/backend/access/transam/multixact.c | 6 ++---
src/backend/access/transam/twophase.c | 47 ++++++++++++++++++++++++++--------
src/backend/storage/lmgr/lock.c | 6 ++---
src/include/access/twophase.h | 4 +--
4 files changed, 44 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 07ae7a31db..c399871940 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, false);
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 add979bf0b..64679dd2de 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,14 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
if (hdr->initfileinval)
RelationCacheInitFilePostInvalidate();
+ /*
+ * Acquire the two-phase lock. We want to work on the two-phase callbacks
+ * while holding it to avoid potential conflicts with other transactions
+ * attempting to use the same GID, so the lock is released once the shared
+ * memory state is cleared.
+ */
+ LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
+
/* And now do the callbacks */
if (isCommit)
ProcessRecords(bufptr, xid, twophase_postcommit_callbacks);
@@ -1568,6 +1587,15 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
PredicateLockTwoPhaseFinish(xid, isCommit);
+ /* Clear shared memory state */
+ RemoveGXact(gxact);
+
+ /*
+ * Release the lock as all callbacks are called and shared memory cleanup
+ * is done.
+ */
+ LWLockRelease(TwoPhaseStateLock);
+
/* Count the prepared xact as committed or aborted */
AtEOXact_PgStat(isCommit);
@@ -1577,9 +1605,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,
[parent: 29ddb548f683]