Re: Implement waiting for wal lsn replay: reloaded - Mailing list pgsql-hackers

Hi, Xuneng!

On Sun, Sep 6, 2026 at 12:11 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:
>
> On Sun, Sep 6, 2026 at 10:35 AM Xuneng Zhou <xunengzhou@gmail.com> wrote:
> > On Wed, Aug 26, 2026 at 1:31 PM Xuneng Zhou <xunengzhou@gmail.com> 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