From 837b4fd5ef9ba79c6881eb2eb5da38c9502a9721 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Tue, 2 Jun 2026 15:38:47 -0400 Subject: [PATCH v16 07/21] pg_stat_io: Count buffer eviction after invalidation GetVictimBuffer() may double count pg_stat_io evictions/reuses when we attempt to invalidate the buffer but some other backend has already redirtied it or pinned it. This should be rare, but fixing it clarifies the code. This didn't seem like a serious enough issue to backpatch. Author: Melanie Plageman Reviewed-by: Andrey Borodin Discussion: https://postgr.es/m/D0E7C430-76A9-4030-B8AB-109E7D7ED8C1%40yandex-team.ru --- src/backend/storage/buffer/bufmgr.c | 34 ++++++++++++++++------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 7842fe812ae..e14c5f9ba88 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -2606,6 +2606,7 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) Buffer buf; uint64 buf_state; bool from_ring; + bool buf_valid; /* * Ensure, before we pin a victim buffer, that there's a free refcount @@ -2693,8 +2694,21 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) &buf_hdr->tag); } + /* Don't use a stale buf_state value after InvalidateVictimBuffer */ + buf_valid = buf_state & BM_VALID; - if (buf_state & BM_VALID) + /* + * If the buffer has an entry in the buffer mapping table, delete it. This + * can fail because another backend could have pinned or dirtied the + * buffer. + */ + if ((buf_state & BM_TAG_VALID) && !InvalidateVictimBuffer(buf_hdr)) + { + UnpinBuffer(buf_hdr); + goto again; + } + + if (buf_valid) { /* * When a BufferAccessStrategy is in use, blocks evicted from shared @@ -2708,25 +2722,15 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) * counted as IOOP_REUSE in the corresponding strategy context. * * At this point, we can accurately count evictions and reuses, - * because we have successfully claimed the valid buffer. Previously, - * we may have been forced to release the buffer due to concurrent - * pinners or erroring out. + * because we have successfully claimed the valid buffer and + * invalidated its previous tenant. Previously, we may have been + * forced to release the buffer due to concurrent pinners or erroring + * out. */ pgstat_count_io_op(IOOBJECT_RELATION, io_context, from_ring ? IOOP_REUSE : IOOP_EVICT, 1, 0); } - /* - * If the buffer has an entry in the buffer mapping table, delete it. This - * can fail because another backend could have pinned or dirtied the - * buffer. - */ - if ((buf_state & BM_TAG_VALID) && !InvalidateVictimBuffer(buf_hdr)) - { - UnpinBuffer(buf_hdr); - goto again; - } - /* a final set of sanity checks */ #ifdef USE_ASSERT_CHECKING buf_state = pg_atomic_read_u64(&buf_hdr->state); -- 2.47.3