Re: Use WALReadFromBuffers in more places

On Thu, Sep 03, 2026 at 05:23:00PM -0700, Bharath Rupireddy wrote:
> Yes, repack and 2PC are covered by the 0002 patch. But I haven't yet
> used it for the WAL summarizer. Would it be okay if I do some testing
> with the WAL summarizer and propose it as a follow-up patch?

I don't expect anything bad, but impossible to say without actual
numbers. I've missed that summarizer_read_local_xlog_page is a
separate code path. If you wish to leave that out now, that's fine.
It could always be dealt with later if needed.

> With WAL direct IO on, the patch reduces the walsender's WAL reads
> from 1.0 GB to 1.6 MB per run, removing 14.5 MB/s of physical disk
> reads and improving publisher throughput by about 17% (6,393 to 7,489
> TPS). The throughput gain comes from eliminating the WAL read IO on
> disk, so WAL writes no longer compete with WAL reads for disk IO. With
> WAL direct IO off, the same reads are eliminated at the syscall level
> with no throughput change, so it never regresses.

I'm not surprised by these results. Thanks for the numbers. That
seems pretty good off the bat. Nice.

> The physical walsender change was supposed to be purely mechanical,
> but upon thinking more, I would rather not touch that part. Also, the
> way I had it, it had a bug where it ignored the retry part. Sorry
> about that.

Okay, I can see that v6 has eliminated the bits in XLogSendPhysical()
compared to v5.

> I split the patches into two. 0001 is for logical walsender, 0002 is
> for local WAL reads. 0003 adds a TAP test using an injection point for
> the segment boundary issue handled in both patches (I don't intend to
> get this committed unless anyone thinks otherwise). A read fully
> satisfied from WAL buffers can leave the segment file open on the
> wrong segment. This is handled by closing the open segment after such
> a read so the next file read reopens the correct one.
>
> PS: There is an opportunity to deduplicate with a wrapper function on
> the code that 0001 and 0002 add for WALReadFromBuffers()+WALRead(). I
> chose not to add that wrapper, just because we cannot reuse it in the
> physical walsenders, defeating the purpose of deduplication. I am open
> to thoughts on this.

Hmm. I am not sure if that would be worth doing. Could be always
wrong, of course.

+ }
+ 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().

+ Assert(rbytes == count);

This assert feels redundant due to the other checks done above.
Applies to patches 0001 and 0002.
--
Michael