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.