On Wed, Jul 22, 2026 at 12:53 PM Jingtang Zhang <[email protected]> wrote: > > 4830f1024325 made VACUUM's failsafe abandon its buffer access strategy > and use normal shared-buffer replacement. Commit 9256822608f3 later > converted VACUUM's first phase to use streaming read I/O. > > Was it an oversight in the read stream conversion that the stream is > initialized with VACUUM's strategy, but is not updated when failsafe > clears vacrel->bstrategy? As a result, the active stream continues to > use the VACUUM ring for later heap reads.
Thanks for the report and patch. Yes, this was an oversight. > This patch allows a read stream's strategy to be changed for future I/O. > VACUUM switches the active stream to a NULL strategy when failsafe > activates, without modifying in-flight I/O operations. Right, because it can change during an ongoing scan, there has to be some way to set it for a new read after the strategy was deactivated. And we have to make sure we retain information on the IOContext that read operations were started in for accounting purposes. For master, I actually think what we should do is save the IOContext in the ReadBuffersOperation instead of the BufferAccessStrategy. I think it is cleaner since you only need the IOContext when completing the IO and you can pass the BufferAccessStrategy directly to StartReadBuffers() without saving it in the ReadBuffersOperation. And, when it comes to this patch and being able to "deactivate" the buffer access strategy, we need a way to do so for future IOs without affecting in-progress IOs, and this structure seems like the best way to do that. I've included two commits targeted at master. The first replaces BufferAccessStrategy with IOContext in the ReadBuffersOperation and the second resets the BufferAccessStrategy in the read stream to fix the reported bug with failsafe mode. (attached and prefixed "master"). I'll note I did add an include of pgstat.h in bufmgr.h which is pretty undesirable. I'm not sure if I should make a header for the IOContext/IOObject stuff to avoid this. For backbranches, we should just do what you did in your patch. I included an updated version of it with a few tweaks. Luckily ReadStream is opaque, so these changes are still ABI-compatible (attached and prefixed "backbranches"). My proposed changes to master would make ABI-breaking changes to ReadBuffersOperation, so we shouldn't do that in backbranches. - Melanie
From c5b1250a379fb005209b763bd19a075009ddb6d1 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Mon, 3 Aug 2026 17:31:49 -0400 Subject: [PATCH] Fix VACUUM failsafe mode's dropping of the buffer access strategy Since 4830f1024325, when VACUUM's wraparound failsafe triggers, it stops using the BAS_VACUUM buffer access strategy so that the rest of the vacuum may use all of shared buffers and reclaim transaction IDs as quickly as possible. However, when 9256822608f3 made vacuum's first phase use the read stream, it accidentally disabled this functionality. To re-enable it, we have to clear the strategy actually being used by the read stream. We must do this only for newly started reads -- reads already in progress must be accounted under the IO context they were started in. To achieve this in a backpatchable way, track the BufferAccessStrategy in the ReadStream object itself and set the ReadBuffersOperation's strategy member just before each read is started -- in case it has changed. --- src/backend/access/heap/vacuumlazy.c | 19 ++++++++++++++++--- src/backend/storage/aio/read_stream.c | 19 ++++++++++++++++++- src/include/storage/read_stream.h | 1 + 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 39395aed0d5..2dc9a9de6a9 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -1385,6 +1385,17 @@ lazy_scan_heap(LVRelState *vacrel) PROGRESS_VACUUM_PHASE_SCAN_HEAP); } + /* + * If the wraparound failsafe has engaged -- either via the check + * above or during index vacuuming invoked from this loop -- stop + * using the buffer access strategy. Currently in-progress reads are + * not affected. This only affects phase I vacuum scan of the heap, so + * we cannot clear the read stream strategy in + * lazy_check_wraparound_failsafe(). + */ + if (unlikely(VacuumFailsafeActive)) + read_stream_clear_strategy(stream); + buf = read_stream_next_buffer(stream, &per_buffer_data); /* The relation is exhausted. */ @@ -2905,9 +2916,11 @@ lazy_check_wraparound_failsafe(LVRelState *vacrel) VacuumFailsafeActive = true; /* - * Abandon use of a buffer access strategy to allow use of all of - * shared buffers. We assume the caller who allocated the memory for - * the BufferAccessStrategy will free it. + * Clear this just for tidiness. An ongoing phase I heap scan already + * has its own copy of the strategy (which it will clear itself) and + * none of the other vacuum phases will read from the strategy member + * once failsafe mode is engaged. We assume the caller who allocated + * memory for the BufferAccessStrategy will free it. */ vacrel->bstrategy = NULL; diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index a318539e56c..fff17eab3cf 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -119,6 +119,7 @@ struct ReadStream int16 resume_readahead_distance; int16 resume_combine_distance; int read_buffers_flags; + BufferAccessStrategy strategy; bool sync_mode; /* using io_method=sync */ bool batch_mode; /* READ_STREAM_USE_BATCHING */ bool advice_enabled; @@ -441,6 +442,8 @@ read_stream_start_pending_read(ReadStream *stream) while (stream->initialized_buffers < buffer_index + nblocks) stream->buffers[stream->initialized_buffers++] = InvalidBuffer; requested_nblocks = nblocks; + /* Set this here in case the BufferAccessStrategy has changed */ + stream->ios[io_index].op.strategy = stream->strategy; need_wait = StartReadBuffers(&stream->ios[io_index].op, &stream->buffers[buffer_index], stream->pending_read_blocknum, @@ -932,6 +935,7 @@ read_stream_begin_impl(int flags, stream->seq_until_processed = InvalidBlockNumber; stream->temporary = SmgrIsTemp(smgr); stream->distance_decay_holdoff = 0; + stream->strategy = strategy; /* * Skip the initial ramp-up phase if the caller says we're going to be @@ -1376,7 +1380,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) BlockNumber read_stream_next_block(ReadStream *stream, BufferAccessStrategy *strategy) { - *strategy = stream->ios[0].op.strategy; + *strategy = stream->strategy; return read_stream_get_block(stream, NULL); } @@ -1407,6 +1411,19 @@ read_stream_resume(ReadStream *stream) stream->combine_distance = stream->resume_combine_distance; } +/* + * Stop using a buffer access strategy for future reads from this stream. + * + * This does not change whether or not any in-progress IOs are using the + * buffer access strategy. Note that the caller is still responsible for + * freeing the memory. + */ +void +read_stream_clear_strategy(ReadStream *stream) +{ + stream->strategy = NULL; +} + /* * Reset a read stream by releasing any queued up buffers, allowing the stream * to be used again for different blocks. This can be used to clear an diff --git a/src/include/storage/read_stream.h b/src/include/storage/read_stream.h index 48995c6d534..e2dcf1be50a 100644 --- a/src/include/storage/read_stream.h +++ b/src/include/storage/read_stream.h @@ -102,6 +102,7 @@ extern ReadStream *read_stream_begin_smgr_relation(int flags, size_t per_buffer_data_size); extern BlockNumber read_stream_pause(ReadStream *stream); extern void read_stream_resume(ReadStream *stream); +extern void read_stream_clear_strategy(ReadStream *stream); extern void read_stream_reset(ReadStream *stream); extern void read_stream_end(ReadStream *stream); extern void read_stream_enable_stats(ReadStream *stream, struct IOStats *stats); -- 2.47.3
From adbb710f122c08a0594a3082880402dd16e1ec6d Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Mon, 3 Aug 2026 17:06:12 -0400 Subject: [PATCH 1/2] Record the IOContext on a ReadBuffersOperation instead of the strategy A ReadBuffersOperation only needs the buffer access strategy while a read is being started to choose the buffer to read into. When the IO is completed, it needs to know what IOContext to count it under. Instead of saving the strategy in every operation, pass it to StartReadBuffers() and save the IOContext in the operation. We need to save the BufferAccessStrategy in the ReadStream because there are users that may abandon use of it during an ongoing stream, and we need to be able to clear it for future IOs without affecting in-progress IOs. If we have it in the ReadStream itself, it is cleaner not to have it duplicated in each operation as well. --- src/backend/storage/aio/read_stream.c | 11 ++++--- src/backend/storage/buffer/bufmgr.c | 47 ++++++++++++--------------- src/include/storage/bufmgr.h | 16 +++++++-- src/test/modules/test_aio/test_aio.c | 4 +-- 4 files changed, 42 insertions(+), 36 deletions(-) diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index a318539e56c..03dd7fee0ae 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -119,6 +119,7 @@ struct ReadStream int16 resume_readahead_distance; int16 resume_combine_distance; int read_buffers_flags; + BufferAccessStrategy strategy; bool sync_mode; /* using io_method=sync */ bool batch_mode; /* READ_STREAM_USE_BATCHING */ bool advice_enabled; @@ -445,7 +446,8 @@ read_stream_start_pending_read(ReadStream *stream) &stream->buffers[buffer_index], stream->pending_read_blocknum, &nblocks, - flags); + flags, + stream->strategy); stream->pinned_buffers += nblocks; /* Remember whether we need to wait before returning this buffer. */ @@ -932,6 +934,7 @@ read_stream_begin_impl(int flags, stream->seq_until_processed = InvalidBlockNumber; stream->temporary = SmgrIsTemp(smgr); stream->distance_decay_holdoff = 0; + stream->strategy = strategy; /* * Skip the initial ramp-up phase if the caller says we're going to be @@ -962,7 +965,6 @@ read_stream_begin_impl(int flags, stream->ios[i].op.smgr = smgr; stream->ios[i].op.persistence = persistence; stream->ios[i].op.forknum = forknum; - stream->ios[i].op.strategy = strategy; } return stream; @@ -1101,7 +1103,8 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) if (likely(!StartReadBuffer(&stream->ios[0].op, &stream->buffers[oldest_buffer_index], next_blocknum, - flags))) + flags, + stream->strategy))) { /* Fast return. */ read_stream_count_prefetch(stream); @@ -1376,7 +1379,7 @@ read_stream_next_buffer(ReadStream *stream, void **per_buffer_data) BlockNumber read_stream_next_block(ReadStream *stream, BufferAccessStrategy *strategy) { - *strategy = stream->ios[0].op.strategy; + *strategy = stream->strategy; return read_stream_get_block(stream, NULL); } diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index 169829eb020..8d412ee544d 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -1357,11 +1357,11 @@ ReadBuffer_common(Relation rel, SMgrRelation smgr, char smgr_persistence, operation.rel = rel; operation.persistence = persistence; operation.forknum = forkNum; - operation.strategy = strategy; if (StartReadBuffer(&operation, &buffer, blockNum, - flags)) + flags, + strategy)) WaitReadBuffers(&operation); return buffer; @@ -1373,7 +1373,8 @@ StartReadBuffersImpl(ReadBuffersOperation *operation, BlockNumber blockNum, int *nblocks, int flags, - bool allow_forwarding) + bool allow_forwarding, + BufferAccessStrategy strategy) { int actual_nblocks = *nblocks; int maxcombine = 0; @@ -1398,10 +1399,12 @@ StartReadBuffersImpl(ReadBuffersOperation *operation, } else { - io_context = IOContextForStrategy(operation->strategy); + io_context = IOContextForStrategy(strategy); io_object = IOOBJECT_RELATION; } + operation->io_context = io_context; + for (int i = 0; i < actual_nblocks; ++i) { bool found; @@ -1449,7 +1452,7 @@ StartReadBuffersImpl(ReadBuffersOperation *operation, operation->persistence, operation->forknum, blockNum + i, - operation->strategy, + strategy, io_object, io_context, &found); } @@ -1619,10 +1622,12 @@ StartReadBuffers(ReadBuffersOperation *operation, Buffer *buffers, BlockNumber blockNum, int *nblocks, - int flags) + int flags, + BufferAccessStrategy strategy) { return StartReadBuffersImpl(operation, buffers, blockNum, nblocks, flags, - true /* expect forwarded buffers */ ); + true /* expect forwarded buffers */ , + strategy); } /* @@ -1637,13 +1642,15 @@ bool StartReadBuffer(ReadBuffersOperation *operation, Buffer *buffer, BlockNumber blocknum, - int flags) + int flags, + BufferAccessStrategy strategy) { int nblocks = 1; bool result; result = StartReadBuffersImpl(operation, buffer, blocknum, &nblocks, flags, - false /* single block, no forwarding */ ); + false /* single block, no forwarding */ , + strategy); Assert(nblocks == 1); /* single block can't be short */ return result; @@ -1759,20 +1766,13 @@ bool WaitReadBuffers(ReadBuffersOperation *operation) { PgAioReturn *aio_ret = &operation->io_return; - IOContext io_context; IOObject io_object; bool needed_wait = false; if (operation->persistence == RELPERSISTENCE_TEMP) - { - io_context = IOCONTEXT_NORMAL; io_object = IOOBJECT_TEMP_RELATION; - } else - { - io_context = IOContextForStrategy(operation->strategy); io_object = IOOBJECT_RELATION; - } /* * If we get here without an IO operation having been issued, the @@ -1833,7 +1833,7 @@ WaitReadBuffers(ReadBuffersOperation *operation) * itself was already counted earlier in AsyncReadBuffers() -- * either by us or by another backend if this is a foreign IO. */ - pgstat_count_io_op_time(io_object, io_context, IOOP_READ, + pgstat_count_io_op_time(io_object, operation->io_context, IOOP_READ, io_start, 0, 0); } else @@ -1860,7 +1860,7 @@ WaitReadBuffers(ReadBuffersOperation *operation) * Track this as a 'hit' for this backend. The backend * performing the IO will track it as a 'read'. */ - TrackBufferHit(io_object, io_context, + TrackBufferHit(io_object, operation->io_context, operation->rel, operation->persistence, operation->smgr, operation->forknum, blocknum); @@ -1949,21 +1949,14 @@ AsyncReadBuffers(ReadBuffersOperation *operation, int *nblocks_progress) PgAioHandle *ioh; uint32 ioh_flags = 0; void *io_pages[MAX_IO_COMBINE_LIMIT]; - IOContext io_context; IOObject io_object; instr_time io_start; StartBufferIOResult status; if (persistence == RELPERSISTENCE_TEMP) - { - io_context = IOCONTEXT_NORMAL; io_object = IOOBJECT_TEMP_RELATION; - } else - { - io_context = IOContextForStrategy(operation->strategy); io_object = IOOBJECT_RELATION; - } /* * When this IO is executed synchronously, either because the caller will @@ -2080,7 +2073,7 @@ AsyncReadBuffers(ReadBuffersOperation *operation, int *nblocks_progress) * it must have started out as a miss in PinBufferForBlock(). The * other backend will track this as a 'read'. */ - TrackBufferHit(io_object, io_context, + TrackBufferHit(io_object, operation->io_context, operation->rel, operation->persistence, operation->smgr, operation->forknum, blocknum); @@ -2153,7 +2146,7 @@ AsyncReadBuffers(ReadBuffersOperation *operation, int *nblocks_progress) smgrstartreadv(ioh, operation->smgr, forknum, blocknum, io_pages, io_buffers_len); - pgstat_count_io_op_time(io_object, io_context, IOOP_READ, + pgstat_count_io_op_time(io_object, operation->io_context, IOOP_READ, io_start, 1, io_buffers_len * BLCKSZ); if (persistence == RELPERSISTENCE_TEMP) diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h index 6837b35fc6d..d5a31ad428c 100644 --- a/src/include/storage/bufmgr.h +++ b/src/include/storage/bufmgr.h @@ -20,6 +20,7 @@ #include "storage/buf.h" #include "storage/bufpage.h" #include "storage/relfilelocator.h" +#include "pgstat.h" #include "utils/relcache.h" #include "utils/snapmgr.h" @@ -135,7 +136,14 @@ struct ReadBuffersOperation SMgrRelation smgr; char persistence; ForkNumber forknum; - BufferAccessStrategy strategy; + + /* + * The IO context this read is counted under. This is saved in the + * operation because it is possible for the IO context used for read + * operations to change while IO is ongoing, and this read should be + * counted under the IO context in which it was started. + */ + IOContext io_context; /* * The following private members are private state for communication @@ -245,12 +253,14 @@ extern Buffer ReadBufferWithoutRelcache(RelFileLocator rlocator, extern bool StartReadBuffer(ReadBuffersOperation *operation, Buffer *buffer, BlockNumber blocknum, - int flags); + int flags, + BufferAccessStrategy strategy); extern bool StartReadBuffers(ReadBuffersOperation *operation, Buffer *buffers, BlockNumber blockNum, int *nblocks, - int flags); + int flags, + BufferAccessStrategy strategy); extern bool WaitReadBuffers(ReadBuffersOperation *operation); extern void ReleaseBuffer(Buffer buffer); diff --git a/src/test/modules/test_aio/test_aio.c b/src/test/modules/test_aio/test_aio.c index 6270775af7c..ffe9258e1e8 100644 --- a/src/test/modules/test_aio/test_aio.c +++ b/src/test/modules/test_aio/test_aio.c @@ -737,14 +737,14 @@ read_buffers(PG_FUNCTION_ARGS) operation->rel = rel; operation->smgr = smgr; operation->persistence = rel->rd_rel->relpersistence; - operation->strategy = NULL; operation->forknum = MAIN_FORKNUM; io_reqds[nios] = StartReadBuffers(operation, &buffers[nblocks_done], startblock + nblocks_done, &nblocks_this_io, - 0); + 0, + NULL); nblocks_per_io[nios] = nblocks_this_io; nios++; nblocks_done += nblocks_this_io; -- 2.47.3
From 48512d279c820192959a1599b48784c8db40623c Mon Sep 17 00:00:00 2001 From: Melanie Plageman <[email protected]> Date: Mon, 3 Aug 2026 17:06:30 -0400 Subject: [PATCH 2/2] Fix VACUUM failsafe mode's dropping of the buffer access strategy Since 4830f1024325, when VACUUM's wraparound failsafe triggers, it stops using the BAS_VACUUM buffer access strategy so that the rest of the vacuum may use all of shared buffers and reclaim transaction IDs as quickly as possible. However, when 9256822608f3 made vacuum's first phase use the read stream, it accidentally disabled this functionality. To re-enable it, we have to clear the strategy actually being used by the read stream. --- src/backend/access/heap/vacuumlazy.c | 19 ++++++++++++++++--- src/backend/storage/aio/read_stream.c | 13 +++++++++++++ src/include/storage/read_stream.h | 1 + 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 39395aed0d5..2dc9a9de6a9 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -1385,6 +1385,17 @@ lazy_scan_heap(LVRelState *vacrel) PROGRESS_VACUUM_PHASE_SCAN_HEAP); } + /* + * If the wraparound failsafe has engaged -- either via the check + * above or during index vacuuming invoked from this loop -- stop + * using the buffer access strategy. Currently in-progress reads are + * not affected. This only affects phase I vacuum scan of the heap, so + * we cannot clear the read stream strategy in + * lazy_check_wraparound_failsafe(). + */ + if (unlikely(VacuumFailsafeActive)) + read_stream_clear_strategy(stream); + buf = read_stream_next_buffer(stream, &per_buffer_data); /* The relation is exhausted. */ @@ -2905,9 +2916,11 @@ lazy_check_wraparound_failsafe(LVRelState *vacrel) VacuumFailsafeActive = true; /* - * Abandon use of a buffer access strategy to allow use of all of - * shared buffers. We assume the caller who allocated the memory for - * the BufferAccessStrategy will free it. + * Clear this just for tidiness. An ongoing phase I heap scan already + * has its own copy of the strategy (which it will clear itself) and + * none of the other vacuum phases will read from the strategy member + * once failsafe mode is engaged. We assume the caller who allocated + * memory for the BufferAccessStrategy will free it. */ vacrel->bstrategy = NULL; diff --git a/src/backend/storage/aio/read_stream.c b/src/backend/storage/aio/read_stream.c index 03dd7fee0ae..57b107044a9 100644 --- a/src/backend/storage/aio/read_stream.c +++ b/src/backend/storage/aio/read_stream.c @@ -1410,6 +1410,19 @@ read_stream_resume(ReadStream *stream) stream->combine_distance = stream->resume_combine_distance; } +/* + * Stop using a buffer access strategy for future reads from this stream. + * + * This does not change whether or not any in-progress IOs are using the + * buffer access strategy. Note that the caller is still responsible for + * freeing the memory. + */ +void +read_stream_clear_strategy(ReadStream *stream) +{ + stream->strategy = NULL; +} + /* * Reset a read stream by releasing any queued up buffers, allowing the stream * to be used again for different blocks. This can be used to clear an diff --git a/src/include/storage/read_stream.h b/src/include/storage/read_stream.h index 48995c6d534..e2dcf1be50a 100644 --- a/src/include/storage/read_stream.h +++ b/src/include/storage/read_stream.h @@ -102,6 +102,7 @@ extern ReadStream *read_stream_begin_smgr_relation(int flags, size_t per_buffer_data_size); extern BlockNumber read_stream_pause(ReadStream *stream); extern void read_stream_resume(ReadStream *stream); +extern void read_stream_clear_strategy(ReadStream *stream); extern void read_stream_reset(ReadStream *stream); extern void read_stream_end(ReadStream *stream); extern void read_stream_enable_stats(ReadStream *stream, struct IOStats *stats); -- 2.47.3
