Re: Periodic FSM vacuum doesn't happen in one-pass strategy vacuum.

On Mon, Mar 31, 2025 at 3:29 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> On Mon, Mar 31, 2025 at 3:12 PM Melanie Plageman
> <melanieplageman(at)gmail(dot)com> wrote:
> >
> > On Mon, Mar 31, 2025 at 6:03 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> > >
> > > With commit c120550edb86, If we got the cleanup lock on the page,
> > > lazy_scan_prune() marks dead item IDs directly to LP_UNUSED. So the
> > > following check with has_lpdead_items made the periodic FSM vacuum in
> > > the one-pass strategy vacuum no longer being triggered:
> > >
> > > if (got_cleanup_lock && vacrel->nindexes == 0 && has_lpdead_items &&
> > > blkno - next_fsm_block_to_vacuum >= VACUUM_FSM_EVERY_PAGES)
> > > {
> > > FreeSpaceMapVacuumRange(vacrel->rel, next_fsm_block_to_vacuum,
> > > blkno);
> > > next_fsm_block_to_vacuum = blkno;
> > > }
> > >
> > > Before c120550edb86, since we marked dead item IDs to LP_DEAD once
> > > even in the one-pass strategy vacuum, we used to call
> > > lazy_vacuum_heap_page() to vacuum the page and to call
> > > FreeSpaceMapVacuum() periodically, so we had that check.
> >
> > Whoops, yea, I had a feeling that something wasn't right here when I
> > saw that thread a couple weeks ago about FSM vacuuming taking forever
> > at the end of a vacuum and then I remember looking at the code and
> > thinking why is VACUUM_FSM_EVERY_PAGES named that if it only is used
> > when there are no indexes or a multi-pass vacuum.
> >
> > I even made a comment in [1] that we would only do FSM_EVERY_PAGES
> > when we have multi-pass vacuum -- which is even less after 17.
> >
> > Isn't it ironic that I actually broke it.
> >
> > > I've attached a patch to fix it.
> >
> > Looks like you forgot
>
> Oops, attached now.
>
> Looking the places related to VACUUM_FSM_EVERY_PAGES further, the
> comment atop of vacuumlazy.c seems incorrect:
>
> * In between phases, vacuum updates the freespace map (every
> * VACUUM_FSM_EVERY_PAGES).
>
> IIUC the context is two-pass strategy vacuum (i.e., the table has
> indexes) and VACUUM_FSM_EVERY_PAGES is ignored in this case.

I've attached the patch with the commit message. I didn't include the
changes to the comment above in this patch so this is a pure bug
fixing patch.

I'm going to push this fix up to HEAD and v17 early next week, unless
there is no objection.

Regards,

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