From cd2ee3c048a0a07c7590f042a2a8369eece45c0c Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Tue, 2 Jun 2026 18:38:11 -0400 Subject: [PATCH v16 09/21] Make StrategyRejectBuffer encapsulate more logic Callers of StrategyRejectBuffer() checked whether reusing the buffer would require a WAL flush before calling it. Move that check into StrategyRejectBuffer() itself. This also allows us to do the check less often, since it now runs after the other, cheaper pre-conditions. This is a preparatory commit for further refactoring of GetVictimBuffer(). Because we moved BufHdrGetBlock() into buf_internals.h, protect it more directly from being called on local buffers with an assert. 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 | 9 +-------- src/backend/storage/buffer/freelist.c | 26 ++++++++++++++++++++------ src/include/storage/buf_internals.h | 9 ++++++++- 3 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 1a8cf4ddc30..d32df08ec1a 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -71,11 +71,6 @@ #include "utils/timestamp.h" #include "utils/wait_event.h" - -/* Note: these two macros only work on shared buffers, not local ones! */ -#define BufHdrGetBlock(bufHdr) ((Block) (BufferBlocks + ((Size) (bufHdr)->buf_id) * BLCKSZ)) -#define BufferGetLSN(bufHdr) (PageGetLSN(BufHdrGetBlock(bufHdr))) - /* Note: this macro only works on local buffers, not shared ones! */ #define LocalBufHdrGetBlock(bufHdr) \ LocalBufferBlockPointers[-((bufHdr)->buf_id + 2)] @@ -2678,9 +2673,7 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) * inside StrategyGetBuffer(). */ if (strategy && from_ring && - buf_state & BM_PERMANENT && - XLogNeedsFlush(BufferGetLSN(buf_hdr)) && - StrategyRejectBuffer(strategy, buf_hdr, from_ring)) + StrategyRejectBuffer(strategy, buf_hdr, buf_state)) { UnlockReleaseBuffer(buf); goto again; diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index fdb5bad7910..746740105a9 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -15,6 +15,7 @@ */ #include "postgres.h" +#include "access/xlog.h" #include "pgstat.h" #include "port/atomics.h" #include "storage/buf_internals.h" @@ -741,23 +742,36 @@ IOContextForStrategy(BufferAccessStrategy strategy) * StrategyRejectBuffer -- consider rejecting a dirty buffer * * When a nondefault strategy is used, the buffer manager calls this function - * when it turns out that the buffer selected by StrategyGetBuffer needs to - * be written out and doing so would require flushing WAL too. This gives us - * a chance to choose a different victim. + * when it turns out that the victim buffer selected needs to be written out + * and doing so would require flushing WAL too. This gives us a chance to + * choose a different victim. * * Returns true if buffer manager should ask for a new victim, and false * if this buffer should be written and re-used. */ bool -StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf, bool from_ring) +StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf, uint64 buf_state) { + Assert(strategy); + /* We only do this in bulkread mode */ if (strategy->btype != BAS_BULKREAD) return false; /* Don't muck with behavior of normal buffer-replacement strategy */ - if (!from_ring || - strategy->buffers[strategy->current] != BufferDescriptorGetBuffer(buf)) + if (strategy->buffers[strategy->current] != BufferDescriptorGetBuffer(buf)) + return false; + + /* + * Rejecting a dirty ring buffer is only useful when reusing it would + * require flushing WAL. This only applies to permanent buffers; unlogged + * buffers can have fake LSNs, so XLogNeedsFlush() is not meaningful for + * them. + */ + if (!(buf_state & BM_PERMANENT)) + return false; + + if (!XLogNeedsFlush(BufferGetLSN(buf))) return false; /* diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 5297b4ea2a2..b52af7101f2 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -146,6 +146,13 @@ StaticAssertDecl(MAX_BACKENDS_BITS <= (BUF_LOCK_BITS - 2), StaticAssertDecl(BM_MAX_USAGE_COUNT < (UINT64CONST(1) << BUF_USAGECOUNT_BITS), "BM_MAX_USAGE_COUNT doesn't fit in BUF_USAGECOUNT_BITS bits"); + +/* Note: these two macros only work on shared buffers, not local ones! */ +#define BufHdrGetBlock(bufHdr) \ + (AssertMacro((bufHdr)->buf_id >= 0), \ + (Block) (BufferBlocks + ((Size) (bufHdr)->buf_id) * BLCKSZ)) +#define BufferGetLSN(bufHdr) (PageGetLSN(BufHdrGetBlock(bufHdr))) + /* * Buffer tag identifies which disk block the buffer contains. * @@ -590,7 +597,7 @@ extern IOContext IOContextForStrategy(BufferAccessStrategy strategy); extern BufferDesc *StrategyGetBuffer(BufferAccessStrategy strategy, uint64 *buf_state, bool *from_ring); extern bool StrategyRejectBuffer(BufferAccessStrategy strategy, - BufferDesc *buf, bool from_ring); + BufferDesc *buf, uint64 buf_state); extern int StrategySyncStart(uint32 *complete_passes, uint32 *num_buf_alloc); extern void StrategyNotifyBgWriter(int bgwprocno); -- 2.47.3