Re: lazy_scan_heap() should release lock on buffer before vacuuming FSM

On Mon, Nov 13, 2023 at 8:26 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2023-11-13 17:13:32 -0500, Melanie Plageman wrote:
> > diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
> > index 6985d299b2..8b729828ce 100644
> > --- a/src/backend/access/heap/vacuumlazy.c
> > +++ b/src/backend/access/heap/vacuumlazy.c
> > @@ -1046,18 +1046,6 @@ lazy_scan_heap(LVRelState *vacrel)
> > /* Forget the LP_DEAD items that we just vacuumed */
> > dead_items->num_items = 0;
> >
> > - /*
> > - * Periodically perform FSM vacuuming to make newly-freed
> > - * space visible on upper FSM pages. Note we have not yet
> > - * performed FSM processing for blkno.
> > - */
> > - if (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;
> > - }
> > -
> > /*
> > * Now perform FSM processing for blkno, and move on to next
> > * page.
> > @@ -1071,6 +1059,18 @@ lazy_scan_heap(LVRelState *vacrel)
> >
> > UnlockReleaseBuffer(buf);
> > RecordPageWithFreeSpace(vacrel->rel, blkno, freespace);
> > +
> > + /*
> > + * Periodically perform FSM vacuuming to make newly-freed
> > + * space visible on upper FSM pages.
> > + */
> > + if (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;
> > + }
> > +
> > continue;
> > }
>
> Previously there was this comment about "not yet", hinting at that being
> important for FreeSpaceMapVacuumRange's API. I think as-is we now don't vacuum
> the actually freshly updated page contents.
>
> FreeSpaceMapVacuumRange()'s comment says:
> * As above, but assume that only heap pages between start and end-1 inclusive
> * have new free-space information, so update only the upper-level slots
> * covering that block range. end == InvalidBlockNumber is equivalent to
> * "all the rest of the relation".
>
> So FreeSpaceMapVacuumRange(..., blkno) will not actually process the "effects"
> of the RecordPageWithFreeSpace() above it - which seems confusing.

Ah, so shall I pass blkno + 1 as end?

> Aside:
>
> I just tried to reach the path and noticed something odd:
>
> =# show autovacuum;
> ┌────────────┐
> │ autovacuum │
> ├────────────┤
> │ off │
> └────────────┘
> (1 row)
>
> =# \dt+ copytest_0
> List of relations
> ┌────────┬────────────┬───────┬────────┬─────────────┬───────────────┬─────────┬─────────────┐
> │ Schema │ Name │ Type │ Owner │ Persistence │ Access method │ Size │ Description │
> ├────────┼────────────┼───────┼────────┼─────────────┼───────────────┼─────────┼─────────────┤
> │ public │ copytest_0 │ table │ andres │ permanent │ heap │ 1143 MB │ │
> └────────┴────────────┴───────┴────────┴─────────────┴───────────────┴─────────┴─────────────┘
>
> =# DELETE FROM copytest_0;
> =# VACUUM (VERBOSE) copytest_0;
> ...
> INFO: 00000: table "copytest_0": truncated 146264 to 110934 pages
> ...
> tuples missed: 5848 dead from 89 pages not removed due to cleanup lock contention
> ...
>
> A bit of debugging later I figured out that this is due to the background
> writer. If I SIGSTOP bgwriter, the whole relation is truncated...

That's a bit sad. But isn't that what you would expect bgwriter to do?
Write out dirty buffers? It doesn't know that those pages consist of
only dead tuples and that you will soon truncate them. Or are you
suggesting that bgwriter should do something like use the FSM to avoid
flushing pages which have a lot of free space? Would the FSM even be
updated in this case to reflect that those pages have free space?

- Melanie