Re: Confine vacuum skip logic to lazy_scan_skip

On Thu, Feb 13, 2025 at 4:55 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> 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).

You're right, thanks.

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

Good point.

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

How about adding 'vacrel->scanned_pages > 0' to the if statement?
Which seems not odd to me.

Looking at the 0002 patch, it seems you reverted the change to the
following comment:

/*
* 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 we have not yet processed blkno+1.
*/

I feel that the previous change I saw in v17 is clearer:

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

The rest looks good to me.

Regards,

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