Re: Use streaming read I/O in BRIN vacuuming - Mailing list pgsql-hackers

On Thu, Nov 13, 2025 at 4:47 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> On Wed, Nov 12, 2025 at 11:57 AM Arseniy Mukhin
> <arseniy.mukhin.dev@gmail.com> wrote:
> >
> > 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
sureeverything 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.

Pushed.

Regards,

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