From 10f0a00df8ddfa0678f2366f259c3bfd04fa2701 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Mon, 17 Aug 2026 16:08:28 -0400 Subject: [PATCH v16 10/21] Split StrategyGetBuffer() into source-specific helpers StrategyGetBuffer() (called by GetVictimBuffer()), contrary to its name, did not always return a strategy buffer. It might have returned a buffer from the strategy ring, a buffer from shared buffers, or a buffer from shared buffers that was now in the strategy ring. Split it into dedicated helpers: GetBufferFromRing() returns a reusable buffer from the strategy ring and GetBufferFromClocksweep() returns a buffer from shared buffers. GetVictimBuffer() now calls them directly, making it explicit where each kind of buffer comes from and where a buffer taken from shared buffers is recorded in the ring. This is purely structural; no behavior change. Author: Melanie Plageman Earler version Reviewed-by: Andrey Borodin Discussion: https://postgr.es/m/D0E7C430-76A9-4030-B8AB-109E7D7ED8C1%40yandex-team.ru --- src/backend/storage/buffer/bufmgr.c | 29 ++++++++++---- src/backend/storage/buffer/freelist.c | 54 ++++++--------------------- src/include/storage/buf_internals.h | 6 ++- 3 files changed, 37 insertions(+), 52 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index d32df08ec1a..be2dd9bf7b5 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -2614,10 +2614,25 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) again: /* - * Select a victim buffer. The buffer is returned pinned and owned by - * this backend. + * 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 = StrategyGetBuffer(strategy, &buf_state, &from_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); /* @@ -2628,7 +2643,7 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) /* * If the buffer was dirty, try to write it out. There is a race * condition here, another backend could dirty the buffer between - * StrategyGetBuffer() checking that it is not in use and invalidating the + * GetBufferFrom*() checking that it is not in use and invalidating the * buffer below. That's addressed by InvalidateVictimBuffer() verifying * that the buffer is not dirty. */ @@ -2643,13 +2658,13 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) * compacting the page contents while we write). We must use a * conditional lock acquisition here to avoid deadlock. Even though * the buffer was not pinned (and therefore surely not locked) when - * StrategyGetBuffer returned it, someone else could have pinned and + * GetBufferFrom*() returned it, someone else could have pinned and * (share-)exclusive-locked it by the time we get here. If we try to * get the lock unconditionally, we'd block waiting for them; if they * later block waiting for us, deadlock ensues. (This has been * observed to happen when two backends are both trying to split btree * index pages, and the second one just happens to be trying to split - * the page the first one got from StrategyGetBuffer.) + * the page the first one got from GetBufferFrom*().) */ if (!BufferLockConditional(buf, buf_hdr, BUFFER_LOCK_SHARE_EXCLUSIVE)) { @@ -2670,7 +2685,7 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) * * 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 StrategyGetBuffer(). + * inside GetBufferFromRing(). */ if (strategy && from_ring && StrategyRejectBuffer(strategy, buf_hdr, buf_state)) diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index 746740105a9..7a8b20c6929 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -96,13 +96,9 @@ typedef struct BufferAccessStrategyData /* Prototypes for internal functions */ -static BufferDesc *GetBufferFromRing(BufferAccessStrategy strategy, - uint64 *buf_state); -static void AddBufferToRing(BufferAccessStrategy strategy, - BufferDesc *buf); /* - * ClockSweepTick - Helper routine for StrategyGetBuffer() + * ClockSweepTick - Helper routine for GetBufferFromClocksweep() * * Move the clock hand one buffer ahead of its current position and return the * id of the buffer now under the hand. @@ -167,43 +163,17 @@ ClockSweepTick(void) } /* - * StrategyGetBuffer - * - * Called by the bufmgr to get the next candidate buffer to use in - * GetVictimBuffer(). The only hard requirement GetVictimBuffer() has is that - * the selected buffer must not currently be pinned by anyone. - * - * strategy is a BufferAccessStrategy object, or NULL for default strategy. - * - * It is the callers responsibility to ensure the buffer ownership can be - * tracked via TrackNewBufferPin(). - * - * The buffer is pinned and marked as owned, using TrackNewBufferPin(), - * before returning. + * Return a buffer from clock sweep, pinned and marked as owned using + * TrackNewBufferPin(). It is the caller's responsibility to make sure the + * buffer ownership can be tracked. */ BufferDesc * -StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_ring) +GetBufferFromClocksweep(uint64 *buf_state) { BufferDesc *buf; int bgwprocno; int trycounter; - *from_ring = false; - - /* - * If given a strategy object, see whether it can select a buffer. We - * assume strategy objects don't need buffer_strategy_lock. - */ - if (strategy != NULL) - { - buf = GetBufferFromRing(strategy, buf_state); - if (buf != NULL) - { - *from_ring = true; - return buf; - } - } - /* * If asked, we need to waken the bgwriter. Since we don't want to rely on * a spinlock for this we force a read from shared memory once, and then @@ -304,8 +274,6 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_r local_buf_state)) { /* Found a usable buffer */ - if (strategy != NULL) - AddBufferToRing(strategy, buf); *buf_state = local_buf_state; TrackNewBufferPin(BufferDescriptorGetBuffer(buf)); @@ -360,8 +328,8 @@ StrategySyncStart(uint32 *complete_passes, uint32 *num_buf_alloc) /* * StrategyNotifyBgWriter -- set or clear allocation notification latch * - * If bgwprocno isn't -1, the next invocation of StrategyGetBuffer will - * set that latch. Pass -1 to clear the pending notification before it + * If bgwprocno isn't -1, the next invocation of GetBufferFromClocksweep() + * will set that latch. Pass -1 to clear the pending notification before it * happens. This feature is used by the bgwriter process to wake itself up * from hibernation, and is not meant for anybody else to use. */ @@ -370,8 +338,8 @@ StrategyNotifyBgWriter(int bgwprocno) { /* * We acquire buffer_strategy_lock just to ensure that the store appears - * atomic to StrategyGetBuffer. The bgwriter should call this rather - * infrequently, so there's no performance penalty from being safe. + * atomic to GetBufferFromClocksweep. The bgwriter should call this + * rather infrequently, so there's no performance penalty from being safe. */ SpinLockAcquire(&StrategyControl->buffer_strategy_lock); StrategyControl->bgwprocno = bgwprocno; @@ -620,7 +588,7 @@ FreeAccessStrategy(BufferAccessStrategy strategy) * The buffer is pinned and marked as owned, using TrackNewBufferPin(), before * returning. */ -static BufferDesc * +BufferDesc * GetBufferFromRing(BufferAccessStrategy strategy, uint64 *buf_state) { BufferDesc *buf; @@ -699,7 +667,7 @@ GetBufferFromRing(BufferAccessStrategy strategy, uint64 *buf_state) * Caller must hold the buffer header spinlock on the buffer. Since this * is called with the spinlock held, it had better be quite cheap. */ -static void +void AddBufferToRing(BufferAccessStrategy strategy, BufferDesc *buf) { strategy->buffers[strategy->current] = BufferDescriptorGetBuffer(buf); diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index b52af7101f2..c4c155b7e0c 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -594,8 +594,10 @@ extern void TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint64 set_flag /* freelist.c */ extern IOContext IOContextForStrategy(BufferAccessStrategy strategy); -extern BufferDesc *StrategyGetBuffer(BufferAccessStrategy strategy, - uint64 *buf_state, bool *from_ring); +extern BufferDesc *GetBufferFromRing(BufferAccessStrategy strategy, + uint64 *buf_state); +extern void AddBufferToRing(BufferAccessStrategy strategy, BufferDesc *buf); +extern BufferDesc *GetBufferFromClocksweep(uint64 *buf_state); extern bool StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf, uint64 buf_state); -- 2.47.3