Re: Use streaming read I/O in BRIN vacuuming

On Wed, Nov 12, 2025 at 11:57 AM Arseniy Mukhin
<arseniy(dot)mukhin(dot)dev(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Mon, Oct 13, 2025 at 5:54 PM Arseniy Mukhin
> <arseniy(dot)mukhin(dot)dev(at)gmail(dot)com> wrote:
> >
> > Hi,
> >
> > On Thu, Oct 9, 2025 at 2:03 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > On Sun, Aug 31, 2025 at 12:48 PM Arseniy Mukhin
> > > <arseniy(dot)mukhin(dot)dev(at)gmail(dot)com> wrote:
> > > >
> > > > Hi!
> > > >
> > > > On Sun, Aug 31, 2025 at 8:49 PM Andrey Borodin <x4mmm(at)yandex-team(dot)ru> wrote:
> > > > >
> > > > > Hi!
> > > > >
> > > > > > On 31 Aug 2025, at 21:17, Arseniy Mukhin <arseniy(dot)mukhin(dot)dev(at)gmail(dot)com> 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.

Thank you for updating the patch and sharing the performance test
results! The patch looks good to me. I'm going to push it, barring any
objections.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com