On Wed, Aug 26, 2026 at 1:31 PM Xuneng Zhou <[email protected]> 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. Current uses > of WAIT FOR in tap tests are unaffected by the new proposal per > inspection by Sol. All local tests passed.
Sadly, we might need to extend this guard to flush/write waiters as well since the deadlock could form in archive recovery mode. standby_replay = replay position standby_write = max(receiver write position, replay position) standby_flush = max(receiver flush position, replay position) This ceiling makes write/flush progress implicitly depend on the replay. We could somehow relax the restriction for waiters in streaming mode. I don't know whether it is a good time to do so or the complexity is worthwhile. > 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.
v2-0001-Prevent-WAIT-FOR-LSN-from-deadlocking-recovery.patch
Description: Binary data
v2-0003-Re-read-standby-LSN-after-recovery-ends.patch
Description: Binary data
v2-0002-Wake-primary_flush-waiters-after-implicit-WAL-flu.patch
Description: Binary data
