On 9/11/26 11:33, Andrey Borodin wrote:
Hi Maksim,
On 11 Sep 2026, Maksim Melnikov wrote:
I am not sure but for example
it can be LookupGXact.
Yes, your suspicion about LookupGXact() is correct. With your patch
applied, I could still trigger its Assert(gxact->prepare_start_lsn) by
pausing in the WAL branch and completing a checkpoint. Both holders
use LW_SHARED, so the checkpoint can proceed. This was a direct call
to the helper via a test wrapper, not an end-to-end logical replication
test.
I also reproduced the original race for both COMMIT PREPARED and
ROLLBACK PREPARED. Your fix blocks that interleaving in both cases.
Would it be better to take LW_EXCLUSIVE in CheckPointTwoPhase() and
protect the state reads in both FinishPreparedTransaction() and
LookupGXact() with LW_SHARED? That would address both readers without
serializing their I/O under a global exclusive lock. The reader lock
would still need to cover the WAL read, not just copying the LSN,
because the checkpoint can otherwise proceed to recycling that WAL.
Thank you!
Best regards, Andrey Borodin.
Hi Andrey,
Yes, I agree that we should acquire LW_EXCLUSIVE in CheckPointTwoPhase,
also I agree that better to wrap I/O on FinishPreparedTransaction with
just LW_SHARED, but I have some doubts about LookupGXact bacause as I
see here we change gxact state and it seems better do it with EXCLUSIVE
lock. Maybe we should acquire LW_EXCLUSIVE in LookupGXact and release it
and then acquire LW_SHARED for I/O. What do you think? For you
convenience I've attached draft patch.
Thanks you.
Best regards
Maksim Melnikov
From 2485aa28e16d9bcd7f96c2b6acd41c2f3edf86b3 Mon Sep 17 00:00:00 2001
From: Maksim Melnikov <[email protected]>
Date: Thu, 10 Sep 2026 19:37:16 +0300
Subject: [PATCH v2] Fixing race between prepared transaction commit and
checkpointer.
---
src/backend/access/transam/twophase.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 48e478a4ecb..9435204313c 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1531,6 +1531,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
fxid = gxact->fxid;
xid = XidFromFullTransactionId(fxid);
+ LWLockAcquire(TwoPhaseStateLock, LW_SHARED);
/*
* Read and validate 2PC state data. State data will typically be stored
* in WAL files if the LSN is after the last checkpoint record, or moved
@@ -1541,6 +1542,7 @@ FinishPreparedTransaction(const char *gid, bool isCommit)
else
XlogReadTwoPhaseData(gxact->prepare_start_lsn, &buf, NULL);
+ LWLockRelease(TwoPhaseStateLock);
/*
* Disassemble the header area
@@ -1853,7 +1855,7 @@ CheckPointTwoPhase(XLogRecPtr redo_horizon)
* prepare_end_lsn set prior to the last checkpoint yet is marked invalid,
* because of the efforts with delayChkptFlags.
*/
- LWLockAcquire(TwoPhaseStateLock, LW_SHARED);
+ LWLockAcquire(TwoPhaseStateLock, LW_EXCLUSIVE);
for (i = 0; i < TwoPhaseState->numPrepXacts; i++)
{
/*
--
2.43.0
