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(dot)mshk(at)gmail(dot)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