Hi, On Mon, Oct 13, 2025 at 5:54 PM Arseniy Mukhin <[email protected]> wrote: > > Hi, > > On Thu, Oct 9, 2025 at 2:03 AM Masahiko Sawada <[email protected]> wrote: > > > > On Sun, Aug 31, 2025 at 12:48 PM Arseniy Mukhin > > <[email protected]> wrote: > > > > > > Hi! > > > > > > On Sun, Aug 31, 2025 at 8:49 PM Andrey Borodin <[email protected]> > > > wrote: > > > > > > > > Hi! > > > > > > > > > On 31 Aug 2025, at 21:17, Arseniy Mukhin > > > > > <[email protected]> wrote: > > > > > > > > > > PFA the patch that migrates BRIN vacuum to the read stream API. > > > > > > > > The patch is nice and straightforward. Looks good to me. > > > > > > > > > > Thank you for the review! > > > > > > > Some notes that do not seem to me problem of this patch: > > > > 1. This comment is copied 7 times already across codebase. > > > > "It is safe to use batchmode as block_range_read_stream_cb" > > > > Maybe we can refactor comments and function names... > > > > > > Yes, I had similar thoughts, but having these comments at callsites > > > has its own benefits, there is a thread about these comments [0]... > > > > > > > 2. Somehow brin_vacuum_scan() avoid dance of getting > > > > RelationGetNumberOfBlocks() many times to be entirely sure everything > > > > is scanned. Unlike other index vacuums, see btvacuumscan() for example. > > > > > > If I understand correctly, in other access methods you need to be sure > > > that you read the relation up to the end, so you don't leave any index > > > tuples that should be pruned. BRIN doesn't have a prune phase, there > > > is only a cleanup phase. So it seems it's not a big deal if you miss > > > several pages that were allocated during the vacuum. > > > > > > > Thank you for proposing the patch! I've reviewed the patch and have > > some comments: > > Thank you for the review! > > > > > + stream = read_stream_begin_relation(READ_STREAM_MAINTENANCE | > > + READ_STREAM_FULL | > > + READ_STREAM_SEQUENTIAL | > > + READ_STREAM_USE_BATCHING, > > + strategy, > > + idxrel, > > + MAIN_FORKNUM, > > + block_range_read_stream_cb, > > + &p, > > + 0); > > > > Unlike other index AM's it uses READ_STREAM_SEQUENTIAL. If there are > > some reasons to use it, we should leave comments there. > > Good point, thank you. I looked again at the usage of the > READ_STREAM_SEQUENTIAL flag, and I'm leaning toward not using it here. > But I'm not completely sure, so I decided to ask about the flag usage > in the thread [0]. >
I removed READ_STREAM_SEQUENTIAL. The comment around READ_STREAM_SEQUENTIAL says it should be used for cases where sequential access might not be correctly detected. We use block_range_read_stream_cb here, so the pattern should be clear. PFA the new version. Best regards, Arseniy Mukhin
From 9dd9176f98f57c77ecaff47c2ad1753657f807a6 Mon Sep 17 00:00:00 2001 From: Arseniy Mukhin <[email protected]> Date: Sun, 31 Aug 2025 15:44:17 +0300 Subject: [PATCH v2] Use streaming read I/O in BRIN vacuuming BRIN vacuum processes all index pages in physical order. Now it uses the read stream API instead of explicitly invoking ReadBuffer(). --- src/backend/access/brin/brin.c | 32 +++++++++++++++++++++++--------- 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 2f7d1437919..cb3331921cb 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -2171,28 +2171,42 @@ union_tuples(BrinDesc *bdesc, BrinMemTuple *a, BrinTuple *b) static void brin_vacuum_scan(Relation idxrel, BufferAccessStrategy strategy) { - BlockNumber nblocks; - BlockNumber blkno; + BlockRangeReadStreamPrivate p; + ReadStream *stream; + Buffer buf; + + p.current_blocknum = 0; + p.last_exclusive = RelationGetNumberOfBlocks(idxrel); + + /* + * It is safe to use batchmode as block_range_read_stream_cb takes no + * locks. + */ + stream = read_stream_begin_relation(READ_STREAM_MAINTENANCE | + READ_STREAM_FULL | + READ_STREAM_USE_BATCHING, + strategy, + idxrel, + MAIN_FORKNUM, + block_range_read_stream_cb, + &p, + 0); /* * Scan the index in physical order, and clean up any possible mess in * each page. */ - nblocks = RelationGetNumberOfBlocks(idxrel); - for (blkno = 0; blkno < nblocks; blkno++) + while ((buf = read_stream_next_buffer(stream, NULL)) != InvalidBuffer) { - Buffer buf; - CHECK_FOR_INTERRUPTS(); - buf = ReadBufferExtended(idxrel, MAIN_FORKNUM, blkno, - RBM_NORMAL, strategy); - brin_page_cleanup(idxrel, buf); ReleaseBuffer(buf); } + read_stream_end(stream); + /* * Update all upper pages in the index's FSM, as well. This ensures not * only that we propagate leaf-page FSM updates made by brin_page_cleanup, -- 2.43.0
