Re: Confine vacuum skip logic to lazy_scan_skip

On Tue, Feb 11, 2025 at 5:10 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> On Thu, Feb 6, 2025 at 1:06 PM Melanie Plageman
> <melanieplageman(at)gmail(dot)com> wrote:
> >
> > On Wed, Feb 5, 2025 at 5:26 PM Melanie Plageman
> > <melanieplageman(at)gmail(dot)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