Re: Implement waiting for wal lsn replay: reloaded

On Wed, Aug 26, 2026 at 1:31 PM Xuneng Zhou <xunengzhou(at)gmail(dot)com> wrote:
>
> Hi Alexander,
>
> Thanks for taking care of the above patches.
>
> Here are four more to go. Your thoughts are appreciated. Sorry for
> posting them late -- I underestimated the subtlety of them and the
> time required to dispel some portion of that subtlety, plus being
> sidetracked from thread to thread from time to time in the Odyssey of
> issue reporting.
>
> [Alert] To accelerate the pace of bug fixing in this phase, some of
> the writing below is co-authored with Sol. I remain responsible for
> eliminating its hallucination and mine.
>
> 1) An unwanted survival after the hard-fought battle against deadlock
>
> [Disclosure] Sol did the first round investigation of a two-cycle
> deadlock caused by holding relation lock, but it failed to generalize
> the problem to three-cycles, rejected my v19 fix proposal which I
> disagreed with and proposed several fixes which were turned down by
> me. I took the helm for most of the analysis.
>
> ----- Prologue
>
> Waiting from too long to indefinite is what the command tried
> relentlessly to avoid. To achieve this, lots of trade-offs &
> compromises have been made regarding the snapshot management, let
> alone the interface has been metamorphosed several times. However,
> there seems to be an unwanted survival after the hard-fought battle.
> Waiting in standby_replay, aka the default mode, can form a deadlock
> with the startup process when executed by a transaction that retains
> locks from earlier statements at READ COMMITTED. Unfortunately, this
> deadlock could be permanent in certain scenarios.
>
> ----- Direct cycle
>
> Consider a standby backend B:
> BEGIN;
> SELECT * FROM tb;
> WAIT FOR LSN '<future-lsn>' WITH (MODE 'standby_replay'); / WAIT FOR
> LSN '<future-lsn>';
>
> The SELECT snapshot is released at statement end, so the snapshot
> check permits the subsequent WAIT FOR. However, its AccessShareLock on
> tb remains held until transaction end. If WAL below the target LSN
> contains a DDL operation requiring recovery to acquire
> AccessExclusiveLock on tb, such as ALTER TABLE or DROP TABLE, the
> dependencies become:
>
> B waits for startup S to advance replay
> S waits for B to release AccessShareLock(tb)
>
> B -> S -> B
>
> Both processes are then waiting for progress that only the other can provide.
>
> -- Why the deadlock is not detected
> The startup process's heavyweight-lock wait is represented normally:
>
> S -> B
>
> Backend B's replay dependency is not represented in the
> heavyweight-lock graph. WaitForLSN() sleeps on the backend latch,
> rather than through ProcSleep():
>
> GetAwaitedLock() == NULL
> B is not attached to a heavyweight-lock wait queue
>
> When startup's recovery deadlock timeout expires, it sends B a
> RECOVERY_CONFLICT_STARTUP_DEADLOCK request. The current handler
> contains the assumption that a backend not waiting for a heavyweight
> lock cannot be deadlocked:
>
> if (GetAwaitedLock() == NULL)
> return;
>
> Consequently, B ignores the request.
> The actual and represented graphs differ as follows:
>
> Actual graph: B -> S -> B
> Represented graph: S -> B
> Missing dependency: B -> S
>
> This is not a lost-wakeup race. Both processes are correctly asleep,
> but the dependency connecting the LSN-wait subsystem to the lock
> manager is absent from deadlock detection.
> The problem occurs in either ordering:
> 1. B begins waiting first, after which startup blocks and probes B; B
> ignores the probe.
> 2. Startup blocks and completes its probe first, after which B begins
> waiting; startup does not guarantee another probe.
>
> ----- Permanent behavior with unlimited standby delay
>
> With a finite max_standby_streaming_delay or
> max_standby_archive_delay, the standby deadline eventually resolves
> the situation as an ordinary recovery conflict. The waiting
> transaction is canceled, its locks are released, and replay resumes.
> With the relevant standby delay set to -1, however, there is no such
> deadline. GetStandbyLimitTime() represents this as an unlimited wait.
> After its deadlock probe, startup can enter an untimed second wait for
> the relation lock. If the WAIT FOR command also has no timeout, the
> cycle has no autonomous breaker:
>
> B cannot finish until startup replays
> startup cannot replay until B finishes
>
> The result is an indefinite replay stall requiring external
> intervention, such as canceling or terminating the backend, ending its
> transaction, or promoting the standby.
>
> ------ Indirect cycle
>
> If the direct two-process cycle were the whole problem, the fix would
> be much simpler. However,
>
> The missing dependency also permits longer cycles. For example:
> B holds advisory lock L and waits for replay
> C holds AccessShareLock(tb) and waits for L
> S waits for AccessExclusiveLock(tb)
>
> The actual graph is:
> B -> S -> C -> B
> The heavyweight detector can represent:
> S -> C -> B
>
> but traversal stops when it reaches B because B is sleeping in
> WaitForLSN() rather than waiting for a heavyweight lock. This
> demonstrates that the issue is not limited to the replay waiter
> directly holding startup’s relation lock. Apart from the advisory
> lock, can other heavy weight locks participate in the problematic
> three-cycle?
>
> Here is the current-core assessment:
>
> On a hot standby, LockAcquireExtended() refuses any relation or object
> lock stronger than RowExclusiveLock, and every mode conflicting with
> AccessShareLock, RowShareLock, or RowExclusiveLock is itself stronger
> than that. Two ordinary backends therefore cannot conflict on a
> relation or object lock; only the startup process, which bypasses the
> check, can hold AccessExclusiveLock. LOCK TABLE is classified to
> match.
>
> The remaining classes fail on the holder side. A standby backend never
> obtains an XID, so it cannot hold a transaction-ID lock. Tuple, page,
> and speculative-token locks are taken only on write paths, as is
> relation extension — which is excluded from cycle detection outright
> in any case. A backend does hold its own VXID lock, but
> VirtualXactLock() has exactly three callers: WaitForLockersMultiple()
> and WaitForOlderSnapshots(), both DDL-only, and the startup process's
> own non-blocking poll.
>
> That leaves advisory locks as the only core construction.
> Extension-defined locktags remain open-ended, since the recovery
> restriction covers only LOCKTAG_RELATION and LOCKTAG_OBJECT.
>
> ------ My proposal for v19
>
> Add a conservative fail-fast rule: before standby_replay wait, reject
> it if the backend owns any granted heavyweight lock recorded in
> 'LockMethodLocalHash' ('locallock->nLocks > 0').
>
> Although only relation- and advisory-lock cycles are the main concerns
> here, limiting the check to those lock types would encode assumptions
> about which core, extension, or future paths can wait on other lock
> classes. Any locally represented heavyweight lock could become the
> final edge back to the replay waiter. Scanning all granted 'LOCALLOCK'
> entries seems simpler, more robust, and avoids maintaining a fragile
> lock-type whitelist. The backend’s implicit VXID is not included
> because it is not recorded in 'LockMethodLocalHash' and including it
> would reject every transaction. No ordinary core hot-standby SQL
> construction for C -> B through B's VXID has been shown.

