Recovery conflict resolution misses backends that import snapshots

On Sat, Jul 25, 2026 at 11:03:52PM +0000, Scott Ray wrote:

There is a silent bug in standby conflict resolution that can cause
incorrect results. In ResolveRecoveryConflictWithSnapshot, the standby
calls GetConflictingVirtualXIDs once, then waits for each returned VXID
to disappear. However, after GetConflictingVirtualXIDs releases the
ProcArrayLock, a backend may import a conflicting snapshot from one of
the backends GetConflictingVirtualXIDs identified as conflicting. The
resulting VXID conflicts with recovery, but
ResolveRecoveryConflictWithSnapshot doesn't know it has to wait for the
new VXID to disappear before proceeding, so the standby removes tuple
versions or index entries that the importing backend needs.

Hmm. It looks like you are right here. I am not the most fluent here
with this area of the code, so adding in CC a couple of folks who
could perhaps comment, being usually interested on these matters.

The first attached patch adds a TAP test that fails deterministically
on unpatched master. The second makes
ResolveRecoveryConflictWithSnapshot call GetConflictingVirtualXIDs
until the result is empty, at which point importing a conflicting
snapshot is impossible: acquiring a conflicting xmin requires a live,
conflicting VXID as a source.

+	/*
+	 * Scan until no conflicting VXID remains.
[...]
+	for (;;)
+	{

Adding a code pattern that could potentially cause this code path to
loop infinitely is not what I would call a principled approach, I
would call it a risky one.
--
Michael

On Mon, Jul 27, 2026 at 5:19 AM Michael Paquier <michael@paquier.xyz> wrote:

+       /*
+        * Scan until no conflicting VXID remains.
[...]
+       for (;;)
+       {

Adding a code pattern that could potentially cause this code path to
loop infinitely is not what I would call a principled approach, I
would call it a risky one.

I think we should at least have CFI in this loop so that it responds
to promotion, shutdown, etc. Currently, it only happens on the waiting
path: ResolveRecoveryConflictWithVirtualXIDs() → inner while
(!VirtualXactLock(*waitlist, false)) → WaitExceedsMaxStandbyDelay(),
which calls CHECK_FOR_INTERRUPTS(). But if a scan returns a non-empty
list and, by the time you call
ResolveRecoveryConflictWithVirtualXIDs(), all those VXIDs have already
ended, VirtualXactLock(..., false) returns true immediately for each
entry, the wait branch is never taken, and that whole call does zero
CFI.

For a finite value of max_standby_streaming_delay, the deadline is
anchored to WAL receipt and does not reset per iteration as mentioned
in the comment in the patch, so once it passes,
ResolveRecoveryConflictWithVirtualXIDs starts cancelling the
conflicting backends (SignalRecoveryConflictWithVirtualXID), not just
waiting. Recovery then makes forward progress by killing sources. A
new importer must find a source that's still alive in a window that
recovery is actively shrinking by terminating everything in it, and
each new importer is itself immediately cancellable on the next pass
(deadline already elapsed → cancel now). So it converges. For
max_standby_streaming_delay = -1, it anyway means "wait forever for
conflicts to clear", so the new behaviour should be acceptable in that
case.

The other way to avoid looping here is to stop the chain from being
extended: past the deadline, refuse new conflicting snapshot imports
on the standby (aka "publish the intended horizon and reject
conflicting imports"). I feel that is over engineering, instead, the
current proposed solution looks reasonable to me but if we still want
to avoid looping then probably we can attempt something like "reject
conflicting imports" in master.

--
With Regards,
Amit Kapila.

On Monday, July 27th, 2026 at 9:57 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:

I think we should at least have CFI in this loop so that it responds
to promotion, shutdown, etc.

If the goal is to make the standby responsive to shutdown, we should
add ProcessStartupProcInterrupts(), which is already called elsewhere
during redo: xlog_redo() -> CheckRequiredParameterValues() ->
RecoveryRequiresIntParameter() -> ProcessStartupProcInterrupts()
in a loop. CHECK_FOR_INTERRUPTS() doesn't handle shutdown.

If the target is promotion, then we could use CheckForStandbyTrigger().
Conflict resolution occurs while the standby is processing a record,
and from what I can tell, responding to promotion after beginning to
apply the record but before finishing is unprecedented and would force
the standby to decide what to do with the partially-applied record.

The attached v2 calls ProcessStartupProcInterrupts(), and I confirmed
that the standby shuts down promptly when signaled.

On Mon, Jul 27, 2026 at 5:19 AM Michael Paquier
<michael@paquier.xyz> wrote:

Adding a code pattern that could potentially cause this code path to
loop infinitely is not what I would call a principled approach, I
would call it a risky one.

I tried to cause an infinite loop using a standby with
max_standby_streaming_delay = 5s, max_connections = 400, and a pool of
clients that relays one old snapshot forward as fast as it can:

t= 1.1s 212 conflicting VXIDs
t= 5.5s 363 population stops growing, no connection slots left
t= 6.6s 277 cutoff has passed, cancellation begins
t= 7.7s 190
t= 8.8s 105
t= 9.9s 16
t=11.0s 0 replay resumes

The standby begins killing VXIDs and kills them too fast for a
sustained relay - at least on my machine with this setup. The standby
calls pg_usleep(5000) after each signal, which is why the
population remains above 0 for several seconds after the cutoff.

--
Scott Ray

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

Thanks for posting v3.

1. v3 still exposes snapshot-importing backends to wrong results.
Consider procs Exporter, Recovery, and Importer:

(a) Recovery scans, sets Exporter's recoveryConflictTracked to 1, and
begins to wait for the transaction to end using VirtualXactLock().

(b) Importer calls ProcArrayInstallImportedXmin(), acquires the
ProcArrayLock exclusively, and stalls just before reading
recoveryConflictTracked, for example because the OS preempts it.

(c) Exporter commits and takes the lockless branch of
ProcArrayEndTransaction().

(d) Recovery sees Exporter's transaction end, calls
ProcArrayClearRecoveryConflictTracked(), and resumes replay.

(e) Importer resumes, reads that recoveryConflictTracked contains 0,
and finishes importing the snapshot.

(f) Importer now holds a snapshot that needs tuple versions or index
entries that Recovery has removed.

Holding ProcArrayLock in shared mode while clearing
recoveryConflictTracked would prevent this race.

2. v3 can block importing snapshots even when the snapshots do not
conflict with recovery. Consider procs Exporter, Recovery, Importer,
and Blocker:

(a) Recovery scans, sets Exporter and Blocker's
recoveryConflictTracked to 1, and begins to wait for Blocker.

(b) Exporter's transaction ends and it begins a new transaction with
a new snapshot that does not conflict.

(c) Importer attempts to import Exporter's new snapshot, but
recoveryConflictTracked is still 1, and so import fails. The user
receives the false error detail "The source process with PID %d is not
running anymore."

(d) This condition persists until Blocker's transaction completes and
Recovery reaches Exporter's old VXID in the waitlist, at which point
it calls ProcArrayClearRecoveryConflictTracked() and import may
succeed.

GetConflictingVirtualXIDs() could store the lxid of the conflicting
transaction and ProcArrayInstallImportedXmin() could check this value to
determine whether it matches the source of the import. The false error
detail requires a separate fix.

3. This approach can cause parallel pg_dump failures.

(a) The leader connects, opens a transaction, and exports its
snapshot. It does not immediately fork workers. Instead, it
continues preparing, including a full scan of the catalog to find
every table to dump.

(b) GetConflictingVirtualXIDs() sets recoveryConflictTracked to 1 for
the leader.

(c) The leader finishes setting up and forks workers.

(d) Each worker attempts to import the leader's snapshot but fails,
and pg_dump exits with "a worker process died unexpectedly".

The user may set max_standby_streaming_delay to -1 to prioritize
operations on the standby over recovery progress. Raising an error if
a proc tries to import a conflicting snapshot lets the operation die
instead of delaying recovery, regardless of the user's stated
preference.

4. v3 offers no clear backpatch strategy. At its current location,
recoveryConflictTracked breaks ABI compatibility by displacing PGPROC
members including lwWaiting. Also, some of the components v3 uses
postdate affected, supported branches: PG 14 through 16 lack injection
points; and no stable release uses RecoveryConflictReason.

--
Scott Ray

Hi,

Thanks for the detailed review. I agree that the implementation in v3
has problems and should not be revised incrementally.

I have also looked again at the rescan approach in v2. I agree that
the deadline is not reset, so a new conflict found by the next scan
will be cancelled without another grace period. However, this only
bounds how long recovery waits for each observed conflict; it does not
bound how many new conflicts can be created.

Until a signalled source actually exits, another backend may still
import its snapshot and subsequently become a source for a snapshot
retaining the same conflicting xmin. The same handoff can then happen
again with the new exporter. I do not see an invariant in v2 that
prevents this chain from continuing, so I am concerned that the outer
rescan loop does not have a strict termination guarantee. Am I missing
such an invariant?

If there is no such invariant, preventing further propagation after
the deadline appears to require some additional shared state, allowing
conflicting imports to be rejected while recovery drains the remaining
conflicts. Such a rejection may be consistent with the semantics of a
finite standby delay, but I do not yet see a sufficiently simple way
to publish and clear this state safely.

I will rethink the approach before posting another version.

Regards,
Chee