Re: Use WALReadFromBuffers in more places

On Tue, 8 Sept 2026 at 01:57, Bharath Rupireddy
<bharath(dot)rupireddyforpostgres(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Thu, Sep 3, 2026 at 10:41 PM Michael Paquier <michael(at)paquier(dot)xyz> 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