From 8753d7556c8459a4fd4e2c3a55c92e116a76b115 Mon Sep 17 00:00:00 2001 From: "chee.wooson" Date: Fri, 11 Sep 2026 10:34:45 +0800 Subject: [PATCH v3 2/2] Fix recovery conflict resolution to account for imported snapshots Hot standby recovery builds a fixed list of VXIDs whose advertised xmin conflicts with a cleanup WAL record. After that scan, another transaction can import a snapshot from a listed backend and advertise the same xmin. Recovery then misses the importer when the original backend ends. Add a startup-owned recoveryConflictTracked marker to PGPROC. Set it while collecting conflicting VXIDs under ProcArrayLock, reject snapshot imports from marked sources under the importer's exclusive ProcArrayLock, and clear each marker after its tracked VXID ends. Keep this state separate from pending recovery-conflict cancellation reasons. Reported-by: Scott Ray Discussion: https://postgr.es/m/QpAansP4iVg_ttSs9x81PFAptL2sqR3AS06u8Jksm3_bHJvUwQjHOocRajbxBc3iiLdf9ZMC6gtXjZsxdxvOmqp98hLZcZuWyBDuQxS6uZc=@scottray.io --- src/backend/storage/ipc/procarray.c | 41 ++++++++++++++++++++++++++++- src/backend/storage/ipc/standby.c | 15 ++++++++++- src/backend/storage/lmgr/proc.c | 2 ++ src/include/storage/proc.h | 12 +++++++++ src/include/storage/procarray.h | 4 ++- 5 files changed, 71 insertions(+), 3 deletions(-) diff --git a/src/backend/storage/ipc/procarray.c b/src/backend/storage/ipc/procarray.c index b7e03134ed8..4b2d0e1c20f 100644 --- a/src/backend/storage/ipc/procarray.c +++ b/src/backend/storage/ipc/procarray.c @@ -2473,7 +2473,8 @@ GetSnapshotData(Snapshot snapshot) * check that the source transaction is still running, and we'd better do * that atomically with installing the new xmin. * - * Returns true if successful, false if source xact is no longer running. + * Returns true if successful, false if source xact is no longer running or + * recovery is waiting for it. */ bool ProcArrayInstallImportedXmin(TransactionId xmin, @@ -2534,6 +2535,14 @@ ProcArrayInstallImportedXmin(TransactionId xmin, !TransactionIdPrecedesOrEquals(xid, xmin)) continue; + /* + * Recovery has included this transaction in a fixed wait list. Do + * not let an imported snapshot transfer its conflicting xmin to a + * transaction that is absent from that list. + */ + if (pg_atomic_read_u32(&proc->recoveryConflictTracked) != 0) + break; + /* * We're good. Install the new xmin. As in GetSnapshotData, set * TransactionXmin too. (Note that because snapmgr.c called @@ -2562,6 +2571,10 @@ ProcArrayInstallImportedXmin(TransactionId xmin, * order to avoid the case where MyProc's xmin needs to be skipped for * computing xid horizon. * + * Unlike independent SQL snapshot imports, this is used only by parallel + * workers. A parallel worker cannot outlive the leader VXID that recovery + * tracks, so it need not check recoveryConflictTracked. + * * Returns true if successful, false if source xact is no longer running. */ bool @@ -3374,6 +3387,10 @@ GetCurrentVirtualXIDs(TransactionId limitXmin, bool excludeXmin0, * * If dbOid is valid we skip backends attached to other databases. * + * When limitXmin is valid, mark every returned PGPROC so that it cannot be + * used as the source of an imported snapshot. The caller must clear each + * marker after the corresponding VXID has ended. + * * Be careful to *not* pfree the result from this function. We reuse * this array sufficiently often that we use malloc for the result. */ @@ -3432,7 +3449,11 @@ GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid) GET_VXID_FROM_PGPROC(vxid, *proc); if (VirtualTransactionIdIsValid(vxid)) + { + if (TransactionIdIsValid(limitXmin)) + pg_atomic_write_u32(&proc->recoveryConflictTracked, 1); vxids[count++] = vxid; + } } } } @@ -3446,6 +3467,24 @@ GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid) return vxids; } +/* + * Stop preventing snapshot imports from the PGPROC slot associated with vxid. + * + * The tracked VXID has already ended, so the slot might now belong to another + * process or transaction. Clearing the marker is nevertheless safe: the + * startup process is its only setter and handles one conflict wait list at a + * time. A replacement process also initializes the marker to zero before + * entering the procarray. + */ +void +ProcArrayClearRecoveryConflictTracked(VirtualTransactionId vxid) +{ + PGPROC *proc = ProcNumberGetProc(vxid.procNumber); + + if (proc != NULL) + pg_atomic_write_u32(&proc->recoveryConflictTracked, 0); +} + /* * SignalRecoveryConflict -- signal that a process is blocking recovery * diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index 7065840fc26..a78b3aae67b 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -439,12 +439,17 @@ ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist, } } + /* + * The virtual transaction is gone now. If this is a snapshot + * conflict, allow snapshot imports from its PGPROC slot again before + * waiting for the next transaction. + */ if (reason == RECOVERY_CONFLICT_SNAPSHOT) { + ProcArrayClearRecoveryConflictTracked(*waitlist); INJECTION_POINT("recovery-conflict-snapshot-resolved", NULL); } - /* The virtual transaction is gone now, wait for the next one */ waitlist++; } @@ -494,6 +499,14 @@ ResolveRecoveryConflictWithSnapshot(TransactionId snapshotConflictHorizon, return; Assert(TransactionIdIsNormal(snapshotConflictHorizon)); + + /* + * Track each conflicting VXID before releasing ProcArrayLock. Snapshot + * import takes ProcArrayLock exclusively, so an import either completes + * before this scan and is included in the wait list, or observes the + * source's marker after the scan and fails. Consequently, no new + * conflicting VXID can appear while the fixed list is being drained. + */ backends = GetConflictingVirtualXIDs(snapshotConflictHorizon, locator.dbOid); INJECTION_POINT("recovery-conflict-snapshot-scan-complete", NULL); diff --git a/src/backend/storage/lmgr/proc.c b/src/backend/storage/lmgr/proc.c index ab65a6dbcc9..63954d04769 100644 --- a/src/backend/storage/lmgr/proc.c +++ b/src/backend/storage/lmgr/proc.c @@ -505,6 +505,7 @@ InitProcess(void) } #endif pg_atomic_write_u32(&MyProc->pendingRecoveryConflicts, 0); + pg_atomic_write_u32(&MyProc->recoveryConflictTracked, 0); /* Initialize fields for sync rep */ MyProc->waitLSN = InvalidXLogRecPtr; @@ -705,6 +706,7 @@ InitAuxiliaryProcess(void) } #endif pg_atomic_write_u32(&MyProc->pendingRecoveryConflicts, 0); + pg_atomic_write_u32(&MyProc->recoveryConflictTracked, 0); /* * Acquire ownership of the PGPROC's latch, so that we can use WaitLatch diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h index 4c3f431b4eb..2d2e2a98ebd 100644 --- a/src/include/storage/proc.h +++ b/src/include/storage/proc.h @@ -276,6 +276,18 @@ typedef struct PGPROC */ pg_atomic_uint32 pendingRecoveryConflicts; + /* + * Set by the startup process while it waits for this process's VXID to + * resolve a snapshot conflict. A set value prevents other backends from + * importing this process's snapshot and thereby creating a new conflict + * that is absent from the startup process's wait list. + * + * The startup process is the only process that sets this field. It + * clears the field after the tracked VXID has ended. Atomic access + * permits that cleanup even if the PGPROC slot has since been recycled. + */ + pg_atomic_uint32 recoveryConflictTracked; + /************************************************************************ * LWLock waiting ************************************************************************/ diff --git a/src/include/storage/procarray.h b/src/include/storage/procarray.h index d718a5b542f..6c226e1a26b 100644 --- a/src/include/storage/procarray.h +++ b/src/include/storage/procarray.h @@ -73,7 +73,9 @@ extern bool IsBackendPid(int pid); extern VirtualTransactionId *GetCurrentVirtualXIDs(TransactionId limitXmin, bool excludeXmin0, bool allDbs, int excludeVacuum, int *nvxids); -extern VirtualTransactionId *GetConflictingVirtualXIDs(TransactionId limitXmin, Oid dbOid); +extern VirtualTransactionId *GetConflictingVirtualXIDs(TransactionId limitXmin, + Oid dbOid); +extern void ProcArrayClearRecoveryConflictTracked(VirtualTransactionId vxid); extern bool SignalRecoveryConflict(PGPROC *proc, pid_t pid, RecoveryConflictReason reason); extern bool SignalRecoveryConflictWithVirtualXID(VirtualTransactionId vxid, RecoveryConflictReason reason); -- 2.43.0