Hi, Based on the discussion, I tried an alternative approach for master that avoids repeatedly rescanning the procarray.
Recovery currently collects a fixed list of VXIDs whose xmins conflict with a cleanup WAL record. After ProcArrayLock is released, another backend can import a listed backend's snapshot and advertise the same xmin. The importer is not in recovery's wait list, so recovery can finish waiting and replay the cleanup while the imported snapshot still needs the removed data. The attached v3 uses a separate atomic recoveryConflictTracked field in PGPROC. Startup marks each conflicting snapshot source while collecting the wait list under shared ProcArrayLock. ProcArrayInstallImportedXmin() holds ProcArrayLock exclusively and rejects imports from marked sources. This gives the following ordering: - An import completed before the scan is visible to the scan and included in the wait list. - An import attempted after the scan observes the source marker and fails. ResolveRecoveryConflictWithVirtualXIDs() clears each marker immediately after the corresponding VXID finishes. The marker is used only for RECOVERY_CONFLICT_SNAPSHOT. It is separate from pendingRecoveryConflicts because the cancellation bits have a different lifetime and are consumed by backend interrupt processing. Compared with v2, this prevents the chain of conflicting importers from growing instead of rescanning until no conflicts remain. It also retains the boolean return value of ProcArrayInstallImportedXmin(), and the new field does not need explicit initialization in ProcGlobalShmemInit, just as pendingRecoveryConflicts does not. The attached series is: - v3-0001 adds a deterministic TAP reproducer and its injection points. - v3-0002 implements the recoveryConflictTracked protocol. Patch 0001 is expected to fail without patch 0002 because the conflicting snapshot import succeeds. The series is based on master at 9f4bd91a196. I tested it with assertions, injection points, and TAP tests enabled. The build completed successfully, and recovery tests 056_standby_snapshot_export and 057_snapshot_import_conflict passed. The series also applies cleanly to that master commit. This approach conservatively rejects all snapshot imports from a tracked source until its tracked VXID finishes. Feedback on this tradeoff and the marker lifetime would be appreciated. Regards, Chee
From e5977336e4fd809960661fe62f461d515738c3b3 Mon Sep 17 00:00:00 2001 From: "chee.wooson" <[email protected]> Date: Fri, 11 Sep 2026 10:34:32 +0800 Subject: [PATCH v3 1/2] Add TAP test for recovery conflicts from imported snapshots Add injection points around the interval between collecting a snapshot recovery-conflict wait list and finishing the wait for a listed VXID. Use them in a TAP test that attempts to import a conflicting snapshot after the wait list has been collected. Without the accompanying fix, the import succeeds and the test fails because the importer is absent from recovery's fixed wait list. The second half also checks that imports from the source become possible again after its old VXID has been resolved. Reported-by: Scott Ray <[email protected]> Discussion: https://postgr.es/m/QpAansP4iVg_ttSs9x81PFAptL2sqR3AS06u8Jksm3_bHJvUwQjHOocRajbxBc3iiLdf9ZMC6gtXjZsxdxvOmqp98hLZcZuWyBDuQxS6uZc=@scottray.io --- src/backend/storage/ipc/standby.c | 6 + src/test/recovery/meson.build | 1 + .../t/057_snapshot_import_conflict.pl | 116 ++++++++++++++++++ 3 files changed, 123 insertions(+) create mode 100644 src/test/recovery/t/057_snapshot_import_conflict.pl diff --git a/src/backend/storage/ipc/standby.c b/src/backend/storage/ipc/standby.c index 7f011e04990..7065840fc26 100644 --- a/src/backend/storage/ipc/standby.c +++ b/src/backend/storage/ipc/standby.c @@ -439,6 +439,11 @@ ResolveRecoveryConflictWithVirtualXIDs(VirtualTransactionId *waitlist, } } + if (reason == RECOVERY_CONFLICT_SNAPSHOT) + { + INJECTION_POINT("recovery-conflict-snapshot-resolved", NULL); + } + /* The virtual transaction is gone now, wait for the next one */ waitlist++; } @@ -491,6 +496,7 @@ ResolveRecoveryConflictWithSnapshot(TransactionId snapshotConflictHorizon, Assert(TransactionIdIsNormal(snapshotConflictHorizon)); backends = GetConflictingVirtualXIDs(snapshotConflictHorizon, locator.dbOid); + INJECTION_POINT("recovery-conflict-snapshot-scan-complete", NULL); ResolveRecoveryConflictWithVirtualXIDs(backends, RECOVERY_CONFLICT_SNAPSHOT, WAIT_EVENT_RECOVERY_CONFLICT_SNAPSHOT, diff --git a/src/test/recovery/meson.build b/src/test/recovery/meson.build index 72113c5ac6e..4dca2b8acea 100644 --- a/src/test/recovery/meson.build +++ b/src/test/recovery/meson.build @@ -65,6 +65,7 @@ tests += { 't/054_unlogged_sequence_promotion.pl', 't/055_cascade_reconnect.pl', 't/056_standby_snapshot_export.pl', + 't/057_snapshot_import_conflict.pl', ], }, } diff --git a/src/test/recovery/t/057_snapshot_import_conflict.pl b/src/test/recovery/t/057_snapshot_import_conflict.pl new file mode 100644 index 00000000000..bb1ae44525c --- /dev/null +++ b/src/test/recovery/t/057_snapshot_import_conflict.pl @@ -0,0 +1,116 @@ +# Copyright (c) 2026, PostgreSQL Global Development Group +# +# Verify that standby recovery prevents snapshot imports from creating new +# conflicts after it has collected the VXIDs that conflict with a cleanup WAL +# record. + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Cluster; +use PostgreSQL::Test::Utils; +use Test::More; + +if ($ENV{enable_injection_points} ne 'yes') +{ + plan skip_all => 'Injection points not supported by this build'; +} + +my $primary = PostgreSQL::Test::Cluster->new('primary'); +$primary->init(allows_streaming => 1); +$primary->append_conf('postgresql.conf', 'autovacuum = off'); +$primary->start; + +if (!$primary->check_extension('injection_points')) +{ + plan skip_all => 'Extension injection_points not installed'; +} + +$primary->safe_psql( + 'postgres', q[ +CREATE EXTENSION injection_points; +CREATE TABLE t AS SELECT generate_series(1, 100) AS id; +]); + +$primary->backup('backup'); +my $standby = PostgreSQL::Test::Cluster->new('standby'); +$standby->init_from_backup($primary, 'backup', has_streaming => 1); +$standby->append_conf('postgresql.conf', + 'max_standby_streaming_delay = -1'); +$standby->start; + +# Register an old xmin and export its snapshot on the standby. +my $exporter = + $standby->background_psql('postgres', on_error_stop => 0); +$exporter->query_safe('BEGIN ISOLATION LEVEL REPEATABLE READ'); +is($exporter->query_safe('SELECT count(*) FROM t'), 100, + 'exporter sees all rows'); +my $old_snapshot = $exporter->query_safe('SELECT pg_export_snapshot()'); + +$standby->safe_psql( + 'postgres', q[ +SELECT injection_points_attach('recovery-conflict-snapshot-scan-complete', 'wait'); +SELECT injection_points_attach('recovery-conflict-snapshot-resolved', 'wait'); +]); + +# Generate a cleanup record whose replay conflicts with the old snapshot. +$primary->safe_psql( + 'postgres', q[ +DELETE FROM t; +VACUUM t; +]); + +# Recovery has completed its conflict scan, but has not started waiting for +# the exporter yet. +$standby->wait_for_event('startup', + 'recovery-conflict-snapshot-scan-complete'); + +my ($stdout, $stderr); +my $result = $standby->psql( + 'postgres', + "BEGIN ISOLATION LEVEL REPEATABLE READ; " + . "SET TRANSACTION SNAPSHOT '$old_snapshot';", + stdout => \$stdout, + stderr => \$stderr); +isnt($result, 0, 'cannot import a snapshot from a tracked source'); + +# Let recovery start waiting, then end the conflicting VXID. Recovery pauses +# after resolving it, before it can replay the cleanup record. +$standby->safe_psql( + 'postgres', q[ +SELECT injection_points_detach('recovery-conflict-snapshot-scan-complete'); +SELECT injection_points_wakeup('recovery-conflict-snapshot-scan-complete'); +]); +$exporter->query_safe('COMMIT'); +$standby->wait_for_event('startup', + 'recovery-conflict-snapshot-resolved'); + +# Reuse the same backend for a new transaction. Snapshot import must be +# allowed again once startup has resolved the old VXID. +$exporter->query_safe('BEGIN ISOLATION LEVEL REPEATABLE READ'); +my $new_snapshot = $exporter->query_safe('SELECT pg_export_snapshot()'); +$result = $standby->psql( + 'postgres', + "BEGIN ISOLATION LEVEL REPEATABLE READ; " + . "SET TRANSACTION SNAPSHOT '$new_snapshot'; SELECT count(*) FROM t;", + stdout => \$stdout, + stderr => \$stderr); +is($result, 0, 'can import from the source after its tracked VXID ends'); +$stdout =~ s/^\s+|\s+$//g; +is($stdout, '0', 'new snapshot sees the replayed delete'); + +$standby->safe_psql( + 'postgres', q[ +SELECT injection_points_detach('recovery-conflict-snapshot-resolved'); +SELECT injection_points_wakeup('recovery-conflict-snapshot-resolved'); +]); +$exporter->query_safe('COMMIT'); +$exporter->quit; + +$primary->wait_for_replay_catchup($standby); +is($standby->safe_psql('postgres', 'SELECT count(*) FROM t'), 0, + 'standby replays the cleanup after the conflict ends'); + +$standby->stop; +$primary->stop; + +done_testing(); -- 2.43.0
From 8753d7556c8459a4fd4e2c3a55c92e116a76b115 Mon Sep 17 00:00:00 2001 From: "chee.wooson" <[email protected]> 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 <[email protected]> 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
