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. ------ Regards, Alexander Korotkov Supabase
