Re: Recovery conflict resolution misses backends that import snapshots

From: Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Scott Ray <scott(at)scottray(dot)io>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, "Zhijie Hou (Fujitsu)" <houzj(dot)fnst(at)fujitsu(dot)com>, Kuroda, Hayato/黒田 隼人 <kuroda(dot)hayato(at)fujitsu(dot)com>, Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com>
Subject: Re: Recovery conflict resolution misses backends that import snapshots
Date: 2026-07-28 04:44:27
Message-ID: CAA4eK1K09ZZWF-5xxVRL=XMwg=COGCRM8X2bFJJaoYkucXB8PA@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, Jul 27, 2026 at 5:19 AM Michael Paquier <michael(at)paquier(dot)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.

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Smith 2026-07-28 04:59:29 Missing list_free in publicationcmds.c:OpenTableList
Previous Message Alexander Lakhin 2026-07-28 04:00:00 Re: aio tests failing on newer Linux kernels