Re: Implement waiting for wal lsn replay: reloaded

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. 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.