On Fri, Sep 11, 2026 at 5:59 AM Alexander Korotkov <[email protected]> wrote: > > On Wed, Sep 9, 2026 at 8:39 AM Xuneng Zhou <[email protected]> wrote: > > On Wed, Sep 9, 2026 at 11:38 AM Xuneng Zhou <[email protected]> wrote: > > > > > > On Mon, Sep 7, 2026 at 7:28 PM Alexander Korotkov <[email protected]> > > > wrote: > > > > > > > > Hi, Xuneng! > > > > Thanks for the patches. I've revised 0001; 0002 and 0003 look correct > > > > to me as posted. > > > > > > Thanks for updating the patches! 0001 looks much better after revisement. > > > > > > > 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. > > > > > > LGTM. > > > > > > > 2. The message now names the offending lock, using DescribeLockTag(). > > > > > > + 1. It can be useful. > > > > > > > 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. > > > > > > +1 > > > > > > > 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. > > > > > > Thanks for pointing this out. I looked into the described scenario, > > > and it does look more concerning than my original report, which > > > strengthens extending the restriction. > > > > > > > 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. > > > > > > This seems helpful to me. > > > > > > > 6. Added a test for the already-satisfied case, since the docs now > > > > state that rule. > > > > > > I made some minor adjustments to the commit message, doc and comment > > > for the restriction, regarding the forming of deadlock cycle, why it > > > cannot be detected and breaked with current probing mechanism, and the > > > cases of unguaranteed independence streaming. > > > > > > > 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). > > > > > > Make sense. Verified this by compiling the patched xlog.c with and > > > without inline, using local Apple Clang 21 and configured -O2 flags. > > > Both versions inlined the helper into XLogInsertRecord() and produced > > > a byte-for-byte identical assembly. > > > > While reading the 0001 patch, I noticed that the current note section is > > kinda not easy to follow after adding more restrictions and caveats over > > time. It has eight paragraphs after the 0001 patch. The first four is a > > discussion of execution restrictions, but then the text returns to > > explaining every mode before moving into promotion, timelines, and recovery > > conflicts. That interruption makes the section feel a bit scattered. Using > > subsections like caveats, restrictions, edge cases might help, but their > > boundaries seem not very clear to me. Therefore, I am considering grouping > > and reorganizing this section by sub-topics. The section after the 0004 > > patch will look like this: > > > > refentry: WAIT FOR > > └── refsect1: Notes > > ├── refsect2: Execution Restrictions > > │ ├── itemizedlist > > │ │ ├── listitem → para: execution and snapshot restrictions > > │ │ └── listitem → para: held-lock restriction and exception > > │ ├── para: recommended statement order > > │ ├── para: deadlock rationale > > │ └── para: why write/flush modes are also restricted > > └── refsect2: Recovery and Timelines > > ├── para: local promotion > > ├── para: upstream timeline changes > > └── para: recovery-conflict interruptions > > > > Is this sensible? > > I think 0001-0003 are ready. I'm going to push them if no objections. > And I'll review of 0004 in the next couple days.
Thanks. Before the move, would you mind change a line of comment for the restriction in v3-0001 from + * WAIT FOR LSN does not register its dependency on WAL progress as a to * However, we do not register our dependency on WAL progress as a I did not adjust it properly in the last try. -- Regards, Xuneng Zhou HighGo Software Co., Ltd.
v3-0001-Prevent-WAIT-FOR-LSN-from-deadlocking-recovery-on.patch
Description: Binary data
