Re: Confine vacuum skip logic to lazy_scan_skip

Thanks for your review! I've made the changes in attached v18.

I do want to know what you think we should do about what you brought
up about lazy_check_wraparound_failsafe() -- given my reply (below).

On Thu, Feb 13, 2025 at 6:08 PM Masahiko Sawada <sawada.mshk@gmail.com> wrote:
>
> 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?

I've done this in 0002 (can't in 0001 because of it being used inside
the while loop itself).

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

Yes, I agonized over this a bit. The problem with calling
lazy_check_wraparound_failsafe() (and vacuum_delay_point() especially)
after reading the first block is that read_stream_next_buffer()
returns the buffer pinned. We don't want to hang onto that pin for a
long time. But I can't move them to the bottom of the loop after we
release the buffer because some of the code paths don't make it that
far. I don't see a good way other than how I did it or special-casing
block 0. What do you think?

> ---
> +   /* 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?

Nope. That was a buglet (fixed in my v17 but I'm glad you caught it
too). Thanks!

>             /*
>              * 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?

Good idea! Done in attached v18.

> 0003:
>
> -   while ((iter_result = TidStoreIterateNext(iter)) != NULL)
>
> I think we can declare iter_result in the main loop of lazy_vacuum_heap_rel().

Done.

- Melanie