Re: Use WALReadFromBuffers in more places

On Mon, Sep 14, 2026 at 01:07:00PM -0700, Bharath Rupireddy wrote:
> I noticed a CF bot failure in 001_rep_changes.pl because the logical
> walsender was reading all the WAL from WAL buffers, so the disk read
> path was never hit. This caused the TAP test that expects at least one
> disk read to time out. This reminds me of a missing piece in the
> WALReadFromBuffers() journey, that is, reporting how often WAL is read
> from WAL buffers. So far the physical walsender TAP test hasn't
> complained, but in the 0001 patch I added support to report WAL buffer
> hits using the existing IOOP_HIT operation and adjusted the physical
> walsender test accordingly. Note that I couldn't add byte-level hit
> tracking in 0001, which I plan to do separately. The added hit counter
> for WAL is enough to know how often reads come from WAL buffers and
> will fix the CF bot failure.

Hmm. Okay but..

> Another idea to resolve this is to just count reads from WAL buffers
> into the existing reads and read_bytes, the same way WALRead() does,
> and be done with it. This is simpler, though one can't distinguish or
> know how often WAL buffers are hit and reading from WAL files is
> avoided. One argument in favor of this approach is to just treat WAL
> buffer hits like OS page cache hits, and since we don't count those in
> pg_stat_io, that is okay. But I prefer using the hits operation unless
> anyone thinks otherwise.

+ /*
+ * Report the read as a hit on the WAL buffers, so that it can be
+ * distinguished from a read from a file. A read that finds only part of
+ * the requested WAL here is also reported by the file read that the
+ * caller then does, for the rest of it.
+ */
+ if (nread > 0)
+ pgstat_count_io_op(IOOBJECT_WAL, IOCONTEXT_NORMAL, IOOP_HIT, 1, 0);
+
+ return nread;

... It looks to me that you do not have the right idea with v9-0001,
where you decide to count the number of *times* we call
WALReadFromBuffers(). It would be more correct to me to count the
number of hits as of the *number of pages* we are able to read while
attempting to retrieve "count" bytes worth of WAL in a single call of
WALReadFromBuffers(). If some data has been evicted, we may read less
data, but the number of pages hit still seems like the most relevant
part. With that, it is possible to do an educated estimation of how
many data worth XLOG_BLCKSZ has been found. It does not have to be
100% precise, just good enough to know the balance between buffer
reads and physical reads. If we read for example 1GB worth of data in
one call of WALReadFromBuffers() it would be confusing to count that
as 1 hit, that should be (1024^3 / XLOG_BLCKSZ) hits. Let's use a
counter that is incremented while scanning the pages through the while
loop, then increment pg_stat_io once at the end of
WALReadFromBuffers() if (nread > 0).

I am not really convinced that there is a need to have the number of
bytes if we know the number of hits. Even with MAX_SEND_SIZE worth
*only* 16 pages, sending very less than 8kB of data is IMO very
unlikely these days on systems where we care about the IO activity.
The number of bytes would become useful if we send for example a bunch
of small-ish messages in the WAL sender.

> 0002 through 0004 are unchanged, except that the logical walsender
> test in 0002 now waits for the sum of reads and hits, since a read
> from the WAL buffers is reported as a hit and not as a read.

In terms of 0002, 0003 and 0005, anybody who does a serious review of
this patch set will need to run benchmarks to confirm your points. If
you could post a script, that would be useful IMO. The difficulty of
this patch is not its implementation, it's the testing.

I am not convinced that 0004 offers much value if integrated, still it
seems nice to have to stress the CI.
--
Michael