On Tue, Mar 18, 2025 at 4:12 PM Andres Freund <and...@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
