From 3c20233dfb16003101804c1ad911b904b2c24889 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Sat, 31 Dec 2022 15:13:01 -0800 Subject: [PATCH v1 2/2] Never just set the all-frozen bit in VM. --- src/backend/access/heap/vacuumlazy.c | 68 ++++++++++++------------- src/backend/access/heap/visibilitymap.c | 5 ++ 2 files changed, 37 insertions(+), 36 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 5d8fd2fb7..4cea237e0 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -259,7 +259,7 @@ static void lazy_vacuum(LVRelState *vacrel); static bool lazy_vacuum_all_indexes(LVRelState *vacrel); static void lazy_vacuum_heap_rel(LVRelState *vacrel); static int lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, - Buffer buffer, int index, Buffer *vmbuffer); + Buffer buffer, int index, Buffer vmbuffer); static bool lazy_check_wraparound_failsafe(LVRelState *vacrel); static void lazy_cleanup_all_indexes(LVRelState *vacrel); static IndexBulkDeleteResult *lazy_vacuum_one_index(Relation indrel, @@ -1038,7 +1038,7 @@ lazy_scan_heap(LVRelState *vacrel) { Size freespace; - lazy_vacuum_heap_page(vacrel, blkno, buf, 0, &vmbuffer); + lazy_vacuum_heap_page(vacrel, blkno, buf, 0, vmbuffer); /* Forget the LP_DEAD items that we just vacuumed */ dead_items->num_items = 0; @@ -1163,11 +1163,13 @@ lazy_scan_heap(LVRelState *vacrel) { /* * We can pass InvalidTransactionId as the cutoff XID here, - * because setting the all-frozen bit doesn't cause recovery - * conflicts. + * because setting the all-frozen bit doesn't need any recovery + * conflicts (heap_freeze_execute_prepared does all we need). + * Also make sure that the all-visible bit is still set. */ visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr, vmbuffer, InvalidTransactionId, + VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN); } @@ -2391,8 +2393,8 @@ lazy_vacuum_all_indexes(LVRelState *vacrel) static void lazy_vacuum_heap_rel(LVRelState *vacrel) { - int index; - BlockNumber vacuumed_pages; + int index = 0; + BlockNumber vacuumed_pages = 0; Buffer vmbuffer = InvalidBuffer; LVSavedErrInfo saved_err_info; @@ -2409,31 +2411,36 @@ lazy_vacuum_heap_rel(LVRelState *vacrel) VACUUM_ERRCB_PHASE_VACUUM_HEAP, InvalidBlockNumber, InvalidOffsetNumber); - vacuumed_pages = 0; - - index = 0; while (index < vacrel->dead_items->num_items) { - BlockNumber tblk; + BlockNumber blkno; Buffer buf; Page page; Size freespace; vacuum_delay_point(); - tblk = ItemPointerGetBlockNumber(&vacrel->dead_items->items[index]); - vacrel->blkno = tblk; - buf = ReadBufferExtended(vacrel->rel, MAIN_FORKNUM, tblk, RBM_NORMAL, + blkno = ItemPointerGetBlockNumber(&vacrel->dead_items->items[index]); + vacrel->blkno = blkno; + + /* + * Pin the visibility map page in case we need to mark the page + * all-visible. In most cases this will be very cheap, because we'll + * already have the correct page pinned anyway. + */ + visibilitymap_pin(vacrel->rel, blkno, &vmbuffer); + + buf = ReadBufferExtended(vacrel->rel, MAIN_FORKNUM, blkno, RBM_NORMAL, vacrel->bstrategy); LockBuffer(buf, BUFFER_LOCK_EXCLUSIVE); - index = lazy_vacuum_heap_page(vacrel, tblk, buf, index, &vmbuffer); + index = lazy_vacuum_heap_page(vacrel, blkno, buf, index, vmbuffer); /* Now that we've vacuumed the page, record its available space */ page = BufferGetPage(buf); freespace = PageGetHeapFreeSpace(page); UnlockReleaseBuffer(buf); - RecordPageWithFreeSpace(vacrel->rel, tblk, freespace); + RecordPageWithFreeSpace(vacrel->rel, blkno, freespace); vacuumed_pages++; } @@ -2468,7 +2475,8 @@ lazy_vacuum_heap_rel(LVRelState *vacrel) * vacrel->dead_items array. * * Caller must have an exclusive buffer lock on the buffer (though a full - * cleanup lock is also acceptable). + * cleanup lock is also acceptable). vmbuffer must be valid and already have + * a pin on blkno's visibility map page. * * index is an offset into the vacrel->dead_items array for the first listed * LP_DEAD item on the page. The return value is the first index immediately @@ -2476,7 +2484,7 @@ lazy_vacuum_heap_rel(LVRelState *vacrel) */ static int lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer, - int index, Buffer *vmbuffer) + int index, Buffer vmbuffer) { VacDeadItems *dead_items = vacrel->dead_items; Page page = BufferGetPage(buffer); @@ -2557,31 +2565,19 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer, * dirty, exclusively locked, and, if needed, a full page image has been * emitted. */ + Assert(!PageIsAllVisible(page)); if (heap_page_is_all_visible(vacrel, buffer, &visibility_cutoff_xid, &all_frozen)) - PageSetAllVisible(page); - - /* - * All the changes to the heap page have been done. If the all-visible - * flag is now set, also set the VM all-visible bit (and, if possible, the - * all-frozen bit) unless this has already been done previously. - */ - if (PageIsAllVisible(page)) { - uint8 flags = 0; - uint8 vm_status = visibilitymap_get_status(vacrel->rel, - blkno, vmbuffer); + uint8 flags = VISIBILITYMAP_ALL_VISIBLE; - /* Set the VM all-frozen bit to flag, if needed */ - if ((vm_status & VISIBILITYMAP_ALL_VISIBLE) == 0) - flags |= VISIBILITYMAP_ALL_VISIBLE; - if ((vm_status & VISIBILITYMAP_ALL_FROZEN) == 0 && all_frozen) + if (all_frozen) flags |= VISIBILITYMAP_ALL_FROZEN; - Assert(BufferIsValid(*vmbuffer)); - if (flags != 0) - visibilitymap_set(vacrel->rel, blkno, buffer, InvalidXLogRecPtr, - *vmbuffer, visibility_cutoff_xid, flags); + PageSetAllVisible(page); + + visibilitymap_set(vacrel->rel, blkno, buffer, InvalidXLogRecPtr, + vmbuffer, visibility_cutoff_xid, flags); } /* Revert to the previous phase information for error traceback */ diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c index 4ed70275e..28489919d 100644 --- a/src/backend/access/heap/visibilitymap.c +++ b/src/backend/access/heap/visibilitymap.c @@ -146,7 +146,9 @@ visibilitymap_clear(Relation rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags char *map; bool cleared = false; + /* Must never remove all_visible bit while leaving all_frozen bit set */ Assert(flags & VISIBILITYMAP_VALID_BITS); + Assert(flags != VISIBILITYMAP_ALL_VISIBLE); #ifdef TRACE_VISIBILITYMAP elog(DEBUG1, "vm_clear %s %d", RelationGetRelationName(rel), heapBlk); @@ -257,7 +259,10 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, Assert(InRecovery || XLogRecPtrIsInvalid(recptr)); Assert(InRecovery || BufferIsValid(heapBuf)); + + /* Must never set all_frozen bit without also setting all_visible bit */ Assert(flags & VISIBILITYMAP_VALID_BITS); + Assert(flags != VISIBILITYMAP_ALL_FROZEN); /* Check that we have the right heap page pinned, if present */ if (BufferIsValid(heapBuf) && BufferGetBlockNumber(heapBuf) != heapBlk) -- 2.38.1