Re: AIO v2.5

On Tue, Mar 18, 2025 at 4:12 PM Andres Freund <andres@anarazel.de> wrote:
>
> Attached is v2.10

This is a review of 0002:  bufmgr: Improve stats when buffer is read
in concurrently

In the commit message, it might be worth distinguishing that
pg_stat_io and vacuum didn't double count reads, they under-counted
hits. pgBufferUsage and relation-level stats (pg_stat_all_tables etc)
overcounted reads and undercounted hits.

Quick example:
On master, if we try to read 7 blocks and 3 were hits and 2 were
completed by someone else then
- pg_stat_io and VacuumCostBalance would record 3 hits and 2 reads,
which looks like 2 misses.
- pgBufferUsage would record 3 hits and 4 reads, which looks like 4 misses.
- pg_stat_all_tables would record 3 hits and 7 reads, which looks like 4 misses.

The correct number of misses is 2 misses comprising 5 hits and 2 reads
(or 7 reads and 5 hits for pg_stat_all_tables which does the math
later).

@@ -1463,8 +1450,13 @@ WaitReadBuffers(ReadBuffersOperation *operation)
        if (!WaitReadBuffersCanStartIO(buffers[i], false))
        {
            /*
-            * Report this as a 'hit' for this backend, even though it must
-            * have started out as a miss in PinBufferForBlock().
+            * Report and track this as a 'hit' for this backend, even though
+            * it must have started out as a miss in PinBufferForBlock().
+            *
+            * Some of the accesses would otherwise never be counted (e.g.
+            * pgBufferUsage) or counted as a miss (e.g.
+            * pgstat_count_buffer_hit(), as we always call
+            * pgstat_count_buffer_read()).
             */

I think this comment should be changed. It reads like something
written when discovering this problem and not like something useful in
the future. I think you can probably drop the whole second paragraph.

You could make it even more clear by mentioning that the other backend
will count it as a read.

Otherwise, LGTM


- Melanie