B's VXID can still do bad things in other ways, will reply later so as
to bring it to justice...

> Current uses
> of WAIT FOR in tap tests are unaffected by the new proposal per
> inspection by Sol. All local tests passed.
>
> The implications for this fixes is that the waiting is rejected even when:
> - WAL before the target never requests a conflicting lock on that table;
> - no other backend is waiting for B;
> - an advisory lock held by B is unused;
> - the lock is on an object that recovery cannot currently conflict with;
> - the wait has a finite timeout, if the rule is applied to bounded waits too.
>
> The question for the fix is whether these new limitations introduced
> by the proposed fix are considered as acceptable.
>
> ------ My confusion for v20
>
> The essential problem is that 'WAIT FOR ... standby_replay' allows a
> transactional backend to wait indefinitely for the startup process
> while retaining resources that can make startup depend, directly or
> indirectly, on that same backend. Neither this replay dependency is
> represented in its deadlock graph nor guarantees that another
> mechanism will be guaranteed to break the resulting cycle.
>
> The tempting PG20 solution is therefore to represent the
> backend-to-startup edge explicitly and extend deadlock detection to
> traverse mixed replay and heavyweight-lock dependencies. Doing so
> properly, however, requires substantial changes to edge publication
> and cleanup, synchronization, detector traversal, reporting, and
> victim handling. Given the relatively narrow practical scope of the
> problem, that implementation complexity may not be justified by its
> benefit.
>
> 2) Missing wake-up point for primary-flush waiters (walsenders maybe)
>
> There is a path that can advance the primary flush position without
> waking primary_flush waiters. When WAL buffers are full,
> AdvanceXLInsertBuffer() may call XLogWrite() with Flush =
> InvalidXLogRecPtr, requesting only a write. However, if that write
> completes a WAL segment, XLogWrite() fsyncs the segment and advances
> the flush position implicitly. This progress is published, but no
> WaitLSNWakeup() follows on this path. A waiter can therefore remain
> asleep even though pg_current_wal_flush_lsn() has already reached its
> target, until an unrelated later flush or checkpoint wakes it. The
> existing wakeup sites in XLogFlush() and XLogBackgroundFlush() mirror
> the walsender design: XLogWrite() requests notification while holding
> WAL locks, and the caller performs the actual wakeup after releasing
> them. The buffer-recycling path falls outside those two caller sites.
> It also exposes a similar pre-existing gap for deferred walsender
> wakeups, which is not changed by this patch.
>
> The patch applies the same deferred design to primary-flush waiters.
> XLogWrite() records in a process-local flag when it advances the flush
> position, after publishing that progress. XLogInsertRecord() processes
> the request only after releasing the WAL write and insertion locks.
> This avoids acquiring WaitLSNLock or setting waiter latches while
> holding heavily contended WAL locks.
>
> The test uses a dedicated node with 1MB WAL segments and four WAL
> buffers. It parks the WAL writer before XLogBackgroundFlush(),
> registers a primary_flush waiter, and stops it immediately after
> registration. A large, non-flushing, xid-less logical message then
> forces WAL-buffer recycling across the segment boundary. The
> test_wait_lsn module checks the waiter's shared-memory registration
> directly before and after the implicit flush. This avoids timing-based
> assertions: the test proves that the target became durable and that
> the corresponding wakeup removed the waiter, while preventing the WAL
> writer or transaction completion from masking the path under test.
>
> 3) Re-read WAIT FOR LSN position after promotion
> There seems to be a race in WaitForLSN() when a standby leaves
> recovery. WaitForLSN() reads the current LSN before checking
> RecoveryInProgress(). If recovery reaches the target and ends between
> those operations, the function observes that recovery has ended but
> compares the target with the stale, pre-promotion LSN. It can
> therefore return WAIT_LSN_RESULT_NOT_IN_RECOVERY even though the final
> position reached the target.
>
> The attached patch re-reads the current LSN after observing that
> recovery has ended. At that point the final recovery position is
> stable, so the promotion result is based on the latest value.
>
> 4）WAIT FOR LSN result policy when recovery ends
> The current code treats an explicit promotion request specially.
> After observing that recovery has ended, WaitForLSN() rereads the
> final LSN and returns success only when PromoteIsTriggered() is true
> and the target has
> been reached. Otherwise it returns not-in-recovery. As I understand
> it, this produces the following behavior:
>
> - If the backend observes the target while recovery is still active,
> it returns success.
>
> - After an explicit promotion request, it returns success if the final
> LSN reached the target and not-in-recovery otherwise.
>
> - A command issued after explicit promotion can likewise return
> success for an already-replayed target, because PromoteIsTriggered()
> remains set.
>
> - If recovery finishes through recovery_target_action=promote, without
> an explicit promotion request, a waiter that observes the end of
> recovery returns not-in-recovery even if the final numeric LSN has
> reached its target. A waiter that observed the target before recovery
> ended has already returned success.
>
> - With recovery_target_action=pause, pausing itself does not end
> recovery. An unsatisfied waiter continues waiting. If recovery is
> then resumed and finishes without an explicit promotion request, the
> remaining waiter returns not-in-recovery. An explicit promotion while
> paused follows the explicit-promotion behavior above.
>
> - If archive recovery otherwise finishes without an explicit
> promotion, a remaining waiter returns not-in-recovery.
>
> - With recovery_target_action=shutdown, the connection is terminated
> and the command returns no WAIT FOR status.
>
> - On an ordinary primary, the command returns not-in-recovery rather
> than treating its historical replay position as satisfying a standby
> wait.
>
> This can be viewed as a state-oriented policy: standby modes are valid
> only during recovery, with an explicit-promotion exception for
> checking the final replay position. Under that policy,
> not-in-recovery does not necessarily assert that the numeric target
> was never replayed; it reports that recovery ended through a path for
> which the standby wait is no longer valid. This seems like a
> defensible conservative policy. Changing the decision to depend only
> on targetLSN <= currentLSN would broaden the contract by allowing
> every recovery-ending path to produce success from the historical
> replay position. Was this distinction between explicit promotion and
> other ways of ending recovery intentional? If so, perhaps the doc
> should clarify that the post-recovery success exception applies
> specifically to an explicit promotion request.
>
> --
> Regards,
> Xuneng Zhou
> HighGo Software Co., Ltd.

--
Regards,
Xuneng Zhou
HighGo Software Co., Ltd.