On Mon, Nov 3, 2025 at 3:06 PM Melanie Plageman <[email protected]> wrote: > > I found an incorrect assert in CleanVictimBuffer() that was tripping > in CI. Attached v6 is updated with it removed.
v7 rebased over recent changes in bufmgr.c - Melanie
From 59432c8962cd1d7866493492149963485f6e63e1 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Wed, 15 Oct 2025 10:53:48 -0400 Subject: [PATCH v7 1/7] Refactor goto into for loop in GetVictimBuffer() GetVictimBuffer() implemented a loop to optimistically lock a clean victim buffer using a goto. Future commits will add batch flushing functionality to GetVictimBuffer. The new logic works better with standard for loop flow control. This commit is only a refactor and does not introduce any new functionality. Author: Melanie Plageman <[email protected]> Reviewed-by: Nazir Bilal Yavuz <[email protected]> Reviewed-by: Chao Li <[email protected]> Discussion: https://postgr.es/m/2FA0BAC7-5413-4ABD-94CA-4398FE77750D%40gmail.com Discussion: https://postgr.es/m/flat/CAAKRu_Yjn4mvN9NBxtmsCQSGwup45CoA4e05nhR7ADP-v0WCig%40mail.gmail.com --- src/backend/storage/buffer/bufmgr.c | 189 ++++++++++++-------------- src/backend/storage/buffer/freelist.c | 19 ++- src/include/storage/buf_internals.h | 5 + 3 files changed, 110 insertions(+), 103 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 327ddb7adc8..90c24b8d93d 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -68,10 +68,6 @@ #include "utils/timestamp.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)] @@ -2331,125 +2327,116 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) 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. - */ - buf_hdr = StrategyGetBuffer(strategy, &buf_state, &from_ring); - buf = BufferDescriptorGetBuffer(buf_hdr); - - /* - * We shouldn't have any other pins for this buffer. - */ - CheckBufferIsPinnedOnce(buf); - - /* - * If the buffer was dirty, try to write it out. There is a race - * condition here, in that someone might dirty it after we released the - * buffer header lock above, or even while we are writing it out (since - * our share-lock won't prevent hint-bit updates). We will recheck the - * dirty bit after re-locking the buffer header. - */ - if (buf_state & BM_DIRTY) + /* Select a victim buffer using an optimistic locking scheme. */ + for (;;) { - LWLock *content_lock; - Assert(buf_state & BM_TAG_VALID); - Assert(buf_state & BM_VALID); + /* Attempt to claim a victim buffer. Buffer is returned pinned. */ + buf_hdr = StrategyGetBuffer(strategy, &buf_state, &from_ring); + buf = BufferDescriptorGetBuffer(buf_hdr); /* - * We need a share-lock on the buffer contents to write it out (else - * we might write invalid data, eg because someone else is 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 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.) + * We shouldn't have any other pins for this buffer. */ - content_lock = BufferDescriptorGetContentLock(buf_hdr); - if (!LWLockConditionalAcquire(content_lock, LW_SHARED)) - { - /* - * Someone else has locked the buffer, so give it up and loop back - * to get another one. - */ - UnpinBuffer(buf_hdr); - goto again; - } + CheckBufferIsPinnedOnce(buf); /* - * If using a nondefault strategy, and writing the buffer would - * require a WAL flush, let the strategy decide whether to go ahead - * and write/reuse the buffer or to choose another victim. We need a - * lock to inspect the page LSN, so this can't be done inside - * StrategyGetBuffer. + * If the buffer was dirty, try to write it out. There is a race + * condition here, in that someone might dirty it after we released + * the buffer header lock above, or even while we are writing it out + * (since our share-lock won't prevent hint-bit updates). We will + * recheck the dirty bit after re-locking the buffer header. */ - if (strategy != NULL) + if (buf_state & BM_DIRTY) { - XLogRecPtr lsn; + LWLock *content_lock; - /* Read the LSN while holding buffer header lock */ - buf_state = LockBufHdr(buf_hdr); - lsn = BufferGetLSN(buf_hdr); - UnlockBufHdr(buf_hdr); + Assert(buf_state & BM_TAG_VALID); + Assert(buf_state & BM_VALID); + + /* + * We need a share-lock on the buffer contents to write it out + * (else we might write invalid data, eg because someone else is + * 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 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.) + */ + content_lock = BufferDescriptorGetContentLock(buf_hdr); + if (!LWLockConditionalAcquire(content_lock, LW_SHARED)) + { + /* + * Someone else has locked the buffer, so give it up and loop + * back to get another one. + */ + UnpinBuffer(buf_hdr); + continue; + } - if (XLogNeedsFlush(lsn) - && StrategyRejectBuffer(strategy, buf_hdr, from_ring)) + /* + * If using a nondefault strategy, and writing the buffer would + * require a WAL flush, let the strategy decide whether to go + * ahead and write/reuse the buffer or to choose another victim. + * We need the content lock to inspect the page LSN, so this can't + * be done inside StrategyGetBuffer. + */ + if (StrategyRejectBuffer(strategy, buf_hdr, from_ring)) { LWLockRelease(content_lock); UnpinBuffer(buf_hdr); - goto again; + continue; } - } - /* OK, do the I/O */ - FlushBuffer(buf_hdr, NULL, IOOBJECT_RELATION, io_context); - LWLockRelease(content_lock); + /* OK, do the I/O */ + FlushBuffer(buf_hdr, NULL, IOOBJECT_RELATION, io_context); + LWLockRelease(content_lock); - ScheduleBufferTagForWriteback(&BackendWritebackContext, io_context, - &buf_hdr->tag); - } + ScheduleBufferTagForWriteback(&BackendWritebackContext, io_context, + &buf_hdr->tag); + } - if (buf_state & BM_VALID) - { + if (buf_state & BM_VALID) + { + /* + * When a BufferAccessStrategy is in use, blocks evicted from + * shared buffers are counted as IOOP_EVICT in the corresponding + * context (e.g. IOCONTEXT_BULKWRITE). Shared buffers are evicted + * by a strategy in two cases: 1) while initially claiming buffers + * for the strategy ring 2) to replace an existing strategy ring + * buffer because it is pinned or in use and cannot be reused. + * + * Blocks evicted from buffers already in the strategy ring are + * 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. + */ + pgstat_count_io_op(IOOBJECT_RELATION, io_context, + from_ring ? IOOP_REUSE : IOOP_EVICT, 1, 0); + } + /* - * When a BufferAccessStrategy is in use, blocks evicted from shared - * buffers are counted as IOOP_EVICT in the corresponding context - * (e.g. IOCONTEXT_BULKWRITE). Shared buffers are evicted by a - * strategy in two cases: 1) while initially claiming buffers for the - * strategy ring 2) to replace an existing strategy ring buffer - * because it is pinned or in use and cannot be reused. - * - * Blocks evicted from buffers already in the strategy ring are - * 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. + * 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. */ - pgstat_count_io_op(IOOBJECT_RELATION, io_context, - from_ring ? IOOP_REUSE : IOOP_EVICT, 1, 0); - } + if ((buf_state & BM_TAG_VALID) && !InvalidateVictimBuffer(buf_hdr)) + { + UnpinBuffer(buf_hdr); + continue; + } - /* - * 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; + break; } /* a final set of sanity checks */ diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index 28d952b3534..1465984b141 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" @@ -780,12 +781,20 @@ IOContextForStrategy(BufferAccessStrategy strategy) * be written out and doing so would require flushing WAL too. This gives us * a chance to choose a different victim. * + * The buffer must be pinned and content locked and the buffer header spinlock + * must not be held. We must hold the content lock to examine the LSN. + * * 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) { + XLogRecPtr lsn; + + if (!strategy) + return false; + /* We only do this in bulkread mode */ if (strategy->btype != BAS_BULKREAD) return false; @@ -795,11 +804,17 @@ StrategyRejectBuffer(BufferAccessStrategy strategy, BufferDesc *buf, bool from_r strategy->buffers[strategy->current] != BufferDescriptorGetBuffer(buf)) return false; + LockBufHdr(buf); + lsn = BufferGetLSN(buf); + UnlockBufHdr(buf); + + if (XLogNeedsFlush(lsn)) + return false; + /* - * Remove the dirty buffer from the ring; necessary to prevent infinite + * Remove the dirty buffer from the ring; necessary to prevent an infinite * loop if all ring members are dirty. */ strategy->buffers[strategy->current] = InvalidBuffer; - return true; } diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 5400c56a965..04fdea64f83 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -486,6 +486,11 @@ ResourceOwnerForgetBufferIO(ResourceOwner owner, Buffer buffer) /* * Internal buffer management routines */ + +/* 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))) + /* bufmgr.c */ extern void WritebackContextInit(WritebackContext *context, int *max_pending); extern void IssuePendingWritebacks(WritebackContext *wb_context, IOContext io_context); -- 2.43.0
From 8dff2f6ba609909f2820acde080228b799f3fb02 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Wed, 15 Oct 2025 10:54:19 -0400 Subject: [PATCH v7 2/7] Split FlushBuffer() into two parts Before adding write combining to write a batch of blocks when flushing dirty buffers, refactor FlushBuffer() into the preparatory step and actual buffer flushing step. This separation procides symmetry with future code for batch flushing which necessarily separates these steps, as it must prepare multiple buffers before flushing them together. These steps are moved into a new FlushBuffer() helper function, CleanVictimBuffer() which will contain both the batch flushing and single flush code in future commits. Author: Melanie Plageman <[email protected]> Reviewed-by: Chao Li <[email protected]> Reviewed-by: Nazir Bilal Yavuz <[email protected]> Discussion: https://postgr.es/m/2FA0BAC7-5413-4ABD-94CA-4398FE77750D%40gmail.com Discussion: https://postgr.es/m/flat/CAAKRu_Yjn4mvN9NBxtmsCQSGwup45CoA4e05nhR7ADP-v0WCig%40mail.gmail.com --- src/backend/storage/buffer/bufmgr.c | 142 +++++++++++++++++++--------- 1 file changed, 98 insertions(+), 44 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 90c24b8d93d..235e261738b 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -533,6 +533,12 @@ static void FlushUnlockedBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, IOContext io_context); static void FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, IOContext io_context); +static bool PrepareFlushBuffer(BufferDesc *bufdesc, XLogRecPtr *lsn); +static void DoFlushBuffer(BufferDesc *buf, SMgrRelation reln, + IOObject io_object, IOContext io_context, + XLogRecPtr buffer_lsn); +static void CleanVictimBuffer(BufferDesc *bufdesc, bool from_ring, + IOContext io_context); static void FindAndDropRelationBuffers(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber nForkBlock, @@ -2394,12 +2400,8 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) continue; } - /* OK, do the I/O */ - FlushBuffer(buf_hdr, NULL, IOOBJECT_RELATION, io_context); - LWLockRelease(content_lock); - - ScheduleBufferTagForWriteback(&BackendWritebackContext, io_context, - &buf_hdr->tag); + /* Content lock is released inside CleanVictimBuffer */ + CleanVictimBuffer(buf_hdr, from_ring, io_context); } @@ -4270,54 +4272,64 @@ static void FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, IOContext io_context) { - XLogRecPtr recptr; - ErrorContextCallback errcallback; - instr_time io_start; - Block bufBlock; - char *bufToWrite; - uint32 buf_state; + XLogRecPtr lsn; - /* - * Try to start an I/O operation. If StartBufferIO returns false, then - * someone else flushed the buffer before we could, so we need not do - * anything. - */ - if (!StartBufferIO(buf, false, false)) - return; + if (PrepareFlushBuffer(buf, &lsn)) + DoFlushBuffer(buf, reln, io_object, io_context, lsn); +} - /* Setup error traceback support for ereport() */ - errcallback.callback = shared_buffer_write_error_callback; - errcallback.arg = buf; - errcallback.previous = error_context_stack; - error_context_stack = &errcallback; +/* + * Prepare and write out a dirty victim buffer. + * + * Buffer must be pinned, the content lock must be held exclusively, and the + * buffer header spinlock must not be held. The exclusive lock is released and + * the buffer is returned pinned but not locked. + * + * bufdesc may be modified. + */ +static void +CleanVictimBuffer(BufferDesc *bufdesc, + bool from_ring, IOContext io_context) +{ + XLogRecPtr max_lsn = InvalidXLogRecPtr; + LWLock *content_lock; - /* Find smgr relation for buffer */ - if (reln == NULL) - reln = smgropen(BufTagGetRelFileLocator(&buf->tag), INVALID_PROC_NUMBER); + /* Set up this victim buffer to be flushed */ + if (!PrepareFlushBuffer(bufdesc, &max_lsn)) + return; - TRACE_POSTGRESQL_BUFFER_FLUSH_START(BufTagGetForkNum(&buf->tag), - buf->tag.blockNum, - reln->smgr_rlocator.locator.spcOid, - reln->smgr_rlocator.locator.dbOid, - reln->smgr_rlocator.locator.relNumber); + DoFlushBuffer(bufdesc, NULL, IOOBJECT_RELATION, io_context, max_lsn); + content_lock = BufferDescriptorGetContentLock(bufdesc); + LWLockRelease(content_lock); + ScheduleBufferTagForWriteback(&BackendWritebackContext, io_context, + &bufdesc->tag); +} - buf_state = LockBufHdr(buf); +/* + * Prepare the buffer with bufdesc for writing. Returns true if the buffer + * acutally needs writing and false otherwise. lsn returns the buffer's LSN if + * the table is logged. + */ +static bool +PrepareFlushBuffer(BufferDesc *bufdesc, XLogRecPtr *lsn) +{ + uint32 buf_state; /* - * Run PageGetLSN while holding header lock, since we don't have the - * buffer locked exclusively in all cases. + * Try to start an I/O operation. If StartBufferIO returns false, then + * someone else flushed the buffer before we could, so we need not do + * anything. */ - recptr = BufferGetLSN(buf); + if (!StartBufferIO(bufdesc, false, false)) + return false; - /* To check if block content changes while flushing. - vadim 01/17/97 */ - UnlockBufHdrExt(buf, buf_state, - 0, BM_JUST_DIRTIED, - 0); + *lsn = InvalidXLogRecPtr; + buf_state = LockBufHdr(bufdesc); /* - * Force XLOG flush up to buffer's LSN. This implements the basic WAL - * rule that log updates must hit disk before any of the data-file changes - * they describe do. + * Record the buffer's LSN. We will force XLOG flush up to buffer's LSN. + * This implements the basic WAL rule that log updates must hit disk + * before any of the data-file changes they describe do. * * However, this rule does not apply to unlogged relations, which will be * lost after a crash anyway. Most unlogged relation pages do not bear @@ -4330,9 +4342,51 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, * happen, attempting to flush WAL through that location would fail, with * disastrous system-wide consequences. To make sure that can't happen, * skip the flush if the buffer isn't permanent. + * + * We must hold the buffer header lock when examining the page LSN since + * don't have buffer exclusively locked in all cases. */ if (buf_state & BM_PERMANENT) - XLogFlush(recptr); + *lsn = BufferGetLSN(bufdesc); + + /* To check if block content changes while flushing. - vadim 01/17/97 */ + UnlockBufHdrExt(bufdesc, buf_state, + 0, BM_JUST_DIRTIED, + 0); + return true; +} + +/* + * Actually do the write I/O to clean a buffer. buf and reln may be modified. + */ +static void +DoFlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, + IOContext io_context, XLogRecPtr buffer_lsn) +{ + ErrorContextCallback errcallback; + instr_time io_start; + Block bufBlock; + char *bufToWrite; + + /* Setup error traceback support for ereport() */ + errcallback.callback = shared_buffer_write_error_callback; + errcallback.arg = buf; + errcallback.previous = error_context_stack; + error_context_stack = &errcallback; + + /* Find smgr relation for buffer */ + if (reln == NULL) + reln = smgropen(BufTagGetRelFileLocator(&buf->tag), INVALID_PROC_NUMBER); + + TRACE_POSTGRESQL_BUFFER_FLUSH_START(BufTagGetForkNum(&buf->tag), + buf->tag.blockNum, + reln->smgr_rlocator.locator.spcOid, + reln->smgr_rlocator.locator.dbOid, + reln->smgr_rlocator.locator.relNumber); + + /* Force XLOG flush up to buffer's LSN */ + if (XLogRecPtrIsValid(buffer_lsn)) + XLogFlush(buffer_lsn); /* * Now it's safe to write the buffer to disk. Note that no one else should -- 2.43.0
From d4d815c9ef5a270d8e9c7dde59e2121d3c1586a7 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Wed, 15 Oct 2025 13:15:43 -0400 Subject: [PATCH v7 3/7] Eagerly flush bulkwrite strategy ring Operations using BAS_BULKWRITE (COPY FROM and createdb) will inevitably need to flush buffers in the strategy ring in order to reuse them. By eagerly flushing the buffers in a larger run, we encourage larger writes at the kernel level and less interleaving of WAL flushes and data file writes. The effect is mainly noticeable with multiple parallel COPY FROMs. In this case, client backends achieve higher write throughput and end up spending less time waiting on acquiring the lock to flush WAL. Larger flush operations also mean less time waiting for flush operations at the kernel level. The heuristic for eager eviction is to only flush buffers in the strategy ring which do not require a WAL flush. This patch also is a step toward AIO writes, as it lines up multiple buffers that can be issued asynchronously once the infrastructure exists. Reviewed-by: Chao Li <[email protected]> Reviewed-by: Nazir Bilal Yavuz <[email protected]> Earlier version Reviewed-by: Kirill Reshke <[email protected]> Discussion: https://postgr.es/m/2FA0BAC7-5413-4ABD-94CA-4398FE77750D%40gmail.com Discussion: https://postgr.es/m/flat/CAAKRu_Yjn4mvN9NBxtmsCQSGwup45CoA4e05nhR7ADP-v0WCig%40mail.gmail.com --- src/backend/storage/buffer/bufmgr.c | 237 +++++++++++++++++++++++++- src/backend/storage/buffer/freelist.c | 48 ++++++ src/include/storage/buf_internals.h | 4 + 3 files changed, 281 insertions(+), 8 deletions(-) diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 235e261738b..57a3eae865e 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -531,14 +531,25 @@ static void CheckReadBuffersOperation(ReadBuffersOperation *operation, bool is_c static Buffer GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context); static void FlushUnlockedBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, IOContext io_context); + static void FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, IOContext io_context); -static bool PrepareFlushBuffer(BufferDesc *bufdesc, XLogRecPtr *lsn); +static BufferDesc *NextStrategyBufToFlush(BufferAccessStrategy strategy, + Buffer sweep_end, + XLogRecPtr *lsn, int *sweep_cursor); + +static bool BufferNeedsWALFlush(BufferDesc *bufdesc, XLogRecPtr *lsn); +static BufferDesc *PrepareOrRejectEagerFlushBuffer(Buffer bufnum, BlockNumber require, + RelFileLocator *rlocator, bool skip_pinned, + XLogRecPtr *max_lsn); +static bool PrepareFlushBuffer(BufferDesc *bufdesc, + XLogRecPtr *lsn); static void DoFlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, IOContext io_context, XLogRecPtr buffer_lsn); -static void CleanVictimBuffer(BufferDesc *bufdesc, bool from_ring, - IOContext io_context); +static void CleanVictimBuffer(BufferAccessStrategy strategy, + BufferDesc *bufdesc, + bool from_ring, IOContext io_context); static void FindAndDropRelationBuffers(RelFileLocator rlocator, ForkNumber forkNum, BlockNumber nForkBlock, @@ -2401,7 +2412,7 @@ GetVictimBuffer(BufferAccessStrategy strategy, IOContext io_context) } /* Content lock is released inside CleanVictimBuffer */ - CleanVictimBuffer(buf_hdr, from_ring, io_context); + CleanVictimBuffer(strategy, buf_hdr, from_ring, io_context); } @@ -4278,6 +4289,61 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, DoFlushBuffer(buf, reln, io_object, io_context, lsn); } +/* + * Returns true if the buffer needs WAL flushed before it can be written out. + * Caller must not already hold the buffer header spinlock. + */ +static bool +BufferNeedsWALFlush(BufferDesc *bufdesc, XLogRecPtr *lsn) +{ + uint32 buf_state = LockBufHdr(bufdesc); + + *lsn = BufferGetLSN(bufdesc); + + UnlockBufHdr(bufdesc); + + /* + * See buffer flushing code for more details on why we condition this on + * the relation being logged. + */ + return buf_state & BM_PERMANENT && XLogNeedsFlush(*lsn); +} + + +/* + * Returns the buffer descriptor of the buffer containing the next block we + * should eagerly flush or NULL when there are no further buffers to consider + * writing out. + */ +static BufferDesc * +NextStrategyBufToFlush(BufferAccessStrategy strategy, + Buffer sweep_end, + XLogRecPtr *lsn, int *sweep_cursor) +{ + Buffer bufnum; + BufferDesc *bufdesc; + + while ((bufnum = + StrategyNextBuffer(strategy, sweep_cursor)) != sweep_end) + { + /* + * For BAS_BULKWRITE, once you hit an InvalidBuffer, the remaining + * buffers in the ring will be invalid. + */ + if (!BufferIsValid(bufnum)) + break; + + if ((bufdesc = PrepareOrRejectEagerFlushBuffer(bufnum, + InvalidBlockNumber, + NULL, + true, + lsn)) != NULL) + return bufdesc; + } + + return NULL; +} + /* * Prepare and write out a dirty victim buffer. * @@ -4288,21 +4354,176 @@ FlushBuffer(BufferDesc *buf, SMgrRelation reln, IOObject io_object, * bufdesc may be modified. */ static void -CleanVictimBuffer(BufferDesc *bufdesc, +CleanVictimBuffer(BufferAccessStrategy strategy, + BufferDesc *bufdesc, bool from_ring, IOContext io_context) { XLogRecPtr max_lsn = InvalidXLogRecPtr; LWLock *content_lock; + bool first_buffer = true; /* Set up this victim buffer to be flushed */ if (!PrepareFlushBuffer(bufdesc, &max_lsn)) return; - DoFlushBuffer(bufdesc, NULL, IOOBJECT_RELATION, io_context, max_lsn); + if (from_ring && StrategySupportsEagerFlush(strategy)) + { + Buffer sweep_end = BufferDescriptorGetBuffer(bufdesc); + int cursor = StrategyGetCurrentIndex(strategy); + + /* Clean victim buffer and find more to flush opportunistically */ + do + { + DoFlushBuffer(bufdesc, NULL, IOOBJECT_RELATION, io_context, max_lsn); + content_lock = BufferDescriptorGetContentLock(bufdesc); + LWLockRelease(content_lock); + ScheduleBufferTagForWriteback(&BackendWritebackContext, io_context, + &bufdesc->tag); + /* We leave the first buffer pinned for the caller */ + if (!first_buffer) + UnpinBuffer(bufdesc); + first_buffer = false; + } while ((bufdesc = NextStrategyBufToFlush(strategy, sweep_end, + &max_lsn, &cursor)) != NULL); + } + else + { + DoFlushBuffer(bufdesc, NULL, IOOBJECT_RELATION, io_context, max_lsn); + content_lock = BufferDescriptorGetContentLock(bufdesc); + LWLockRelease(content_lock); + ScheduleBufferTagForWriteback(&BackendWritebackContext, io_context, + &bufdesc->tag); + } +} + +/* + * Prepare bufdesc for eager flushing. + * + * Given bufnum, return the buffer descriptor of the buffer to eagerly flush, + * pinned and locked, or NULL if this buffer does not contain a block that + * should be flushed. + * + * require is the BlockNumber required by the caller. Some callers may require + * a specific BlockNumber to be in bufnum because they are assembling a + * contiguous run of blocks. + * + * If the caller needs the block to be from a specific relation, rlocator will + * be provided. + */ +static BufferDesc * +PrepareOrRejectEagerFlushBuffer(Buffer bufnum, BlockNumber require, + RelFileLocator *rlocator, bool skip_pinned, + XLogRecPtr *max_lsn) +{ + BufferDesc *bufdesc; + uint32 old_buf_state; + uint32 buf_state; + XLogRecPtr lsn; + BlockNumber blknum; + LWLock *content_lock; + + if (!BufferIsValid(bufnum)) + return NULL; + + Assert(!BufferIsLocal(bufnum)); + + bufdesc = GetBufferDescriptor(bufnum - 1); + + /* Block may need to be in a specific relation */ + if (rlocator && + !RelFileLocatorEquals(BufTagGetRelFileLocator(&bufdesc->tag), + *rlocator)) + return NULL; + + /* + * Ensure that theres a free refcount entry and resource owner slot for + * the pin before pinning the buffer. While this may leake a refcount and + * slot if we return without a buffer, we should use that slot the next + * time we try and reserve a spot. + */ + ResourceOwnerEnlarge(CurrentResourceOwner); + ReservePrivateRefCountEntry(); + + /* + * Check whether the buffer can be used and pin it if so. Do this using a + * CAS loop, to avoid having to lock the buffer header. We have to lock + * the buffer header later if we succeed in pinning the buffer here, but + * avoiding locking the buffer header if the buffer is in use is worth it. + */ + old_buf_state = pg_atomic_read_u32(&bufdesc->state); + + for (;;) + { + buf_state = old_buf_state; + + if (!(buf_state & BM_DIRTY) || !(buf_state & BM_VALID)) + return NULL; + + /* We don't eagerly flush buffers used by others */ + if (skip_pinned && + (BUF_STATE_GET_REFCOUNT(buf_state) > 0 || + BUF_STATE_GET_USAGECOUNT(buf_state) > 1)) + return NULL; + + if (unlikely(buf_state & BM_LOCKED)) + { + old_buf_state = WaitBufHdrUnlocked(bufdesc); + continue; + } + + /* pin the buffer if the CAS succeeds */ + buf_state += BUF_REFCOUNT_ONE; + + if (pg_atomic_compare_exchange_u32(&bufdesc->state, &old_buf_state, + buf_state)) + { + TrackNewBufferPin(BufferDescriptorGetBuffer(bufdesc)); + break; + } + } + + CheckBufferIsPinnedOnce(bufnum); + + blknum = BufferGetBlockNumber(bufnum); + Assert(BlockNumberIsValid(blknum)); + + /* We only include contiguous blocks in the run */ + if (BlockNumberIsValid(require) && blknum != require) + goto except_unpin_buffer; + + /* Don't eagerly flush buffers requiring WAL flush */ + if (BufferNeedsWALFlush(bufdesc, &lsn)) + goto except_unpin_buffer; + content_lock = BufferDescriptorGetContentLock(bufdesc); + if (!LWLockConditionalAcquire(content_lock, LW_SHARED)) + goto except_unpin_buffer; + + /* + * Now that we have the content lock, we need to recheck if we need to + * flush WAL. + */ + if (BufferNeedsWALFlush(bufdesc, &lsn)) + goto except_unpin_buffer; + + /* Try to start an I/O operation */ + if (!StartBufferIO(bufdesc, false, true)) + goto except_unlock_content; + + if (lsn > *max_lsn) + *max_lsn = lsn; + + buf_state = LockBufHdr(bufdesc); + UnlockBufHdrExt(bufdesc, buf_state, 0, BM_JUST_DIRTIED, 0); + + return bufdesc; + +except_unlock_content: LWLockRelease(content_lock); - ScheduleBufferTagForWriteback(&BackendWritebackContext, io_context, - &bufdesc->tag); + +except_unpin_buffer: + UnpinBuffer(bufdesc); + return NULL; } /* diff --git a/src/backend/storage/buffer/freelist.c b/src/backend/storage/buffer/freelist.c index 1465984b141..10301f4aab2 100644 --- a/src/backend/storage/buffer/freelist.c +++ b/src/backend/storage/buffer/freelist.c @@ -156,6 +156,31 @@ ClockSweepTick(void) return victim; } +/* + * Some BufferAccessStrategies support eager flushing -- which is flushing + * buffers in the ring before they are needed. This can lead to better I/O + * patterns than lazily flushing buffers immediately before reusing them. + */ +bool +StrategySupportsEagerFlush(BufferAccessStrategy strategy) +{ + Assert(strategy); + + switch (strategy->btype) + { + case BAS_BULKWRITE: + return true; + case BAS_VACUUM: + case BAS_NORMAL: + case BAS_BULKREAD: + return false; + default: + elog(ERROR, "unrecognized buffer access strategy: %d", + (int) strategy->btype); + return false; + } +} + /* * StrategyGetBuffer * @@ -307,6 +332,29 @@ StrategyGetBuffer(BufferAccessStrategy strategy, uint32 *buf_state, bool *from_r } } +/* + * Given a position in the ring, cursor, increment the position, and return + * the buffer at this position. + */ +Buffer +StrategyNextBuffer(BufferAccessStrategy strategy, int *cursor) +{ + if (++(*cursor) >= strategy->nbuffers) + *cursor = 0; + + return strategy->buffers[*cursor]; +} + +/* + * Return the current slot in the strategy ring. + */ +int +StrategyGetCurrentIndex(BufferAccessStrategy strategy) +{ + return strategy->current; +} + + /* * StrategySyncStart -- tell BgBufferSync where to start syncing * diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h index 04fdea64f83..c07e309a288 100644 --- a/src/include/storage/buf_internals.h +++ b/src/include/storage/buf_internals.h @@ -506,6 +506,10 @@ extern void TerminateBufferIO(BufferDesc *buf, bool clear_dirty, uint32 set_flag /* freelist.c */ +extern bool StrategySupportsEagerFlush(BufferAccessStrategy strategy); +extern Buffer StrategyNextBuffer(BufferAccessStrategy strategy, + int *cursor); +extern int StrategyGetCurrentIndex(BufferAccessStrategy strategy); extern IOContext IOContextForStrategy(BufferAccessStrategy strategy); extern BufferDesc *StrategyGetBuffer(BufferAccessStrategy strategy, uint32 *buf_state, bool *from_ring); -- 2.43.0
