Re: Confine vacuum skip logic to lazy_scan_skip - Mailing list pgsql-hackers

On Tue, Feb 11, 2025 at 5:10 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Thu, Feb 6, 2025 at 1:06 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > On Wed, Feb 5, 2025 at 5:26 PM Melanie Plageman
> > <melanieplageman@gmail.com> wrote:
> > >
> > > Yes, looking at these results, I also feel good about it. I've updated
> > > the commit metadata in attached v14, but I could use a round of review
> > > before pushing it.
> >
> > I've done a bit of self-review and updated these patches.
>
> This needed a rebase in light of 052026c9b90.
> v16 attached has an additional commit which converts the block
> information parameters to heap_vac_scan_next_block() into flags
> because we can only get one piece of information per block from the
> read stream API. This seemed nicer than a cumbersome struct.
>

Sorry for the late chiming in. I've reviewed the v16 patch set, and
the patches mostly look good. Here are some comments mostly about
cosmetic things:

0001:

-   bool        all_visible_according_to_vm,
-               was_eager_scanned = false;
+   uint8       blk_flags = 0;

Can we probably declare blk_flags inside the main loop?


0002:

In lazy_scan_heap(), we have a failsafe check at the beginning of the
main loop, which is performed before reading the first block. Isn't it
better to move this check after scanning a block (especially after
incrementing scanned_pages)? Otherwise, we would end up calling
lazy_check_wraparound_failsafe() at the very first loop, which
previously didn't happen without the patch. Since we already called
lazy_check_wraparound_failsafe() just before calling lazy_scan_heap(),
the extra check would not make much sense.

---
+   /* Set up the read stream for vacuum's first pass through the heap */
+   stream = read_stream_begin_relation(READ_STREAM_MAINTENANCE,
+                                       vacrel->bstrategy,
+                                       vacrel->rel,
+                                       MAIN_FORKNUM,
+                                       heap_vac_scan_next_block,
+                                       vacrel,
+                                       sizeof(bool));

Is there any reason to use sizeof(bool) instead of sizeof(uint8) here?

---
            /*
             * Vacuum the Free Space Map to make newly-freed space visible on
-            * upper-level FSM pages.  Note we have not yet processed blkno.
+            * upper-level FSM pages.  Note that blkno is the previously
+            * processed block.
             */
            FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
                                    blkno);

Given the blkno is already processed, should we pass 'blkno + 1'
instead of blkno?


0003:

-   while ((iter_result = TidStoreIterateNext(iter)) != NULL)

I think we can declare iter_result in the main loop of lazy_vacuum_heap_rel().

Regards,


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