From 473633011ff4448cf7332de529ca235f5802c749 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Mon, 2 Jun 2025 11:04:14 -0400 Subject: [PATCH v5 09/20] Update VM in pruneheap.c As a step toward updating the VM in the same critical section and WAL record as pruning and freezing (during phase I of vacuuming), first move the VM update (still in its own critical section and WAL record) into heap_page_prune_and_freeze(). This makes review easier. --- src/backend/access/heap/pruneheap.c | 99 +++++++++++++++++++++++----- src/backend/access/heap/vacuumlazy.c | 99 +++++----------------------- src/include/access/heapam.h | 15 +++-- 3 files changed, 106 insertions(+), 107 deletions(-) diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c index 6c3653e776c..05227ce0339 100644 --- a/src/backend/access/heap/pruneheap.c +++ b/src/backend/access/heap/pruneheap.c @@ -360,7 +360,8 @@ identify_and_fix_vm_corruption(Relation relation, /* * Prune and repair fragmentation and potentially freeze tuples on the - * specified page. + * specified page. If the page's visibility status has changed, update it in + * the VM. * * Caller must have pin and buffer cleanup lock on the page. Note that we * don't update the FSM information for page on caller's behalf. Caller might @@ -436,6 +437,8 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, bool do_freeze; bool do_prune; bool do_hint; + uint8 vmflags = 0; + uint8 old_vmbits = 0; bool hint_bit_fpi; int64 fpi_before = pgWalUsage.wal_fpi; @@ -936,7 +939,7 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, * * Now that freezing has been finalized, unset all_visible if there are * any LP_DEAD items on the page. It needs to reflect the present state - * of the page, as expected by our caller. + * of the page, as expected for updating the visibility map. */ if (prstate.all_visible && prstate.lpdead_items == 0) { @@ -952,31 +955,91 @@ heap_page_prune_and_freeze(Relation relation, Buffer buffer, presult->hastup = prstate.hastup; /* - * For callers planning to update the visibility map, the conflict horizon - * for that record must be the newest xmin on the page. However, if the - * page is completely frozen, there can be no conflict and the - * vm_conflict_horizon should remain InvalidTransactionId. This includes - * the case that we just froze all the tuples; the prune-freeze record - * included the conflict XID already so the caller doesn't need it. + * If updating the visibility map, the conflict horizon for that record + * must be the newest xmin on the page. However, if the page is + * completely frozen, there can be no conflict and the vm_conflict_horizon + * should remain InvalidTransactionId. This includes the case that we + * just froze all the tuples; the prune-freeze record included the + * conflict XID already so the VM update record doesn't need it. */ if (presult->all_frozen) presult->vm_conflict_horizon = InvalidTransactionId; else presult->vm_conflict_horizon = prstate.visibility_cutoff_xid; - presult->lpdead_items = prstate.lpdead_items; - /* the presult->deadoffsets array was already filled in */ - /* - * Clear any VM corruption. This does not need to be done in a critical - * section. + * Handle setting visibility map bit based on information from the VM (as + * of last heap_vac_scan_next_block() call), and from all_visible and + * all_frozen variables. */ - presult->vm_corruption = false; if (options & HEAP_PAGE_PRUNE_UPDATE_VM) - presult->vm_corruption = identify_and_fix_vm_corruption(relation, - blockno, buffer, page, - blk_known_av, - prstate.lpdead_items, vmbuffer); + { + if (identify_and_fix_vm_corruption(relation, + blockno, buffer, page, + blk_known_av, + prstate.lpdead_items, vmbuffer)) + { + /* If we fix corruption, don't update the VM further */ + } + + /* + * If the page isn't yet marked all-visible in the VM or it is and + * needs to me marked all-frozen, update the VM Note that all_frozen + * is only valid if all_visible is true, so we must check both + * all_visible and all_frozen. + */ + else if (presult->all_visible && + (!blk_known_av || + (presult->all_frozen && !VM_ALL_FROZEN(relation, blockno, &vmbuffer)))) + { + Assert(prstate.lpdead_items == 0); + vmflags = VISIBILITYMAP_ALL_VISIBLE; + + /* + * If the page is all-frozen, we can pass InvalidTransactionId as + * our cutoff_xid, since a snapshotConflictHorizon sufficient to + * make everything safe for REDO was logged when the page's tuples + * were frozen. + */ + if (presult->all_frozen) + { + Assert(!TransactionIdIsValid(presult->vm_conflict_horizon)); + vmflags |= VISIBILITYMAP_ALL_FROZEN; + } + + /* + * It's possible for the VM bit to be clear and the page-level bit + * to be set if checksums are not enabled. + * + * And even if we are just planning to update the frozen bit in + * the VM, we shouldn't rely on all_visible_according_to_vm as a + * proxy for the page-level PD_ALL_VISIBLE bit being set, since it + * might have become stale. + * + * If the heap page is all-visible but the VM bit is not set, we + * don't need to dirty the heap page. However, if checksums are + * enabled, we do need to make sure that the heap page is dirtied + * before passing it to visibilitymap_set(), because it may be + * logged. + */ + if (!PageIsAllVisible(page) || XLogHintBitIsNeeded()) + { + PageSetAllVisible(page); + MarkBufferDirty(buffer); + } + + old_vmbits = visibilitymap_set(relation, blockno, buffer, InvalidXLogRecPtr, + vmbuffer, presult->vm_conflict_horizon, + vmflags); + } + } + + presult->lpdead_items = prstate.lpdead_items; + /* the presult->deadoffsets array was already filled in */ + + presult->old_vmbits = old_vmbits; + presult->new_vmbits = vmflags; + if (prstate.freeze) { if (presult->nfrozen > 0) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 0c121fdf4e6..c49e81bc5dd 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -1933,7 +1933,6 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno, return false; } - /* qsort comparator for sorting OffsetNumbers */ static int cmpOffsetNumbers(const void *a, const void *b) @@ -1949,7 +1948,8 @@ cmpOffsetNumbers(const void *a, const void *b) * vmbuffer is the buffer containing the VM block with visibility information * for the heap block, blkno. all_visible_according_to_vm is the saved * visibility status of the heap block looked up earlier by the caller. We - * won't rely entirely on this status, as it may be out of date. + * won't rely entirely on this status, as it may be out of date. These will be + * passed on to heap_page_prune_and_freeze() to use while setting the VM. * * *has_lpdead_items is set to true or false depending on whether, upon return * from this function, any LP_DEAD items are still present on the page. @@ -1978,6 +1978,7 @@ lazy_scan_prune(LVRelState *vacrel, /* * Prune all HOT-update chains and potentially freeze tuples on this page. + * Then, if the page's visibility status has changed, update the VM. * * If the relation has no indexes, we can immediately mark would-be dead * items LP_UNUSED. @@ -1986,10 +1987,6 @@ lazy_scan_prune(LVRelState *vacrel, * presult.ndeleted. It should not be confused with presult.lpdead_items; * presult.lpdead_items's final value can be thought of as the number of * tuples that were deleted from indexes. - * - * We will update the VM after collecting LP_DEAD items and freezing - * tuples. Pruning will have determined whether or not the page is - * all-visible. */ prune_options = HEAP_PAGE_PRUNE_FREEZE | HEAP_PAGE_PRUNE_UPDATE_VM; if (vacrel->nindexes == 0) @@ -2081,88 +2078,26 @@ lazy_scan_prune(LVRelState *vacrel, Assert(!presult.all_visible || !(*has_lpdead_items)); /* - * Handle setting visibility map bit based on information from the VM (as - * of last heap_vac_scan_next_block() call), and from all_visible and - * all_frozen variables. + * For the purposes of logging, count whether or not the page was newly + * set all-visible and, potentially, all-frozen. */ - if (presult.vm_corruption) + if ((presult.old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0 && + (presult.new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0) { - /* Don't update the VM if we just cleared corruption in it */ - } - - /* - * If the page isn't yet marked all-visible in the VM or it is and needs - * to me marked all-frozen, update the VM Note that all_frozen is only - * valid if all_visible is true, so we must check both all_visible and - * all_frozen. - */ - else if (presult.all_visible && - (!all_visible_according_to_vm || - (presult.all_frozen && !VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer)))) - { - uint8 old_vmbits; - uint8 flags = VISIBILITYMAP_ALL_VISIBLE; - - /* - * If the page is all-frozen, we can pass InvalidTransactionId as our - * cutoff_xid, since a snapshotConflictHorizon sufficient to make - * everything safe for REDO was logged when the page's tuples were - * frozen. - */ - if (presult.all_frozen) - { - Assert(!TransactionIdIsValid(presult.vm_conflict_horizon)); - flags |= VISIBILITYMAP_ALL_FROZEN; - } - - /* - * It should never be the case that the visibility map page is set - * while the page-level bit is clear, but the reverse is allowed (if - * checksums are not enabled). Regardless, set both bits so that we - * get back in sync. - * - * If the heap page is all-visible but the VM bit is not set, we don't - * need to dirty the heap page. However, if checksums are enabled, we - * do need to make sure that the heap page is dirtied before passing - * it to visibilitymap_set(), because it may be logged. - */ - if (!PageIsAllVisible(page) || XLogHintBitIsNeeded()) - { - PageSetAllVisible(page); - MarkBufferDirty(buf); - } - - old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf, - InvalidXLogRecPtr, - vmbuffer, presult.vm_conflict_horizon, - flags); - - /* - * Even if we are only setting the all-frozen bit, there is a small - * chance that the VM was modified sometime between setting - * all_visible_according_to_vm and checking the visibility during - * pruning. Check the return value of old_vmbits to ensure the - * visibility map counters used for logging are accurate. - * - * If the page wasn't already set all-visible and/or all-frozen in the - * VM, count it as newly set for logging. - */ - if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0) - { - vacrel->vm_new_visible_pages++; - if (presult.all_frozen) - { - vacrel->vm_new_visible_frozen_pages++; - *vm_page_frozen = true; - } - } - else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 && - presult.all_frozen) + vacrel->vm_new_visible_pages++; + if ((presult.new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0) { - vacrel->vm_new_frozen_pages++; + vacrel->vm_new_visible_frozen_pages++; *vm_page_frozen = true; } } + else if ((presult.old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 && + (presult.new_vmbits & VISIBILITYMAP_ALL_FROZEN) != 0) + { + Assert((presult.new_vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0); + vacrel->vm_new_frozen_pages++; + *vm_page_frozen = true; + } return presult.ndeleted; } diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h index 0c7eb5e46f4..b85648456e9 100644 --- a/src/include/access/heapam.h +++ b/src/include/access/heapam.h @@ -235,20 +235,21 @@ typedef struct PruneFreezeResult int recently_dead_tuples; /* - * all_visible and all_frozen indicate if the all-visible and all-frozen - * bits in the visibility map can be set for this page, after pruning. + * all_visible and all_frozen indicate the status of the page as reflected + * in the visibility map after pruning, freezing, and setting any pages + * all-visible in the visibility map. * - * vm_conflict_horizon is the newest xmin of live tuples on the page. The - * caller can use it as the conflict horizon when setting the VM bits. It - * is only valid if we froze some tuples (nfrozen > 0), and all_frozen is - * true. + * vm_conflict_horizon is the newest xmin of live tuples on the page + * (older than OldestXmin). It will only be valid if we did not set the + * page all-frozen in the VM. * * These are only set if the HEAP_PRUNE_FREEZE option is set. */ bool all_visible; bool all_frozen; TransactionId vm_conflict_horizon; - bool vm_corruption; + uint8 old_vmbits; + uint8 new_vmbits; /* * Whether or not the page makes rel truncation unsafe. This is set to -- 2.43.0