Hi, Xuneng! On Sun, Sep 6, 2026 at 12:11 PM Xuneng Zhou <[email protected]> wrote: > > On Sun, Sep 6, 2026 at 10:35 AM Xuneng Zhou <[email protected]> wrote: > > On Wed, Aug 26, 2026 at 1:31 PM Xuneng Zhou <[email protected]> wrote: > > > 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. > > Attached is a reproducer for the described scenario.
Thanks for the patches. I've revised 0001; 0002 and 0003 look correct to me as posted. Changes I made to 0001: 1. The docs said the wait is rejected when the session holds a lock, but the code only rejects waits that are not already satisfied (lsn > GetCurrentLSNForWaitType()). Reworded, and stated explicitly that a wait whose target has already been reached always returns. 2. The message now names the offending lock, using DescribeLockTag(). 3. Dropped "heavyweight" from the user-facing messages. The term is defined for users in monitoring.sgml, but no backend message uses it, and the errdetail repeated it; it now explains the cycle instead. 4. Strengthened the justification for covering standby_write and standby_flush, in the comment, the docs and the commit message. What I think is the decisive argument was missing: a blocked startup process also stops creating restartpoints, and CreateRestartPoint() is what calls RemoveOldXlogFiles(), so pg_wal grows until the filesystem fills and reception stops as well. The independence streaming provides is bounded by free space in pg_wal, not durable for the life of the wait. 5. Documented the recommended usage pattern: issue WAIT FOR outside a transaction block, or as its first statement, before anything that takes locks. Including the trap that a lock from an earlier statement is still held at READ COMMITTED even though its snapshot is gone, so a wait placed after such a statement is rejected even though the isolation check above it passes. 6. Added a test for the already-satisfied case, since the docs now state that rule. For 0002, I only removed inline from PrimaryFlushWakeupProcessRequests(). I think it's not required for a function existing within one module with one caller (compiler may take decision to inline it by itself). I'm still considering your question 4, will return with my opinion on it a bit later. ------ Regards, Alexander Korotkov Supabase
v2-0001-Prevent-WAIT-FOR-LSN-from-deadlocking-recovery.patch
Description: Binary data
v2-0002-Wake-primary_flush-waiters-after-implicit-WAL-flu.patch
Description: Binary data
v2-0003-Re-read-standby-LSN-after-recovery-ends.patch
Description: Binary data
