From 522e6ae63abdb5a3b130f0077da9912033372cef Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Mon, 17 Aug 2026 16:35:26 -0400 Subject: [PATCH v16 11/21] Introduce ClaimVictimBuffer() helper Extract the code to flush, possibly reject, and invalidate a prospective victim buffer from GetVictimBuffer() into a new helper, ClaimVictimBuffer(). When the claim fails, GetVictimBuffer() retries with another buffer exactly as before. While at it, drop the redundant check for strategy when deciding whether to reject a buffer from the strategy ring. This commit is a refactor only. Author: Melanie Plageman Earlier version Reviewed-by: Andrey Borodin Discussion: https://postgr.es/m/D0E7C430-76A9-4030-B8AB-109E7D7ED8C1%40yandex-team.ru --- src/backend/storage/buffer/bufmgr.c | 160 ++++++++++++++++------------ 1 file changed, 93 insertions(+), 67 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index be2dd9bf7b5..09040bbb3c1 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -679,6 +679,10 @@ static pg_always_inline void TrackBufferHit(IOObject io_object, ForkNumber forknum, BlockNumber blocknum); static uint32 MaxWriteBuffers(void); static Buffer GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context); +static bool ClaimVictimBuffer(BufferAccessStrategy strategy, + BufferDesc *buf_hdr, Buffer bufnum, + uint64 buf_state, + bool from_ring, IOContext io_context); static void FlushUnlockedBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, IOContext io_context); static void FlushBuffer(BufferDesc *buf, SMgrRelation reln, @@ -2580,65 +2584,21 @@ InvalidateVictimBuffer(BufferDesc *buf_hdr) } /* - * Determine the largest IO we can assemble given global constraints on the - * number of pinned buffers and max IO size. Currently only a single write is - * inflight at a time, so the batch can consume all the pinned buffers this - * backend is allowed. Only for batches of shared (non-local) relations. + * Helper to claim a victim buffer -- which is invalidating its existing + * contents (including flushing the old contents first if needed). + * Returns true if it successfully claimed the victim buffer and false if it + * failed to do so. Buffer must already be pinned, but if we fail to claim it + * we will unpin it. */ -static uint32 -MaxWriteBuffers(void) -{ - uint32 result = Min(io_combine_limit, GetPinLimit()); - - /* Ensure forward progress */ - return Max(result, 1); -} - -static Buffer -GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) +static bool +ClaimVictimBuffer(BufferAccessStrategy strategy, + BufferDesc *buf_hdr, Buffer bufnum, uint64 buf_state, + bool from_ring, IOContext io_context) { - BufferDesc *buf_hdr; - Buffer buf; - uint64 buf_state; - bool from_ring; - bool buf_valid; - - /* - * Ensure, before we pin a victim buffer, that there's a free refcount - * entry and resource owner slot for the pin. - */ - ReservePrivateRefCountEntry(); - ResourceOwnerEnlarge(CurrentResourceOwner); - - /* we return here if a prospective victim buffer gets used concurrently */ -again: - - /* - * Select a victim buffer. The buffer is returned pinned and owned by this - * backend. If given a strategy object, see whether it can select a - * buffer; otherwise, or if the ring cannot provide one, get a buffer from - * shared buffers using the clock sweep and record it in the ring. - */ - buf_hdr = NULL; - from_ring = false; - if (strategy) - { - buf_hdr = GetBufferFromRing(strategy, &buf_state); - if (buf_hdr) - from_ring = true; - } - if (!buf_hdr) - { - buf_hdr = GetBufferFromClocksweep(&buf_state); - if (strategy) - AddBufferToRing(strategy, buf_hdr); - } - buf = BufferDescriptorGetBuffer(buf_hdr); - /* * We shouldn't have any other pins for this buffer. */ - CheckBufferIsPinnedOnce(buf); + CheckBufferIsPinnedOnce(bufnum); /* * If the buffer was dirty, try to write it out. There is a race @@ -2666,14 +2626,14 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) * index pages, and the second one just happens to be trying to split * the page the first one got from GetBufferFrom*().) */ - if (!BufferLockConditional(buf, buf_hdr, BUFFER_LOCK_SHARE_EXCLUSIVE)) + if (!BufferLockConditional(bufnum, buf_hdr, BUFFER_LOCK_SHARE_EXCLUSIVE)) { /* * Someone else has locked the buffer, so give it up and loop back * to get another one. */ UnpinBuffer(buf_hdr); - goto again; + return false; } /* @@ -2684,27 +2644,22 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) * XLogNeedsFlush() is not meaningful for them. * * We need to hold the content lock in at least share-exclusive mode - * to safely inspect the page LSN, so this couldn't have been done - * inside GetBufferFromRing(). + * to safely inspect the page LSN. */ - if (strategy && from_ring && - StrategyRejectBuffer(strategy, buf_hdr, buf_state)) + if (from_ring && StrategyRejectBuffer(strategy, buf_hdr, buf_state)) { - UnlockReleaseBuffer(buf); - goto again; + UnlockReleaseBuffer(bufnum); + return false; } /* OK, do the I/O */ FlushBuffer(buf_hdr, NULL, IOOBJECT_RELATION, io_context); - BufferLockUnlock(buf, buf_hdr); + BufferLockUnlock(bufnum, buf_hdr); ScheduleBufferTagForWriteback(&BackendWritebackContext, io_context, &buf_hdr->tag); } - /* Don't use a stale buf_state value after InvalidateVictimBuffer */ - buf_valid = 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 @@ -2713,8 +2668,79 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) if ((buf_state & BM_TAG_VALID) && !InvalidateVictimBuffer(buf_hdr)) { UnpinBuffer(buf_hdr); - goto again; + return false; + } + + return true; +} + +/* + * Determine the largest IO we can assemble given global constraints on the + * number of pinned buffers and max IO size. Currently only a single write is + * inflight at a time, so the batch can consume all the pinned buffers this + * backend is allowed. Only for batches of shared (non-local) relations. + */ +static uint32 +MaxWriteBuffers(void) +{ + uint32 result = Min(io_combine_limit, GetPinLimit()); + + /* Ensure forward progress */ + return Max(result, 1); +} + +static Buffer +GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) +{ + BufferDesc *buf_hdr; + Buffer buf; + uint64 buf_state; + bool from_ring; + bool buf_valid; + + /* + * Ensure, before we pin a victim buffer, that there's a free refcount + * entry and resource owner slot for the pin. + */ + ReservePrivateRefCountEntry(); + ResourceOwnerEnlarge(CurrentResourceOwner); + + /* we return here if a prospective victim buffer gets used concurrently */ +again: + + /* + * Select a victim buffer. The buffer is returned pinned and owned by this + * backend. If given a strategy object, see whether it can select a + * buffer; otherwise, or if the ring cannot provide one, get a buffer from + * shared buffers using the clock sweep and record it in the ring. + */ + buf_hdr = NULL; + from_ring = false; + if (strategy) + { + buf_hdr = GetBufferFromRing(strategy, &buf_state); + if (buf_hdr) + from_ring = true; + } + if (!buf_hdr) + { + buf_hdr = GetBufferFromClocksweep(&buf_state); + if (strategy) + AddBufferToRing(strategy, buf_hdr); } + buf = BufferDescriptorGetBuffer(buf_hdr); + + /* Don't use a stale buf_state value after InvalidateVictimBuffer */ + buf_valid = buf_state & BM_VALID; + + /* + * Try to claim the buffer -- flushing it first if dirty, and possibly + * letting the strategy reject it. If someone else takes or dirties the + * buffer before we can claim it, loop back and get another one. + */ + if (!ClaimVictimBuffer(strategy, buf_hdr, buf, buf_state, from_ring, + io_context)) + goto again; if (buf_valid) { -- 2.47.3