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

On Mon, Jun 2, 2025 at 8:15 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>
> I think that this issue presents since commit c120550edb86 but this
> commit optimized the vacuum work for tables with no indexes and wasn't
> intended to change the FSM vacuum behavior for such tables. Therefore,
> I think we can fix the FSM vacuum to restore the original FSM vacuum
> behavior for HEAD and v18, leaving aside the fact that we might want
> to fix/improve VACUUM_FSM_EVERY_PAGES optimization . The original FSM
> vacuum strategy for tables with no index was that we call
> FreeSpaceMapVacuumRange() for every VACUUM_FSM_EVERY_PAGES, followed
> by a final FreeSpaceMapVacuumRange() call for any remaining pages at
> end of lazy_scan_heap():

Agreed. We should treat this as a bug fix and could separately reduce
unneeded FSM vacuuming.

Reviewing your patch, I think there might be an issue still. You
replaced has_lpdead_items with ndeleted. While ndeleted will count
those items we set LP_UNUSED (which is what we want), it also counts
LP_NORMAL items that vacuum sets LP_REDIRECT (see
heap_prune_record_redirect()).

Looking at PageGetHeapFreeSpace(), it seems like it only considers
LP_UNUSED items as freespace. So, if an item is set LP_REDIRECT, there
would be no need to update the FSM, I think.

I started to wonder why we counted setting an LP_NORMAL item
LP_REDIRECT as a "deleted" tuple. I refactored the code in
heap_prune_chain() in 17, adding heap_prune_record_redirect(), which
increments prstate->ndeleted if the root was LP_NORMAL.

This was the equivalent of the following heap_prune_chain() code in PG 16:

if (OffsetNumberIsValid(latestdead))
{
...
/*
* If the root entry had been a normal tuple, we are deleting it, so
* count it in the result. But changing a redirect (even to DEAD
* state) doesn't count.
*/
if (ItemIdIsNormal(rootlp))
ndeleted++;

/*
* If the DEAD tuple is at the end of the chain, the entire chain is
* dead and the root line pointer can be marked dead. Otherwise just
* redirect the root to the correct chain member.
*/
if (i >= nchain)
heap_prune_record_dead(prstate, rootoffnum);
else
heap_prune_record_redirect(prstate, rootoffnum, chainitems[i]);
}

So, I don't think I changed the behavior in 17 with my refactor. But I
don't know why we would want to count items set LP_REDIRECT as
"reclaimed".

I couldn't actually come up with a case where there was an LP_NORMAL
line pointer that vacuum set LP_REDIRECT and there were no intervening
line pointers in the chain that were being set LP_UNUSED. So, maybe it
is not a problem for this patch in practice.

I would like to understand why we count LP_NORMAL -> LP_REDIRECT items
in the deleted count -- even though that isn't the responsibility of
this patch.

- Melanie