Re: Use streaming read I/O in BRIN vacuuming

Hi,

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