Re: Use streaming read I/O in BRIN vacuuming

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:

+   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.

---
Have you done any performance testing with this patch? Since BRIN
indexes typically don't grow very large, I'm curious how much benefit
the read_stream provides for BRIN index cleanup.

Regards,

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