From 32684f41d1dd50f726aa0dfe8a5d816aa5c42d64 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Mon, 15 Jan 2024 12:05:52 -0500 Subject: [PATCH v2] Be more consistent about whether to update the FSM while vacuuming. Previously, when lazy_scan_noprune() was called and returned true, we would update the FSM immediately if the relation had no indexes or if the page contained no dead items. On the other hand, when lazy_scan_prune() was called, we would update the FSM if either of those things was true or if index vacuuming was disabled. Eliminate that behavioral difference by considering vacrel->do_index_vacuuming in both cases. Also, instead of having lazy_scan_noprune() make the decision internally and pass it back to the caller via *recordfreespace, just have it pass the number of LP_DEAD items back to the caller, and then let the caller make the decision. That seems less confusing, since the caller also decides in the lazy_scan_prune() case; moreover, this way, the whole test is in one place, instead of spread out. Patch by me, reviewed by Peter Geoghegan and Melanie Plageman. --- src/backend/access/heap/vacuumlazy.c | 58 ++++++++++++++-------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index b63cad1335..f17816b81d 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -252,7 +252,7 @@ static void lazy_scan_prune(LVRelState *vacrel, Buffer buf, LVPagePruneState *prunestate); static bool lazy_scan_noprune(LVRelState *vacrel, Buffer buf, BlockNumber blkno, Page page, - bool *recordfreespace); + bool *has_lpdead_items); static void lazy_vacuum(LVRelState *vacrel); static bool lazy_vacuum_all_indexes(LVRelState *vacrel); static void lazy_vacuum_heap_rel(LVRelState *vacrel); @@ -958,7 +958,7 @@ lazy_scan_heap(LVRelState *vacrel) page = BufferGetPage(buf); if (!ConditionalLockBufferForCleanup(buf)) { - bool recordfreespace; + bool has_lpdead_items; LockBuffer(buf, BUFFER_LOCK_SHARE); @@ -974,17 +974,29 @@ lazy_scan_heap(LVRelState *vacrel) * Collect LP_DEAD items in dead_items array, count tuples, * determine if rel truncation is safe */ - if (lazy_scan_noprune(vacrel, buf, blkno, page, - &recordfreespace)) + if (lazy_scan_noprune(vacrel, buf, blkno, page, &has_lpdead_items)) { Size freespace = 0; + bool recordfreespace; /* - * Processed page successfully (without cleanup lock) -- just - * need to update the FSM, much like the lazy_scan_prune case. - * Don't bother trying to match its visibility map setting - * steps, though. + * 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); @@ -1935,15 +1947,17 @@ lazy_scan_prune(LVRelState *vacrel, * one or more tuples on the page. We always return true for non-aggressive * callers. * - * recordfreespace flag instructs caller on whether or not it should do - * generic FSM processing for page. + * If this function returns true, *has_lpdead_items gets set to true or false + * depending on whether, upon return from this function, any LP_DEAD items are + * present on the page. If this function returns false, *has_lpdead_items + * is not updated. */ static bool lazy_scan_noprune(LVRelState *vacrel, Buffer buf, BlockNumber blkno, Page page, - bool *recordfreespace) + bool *has_lpdead_items) { OffsetNumber offnum, maxoff; @@ -1960,7 +1974,6 @@ lazy_scan_noprune(LVRelState *vacrel, Assert(BufferGetBlockNumber(buf) == blkno); hastup = false; /* for now */ - *recordfreespace = false; /* for now */ lpdead_items = 0; live_tuples = 0; @@ -2102,18 +2115,8 @@ lazy_scan_noprune(LVRelState *vacrel, hastup = true; missed_dead_tuples += lpdead_items; } - - *recordfreespace = true; - } - else if (lpdead_items == 0) - { - /* - * Won't be vacuuming this page later, so record page's freespace in - * the FSM now - */ - *recordfreespace = true; } - else + else if (lpdead_items != 0) { VacDeadItems *dead_items = vacrel->dead_items; ItemPointerData tmp; @@ -2138,12 +2141,6 @@ lazy_scan_noprune(LVRelState *vacrel, dead_items->num_items); vacrel->lpdead_items += lpdead_items; - - /* - * Assume that we'll go on to vacuum this heap page during final pass - * over the heap. Don't record free space until then. - */ - *recordfreespace = false; } /* @@ -2159,6 +2156,9 @@ lazy_scan_noprune(LVRelState *vacrel, if (hastup) vacrel->nonempty_pages = blkno + 1; + /* Did we find LP_DEAD items? */ + *has_lpdead_items = (lpdead_items > 0); + /* Caller won't need to call lazy_scan_prune with same page */ return true; } -- 2.39.3 (Apple Git-145)