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

Hi,

On 2023-11-13 17:13:32 -0500, Melanie Plageman wrote:
> I noticed that in lazy_scan_heap(), when there are no indexes on the
> table being vacuumed, we don't release the lock on the heap page buffer
> before vacuuming the freespace map. Other call sites of
> FreeSpaceMapVacuumRange() hold no such lock. It seems like a waste to
> hold a lock we don't need.

I think this undersells the situation a bit. We right now do
FreeSpaceMapVacuumRange() for 8GB of data (VACUUM_FSM_EVERY_PAGES) in the main
fork, while holding an exclusive page level lock. There's no guarantee (or
even high likelihood) that those pages are currently in the page cache, given
that we have processed up to 8GB of heap data since. 8GB of data is roughly
2MB of FSM with the default compilation options.

Of course processing 2MB of FSM doesn't take that long, but still, it seems
worse than just reading a page or two.

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

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

Greetings,

Andres Freund