On Tue, 8 Sept 2026 at 01:57, Bharath Rupireddy <[email protected]> wrote: > > Hi, > > On Thu, Sep 3, 2026 at 10:41 PM Michael Paquier <[email protected]> wrote: > > > > + } > > + else if (state->seg.ws_file >= 0) > > + { > > + /* > > + * A read fully satisfied from WAL buffers skips WALRead(), which > > is > > + * where ws_file is closed and reopened as the reader crosses > > + * segments. So a buffer-only read never notices the segment > > change. > > + * ws_file stays open on the old segment while ReadPageInternal() > > + * advances ws_segno. For example, when the first page of segment 2 > > + * comes from buffers, ws_segno becomes 2 but ws_file is still > > open on > > + * segment 1. A later read of segment 2 that falls back to the file > > + * reuses the stale descriptor, since WALRead() decides whether to > > + * reopen from ws_segno (already 2) rather than the open file. It > > + * reads segment 1 and returns the wrong segment's WAL, seen during > > + * decoding as an "unexpected pageaddr" error. Close the segment > > after > > + * a buffer-only read so the next file read reopens the correct > > one. > > + */ > > + state->routine.segment_close(state); > > + } > > > > Cannot that become wasteful for the logical path when reading pages > > from the same segment repeatedly causing opening and closing of the > > same file? That sounds relevant to me if we are still attempting > > to read from the same segment, depending on wal_buffers whose default > > is at 4MB. Something like an extra check based on XLByteInSeg() may > > be adapted, using the targetPagePtr, where we could close the segment > > only if we target a page not on the same segment? > > > > The same argument applies to both v6-0001 and v6-0002, both > > unconditionally closing a segment after completing a read from buffer > > or even not completing a read from buffers and completing the read > > with an extra WALRead(). > > Ah, yes, that is not acceptable. Nice catch! Fixed it by closing the > old segment only when the first WAL page of the new segment is fully > read from buffers. > > > + Assert(rbytes == count); > > > > This assert feels redundant due to the other checks done above. > > Applies to patches 0001 and 0002. > > Removed. > > Please find the attached v7 patches. > > -- > Bharath Rupireddy > Amazon Web Services: https://aws.amazon.com
Hi! I noticed this patch did a small benchmarking on v6/v7, primary-standby on single vm. Seems like XLByteInSeg is really needful here, but I didn't find any v6 perf regression in close() syscall spam. I can share my bench scripts if needed. reads/bytes in benches measured using pg_stat_io ## Results: catch-up subscriber ``` Build TPS walsender reads walsender read_bytes read_time ─────────────────────────────────────────────── HEAD + DIO 49,942 49,751 407 MB 1.66 ms Patched v7 + DIO 46,469 124 818 KB 0 ms ─────────────────────────────────────────────── Delta — -99.8% — ``` The patch virtually eliminates walsender file reads (407 MB -> 818 KB). ## Results: lagging subscriber ``` Build TPS walsender reads walsender read_bytes ──────────────────────────────────────── HEAD + DIO 47,802 127,119 1,041 MB Patched v7 + DIO 48,956 71,118 583 MB ──────────────────────────────────────── Delta — -44% ``` With a lagging subscriber, the patch reduces file reads by 44% and improves TPS (probably noise). Code itself looks fine to me, don't see any major issues. In 0001 this comment looks unnecessarily big for me: > + /* > + * A read fully satisfied from WAL buffers skips WALRead(), which is > + * where ws_file is closed and reopened as the reader crosses > + * segments. So a buffer-only read never notices the segment change. > + * ws_file stays open on the old segment while ReadPageInternal() > + * advances ws_segno. For example, when the first page of segment 2 > + * comes from buffers, ws_segno becomes 2 but ws_file is still open on > + * segment 1. A later read of segment 2 that falls back to the file > + * reuses the stale descriptor, since WALRead() decides whether to > + * reopen from ws_segno (already 2) rather than the open file. It > + * reads segment 1 and returns the wrong segment's WAL, seen during > + * decoding as an "unexpected pageaddr" error. Close the segment when > + * the page just read from buffers is not in the open segment, so the > + * next file read reopens the correct one. Reads staying within the > + * open segment leave it alone, because ws_segno does not change. > + */ Isn't this just a very detailed way to say "close WAL segment that you ought to close?". I think 0002 comment + /* + * Close the segment when a read fully satisfied from WAL buffers is + * not in the open segment, so the next file read reopens the correct + * one. See logical_read_xlog_page() for why this is needed. + */ Is ok. v7 is neutral for buffered io (no regression/only 1-2% noise) and saves a lot of IO for direct IO. Also here [1] & [2], you (and Andres) suggest sending WAL before it has been locally written out and flushed to improve synchronous replication performance. I have actually made a simple PoC on this recently ([3] code and benchmarks used), and it didn't show some improvement (even with standby applying all wal from received primary, beyond flush ptr. ~6-7% improvement at low WAL volume, parity at high volume). So, can you share a thread with this work or start one? Small summary for results [1] https://www.postgresql.org/message-id/CALj2ACVzJPJMLi%2BuhU8Hvw6sYiTOVy-vetkZ4QTQ%3DTOE8dygkA%40mail.gmail.com [2] https://www.postgresql.org/message-id/20230125211540.zylu74dj2uuh3k7w%40awork3.anarazel.de [3] https://github.com/pg-sharding/cpg/pull/107/commits [4] https://github.com/pg-sharding/cpg/tree/wal_flush/bench/walrcvflusher/scripts -- Best regards, Kirill Reshke
