On Thu, Jan 18, 2024 at 3:20 PM Robert Haas <robertmh...@gmail.com> wrote: > > On Thu, Jan 18, 2024 at 10:09 AM Robert Haas <robertmh...@gmail.com> wrote: > > Oh, I see. Good catch. > > > > I've now committed 0001. > > I have now also committed 0002 and 0003. I made some modifications to > 0003. Specifically: > > - I moved has_lpdead_items inside the loop over blocks instead of > putting it at the function toplevel. > - I adjusted the comments in lazy_scan_prune() because it seemed to me > that the comment about "Now scan the page..." had gotten too far > separated from the loop where that happens. > - I combined two lines in an if-test because one of them was kinda short. > > Hope that's OK with you.
Awesome, thanks! I have attached a rebased version of the former 0004 as v11-0001. - Melanie
From e8028cce937a2f2b3e5fccb43c6a20868a3d3314 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@gmail.com> Date: Tue, 16 Jan 2024 17:28:07 -0500 Subject: [PATCH v11] Combine FSM updates for prune and no prune cases lazy_scan_prune() and lazy_scan_noprune() update the freespace map with identical conditions -- both with the goal of doing so once for each page vacuumed. Combine both of their FSM updates. This consolidation is easier now that cb970240f13df2 moved visibility map updates into lazy_scan_prune(). While combining the FSM updates, simplify the logic for calling lazy_scan_new_or_empty() and lazy_scan_noprune(). Discussion: https://postgr.es/m/CA%2BTgmoaLTvipm%3Dxx4rJLr07m908PCu%3DQH3uCjD1UOn8YaEuO2g%40mail.gmail.com --- src/backend/access/heap/vacuumlazy.c | 89 +++++++++++----------------- 1 file changed, 33 insertions(+), 56 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 0fb3953513..f9ce555fde 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -838,6 +838,7 @@ lazy_scan_heap(LVRelState *vacrel) Page page; bool all_visible_according_to_vm; bool has_lpdead_items; + bool got_cleanup_lock = false; if (blkno == next_unskippable_block) { @@ -940,54 +941,28 @@ lazy_scan_heap(LVRelState *vacrel) buf = ReadBufferExtended(vacrel->rel, MAIN_FORKNUM, blkno, RBM_NORMAL, vacrel->bstrategy); page = BufferGetPage(buf); - if (!ConditionalLockBufferForCleanup(buf)) - { - LockBuffer(buf, BUFFER_LOCK_SHARE); - /* Check for new or empty pages before lazy_scan_noprune call */ - if (lazy_scan_new_or_empty(vacrel, buf, blkno, page, true, - vmbuffer)) - { - /* Processed as new/empty page (lock and pin released) */ - continue; - } + got_cleanup_lock = ConditionalLockBufferForCleanup(buf); - /* - * Collect LP_DEAD items in dead_items array, count tuples, - * determine if rel truncation is safe - */ - if (lazy_scan_noprune(vacrel, buf, blkno, page, &has_lpdead_items)) - { - Size freespace = 0; - bool recordfreespace; + if (!got_cleanup_lock) + LockBuffer(buf, BUFFER_LOCK_SHARE); - /* - * We processed the page successfully (without a cleanup - * lock). - * - * Update the FSM, just as we would in the case where - * lazy_scan_prune() is called. Our goal is to update the - * freespace map the last time we touch the page. If the - * relation has no indexes, or if index vacuuming is disabled, - * there will be no second heap pass; if this particular page - * has no dead items, the second heap pass will not touch this - * page. So, in those cases, update the FSM now. - * - * After a call to lazy_scan_prune(), we would also try to - * adjust the page-level all-visible bit and the visibility - * map, but we skip that step in this path. - */ - recordfreespace = vacrel->nindexes == 0 - || !vacrel->do_index_vacuuming - || !has_lpdead_items; - if (recordfreespace) - freespace = PageGetHeapFreeSpace(page); - UnlockReleaseBuffer(buf); - if (recordfreespace) - RecordPageWithFreeSpace(vacrel->rel, blkno, freespace); - continue; - } + /* Check for new or empty pages before lazy_scan_[no]prune call */ + if (lazy_scan_new_or_empty(vacrel, buf, blkno, page, !got_cleanup_lock, + vmbuffer)) + { + /* Processed as new/empty page (lock and pin released) */ + continue; + } + /* + * Collect LP_DEAD items in dead_items array, count tuples, determine + * if rel truncation is safe. If lazy_scan_noprune() returns false, we + * must get a cleanup lock and call lazy_scan_prune(). + */ + if (!got_cleanup_lock && + !lazy_scan_noprune(vacrel, buf, blkno, page, &has_lpdead_items)) + { /* * lazy_scan_noprune could not do all required processing. Wait * for a cleanup lock, and call lazy_scan_prune in the usual way. @@ -995,13 +970,7 @@ lazy_scan_heap(LVRelState *vacrel) Assert(vacrel->aggressive); LockBuffer(buf, BUFFER_LOCK_UNLOCK); LockBufferForCleanup(buf); - } - - /* Check for new or empty pages before lazy_scan_prune call */ - if (lazy_scan_new_or_empty(vacrel, buf, blkno, page, false, vmbuffer)) - { - /* Processed as new/empty page (lock and pin released) */ - continue; + got_cleanup_lock = true; } /* @@ -1014,9 +983,10 @@ lazy_scan_heap(LVRelState *vacrel) * tuple headers of remaining items with storage. It also determines * if truncating this block is safe. */ - lazy_scan_prune(vacrel, buf, blkno, page, - vmbuffer, all_visible_according_to_vm, - &has_lpdead_items); + if (got_cleanup_lock) + lazy_scan_prune(vacrel, buf, blkno, page, + vmbuffer, all_visible_according_to_vm, + &has_lpdead_items); /* * Final steps for block: drop cleanup lock, record free space in the @@ -1028,6 +998,12 @@ lazy_scan_heap(LVRelState *vacrel) * space that lazy_vacuum_heap_page() will make available in cases * where it's possible to truncate the page's line pointer array. * + * Our goal is to update the freespace map the last time we touch the + * page. If the relation has no indexes, or if index vacuuming is + * disabled, there will be no second heap pass; if this particular + * page has no dead items, the second heap pass will not touch this + * page. So, in those cases, update the FSM now. + * * Note: It's not in fact 100% certain that we really will call * lazy_vacuum_heap_rel() -- lazy_vacuum() might yet opt to skip index * vacuuming (and so must skip heap vacuuming). This is deemed okay @@ -1047,9 +1023,10 @@ lazy_scan_heap(LVRelState *vacrel) /* * Periodically perform FSM vacuuming to make newly-freed space * visible on upper FSM pages. This is done after vacuuming if the - * table has indexes. + * table has indexes. Space won't have been freed up if only + * lazy_scan_noprune() was called, so check got_cleanup_lock. */ - if (vacrel->nindexes == 0 && has_lpdead_items && + 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, -- 2.37.2
