From 834c22730b8edc663a5c09e529c34c7ba66d44fb Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Sat, 31 Dec 2022 15:13:01 -0800 Subject: [PATCH v2] Don't accidentally unset all-visible bit in VM. One of the calls to visibilitymap_set() during VACUUM's initial heap pass could unset a page's all-visible bit during the process of setting the same page's all-frozen bit. This could happen in the event of a concurrent HOT update from a transaction that aborts soon after. Since the all_visible_according_to_vm local variable that lazy_scan_heap works off of when setting the VM doesn't reflect the current state of the VM, and since visibilitymap_set() just requested that the all-frozen bit get set in one case, there was a race condition. Heap pages could initially be all-visible just as all_visible_according_to_vm is established, then not be all-visible after the update, and then become eligible to be set all-visible once more following pruning by VACUUM. There is no reason why VACUUM can't remove a concurrently aborted heap-only tuple right away, and so no reason why such a page won't be able to reach the relevant visibilitymap_set() call site. To fix, tighten up the way that visibilitymap_set() is called: request that both the all-visible and all-frozen bits get set whenever the all-frozen bit is set, regardless of what we think we know about the present state of the all-visible bit. Also tighten up the defensive VM checks are performed in passing so that corruption caused by this bug can be detected and repaired by SKIP_PAGES_THRESHOLD vacuuming. Oversight in commit a892234f83, which added the all-frozen bit to the VM fork. Author: Peter Geoghegan Discussion: https://postgr.es/m/CAH2-WznuNGSzF8v6OsgjaC5aYsb3cZ6HW6MLm30X0d65cmSH6A@mail.gmail.com --- src/backend/access/heap/vacuumlazy.c | 97 +++++++++++++------------ src/backend/access/heap/visibilitymap.c | 9 ++- 2 files changed, 55 insertions(+), 51 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index a42e881da..853d1d76a 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -260,7 +260,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, @@ -1040,7 +1040,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; @@ -1092,7 +1092,10 @@ lazy_scan_heap(LVRelState *vacrel) uint8 flags = VISIBILITYMAP_ALL_VISIBLE; if (prunestate.all_frozen) + { + Assert(!TransactionIdIsValid(prunestate.visibility_cutoff_xid)); flags |= VISIBILITYMAP_ALL_FROZEN; + } /* * It should never be the case that the visibility map page is set @@ -1120,8 +1123,8 @@ lazy_scan_heap(LVRelState *vacrel) * got cleared after lazy_scan_skip() was called, so we must recheck * with buffer lock before concluding that the VM is corrupt. */ - else if (all_visible_according_to_vm && !PageIsAllVisible(page) - && VM_ALL_VISIBLE(vacrel->rel, blkno, &vmbuffer)) + else if (all_visible_according_to_vm && !PageIsAllVisible(page) && + visibilitymap_get_status(vacrel->rel, blkno, &vmbuffer) != 0) { elog(WARNING, "page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u", vacrel->relname, blkno); @@ -1165,11 +1168,14 @@ 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. */ + Assert(!TransactionIdIsValid(prunestate.visibility_cutoff_xid)); visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr, vmbuffer, InvalidTransactionId, + VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN); } @@ -1818,7 +1824,11 @@ retry: * cutoff by stepping back from OldestXmin. */ if (prunestate->all_visible && prunestate->all_frozen) + { + /* Using same cutoff when setting VM is now unnecessary */ snapshotConflictHorizon = prunestate->visibility_cutoff_xid; + prunestate->visibility_cutoff_xid = InvalidTransactionId; + } else { /* Avoids false conflicts when hot_standby_feedback in use */ @@ -2388,8 +2398,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; @@ -2406,42 +2416,42 @@ 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++; } - /* Clear the block number information */ vacrel->blkno = InvalidBlockNumber; - if (BufferIsValid(vmbuffer)) - { ReleaseBuffer(vmbuffer); - vmbuffer = InvalidBuffer; - } /* * We set all LP_DEAD items from the first heap pass to LP_UNUSED during @@ -2465,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 @@ -2473,12 +2484,12 @@ 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); OffsetNumber unused[MaxHeapTuplesPerPage]; - int uncnt = 0; + int nunused = 0; TransactionId visibility_cutoff_xid; bool all_frozen; LVSavedErrInfo saved_err_info; @@ -2508,10 +2519,10 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer, Assert(ItemIdIsDead(itemid) && !ItemIdHasStorage(itemid)); ItemIdSetUnused(itemid); - unused[uncnt++] = toff; + unused[nunused++] = toff; } - Assert(uncnt > 0); + Assert(nunused > 0); /* Attempt to truncate line pointer array now */ PageTruncateLinePointerArray(page); @@ -2527,13 +2538,13 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer, xl_heap_vacuum xlrec; XLogRecPtr recptr; - xlrec.nunused = uncnt; + xlrec.nunused = nunused; XLogBeginInsert(); XLogRegisterData((char *) &xlrec, SizeOfHeapVacuum); XLogRegisterBuffer(0, buffer, REGBUF_STANDARD); - XLogRegisterBufData(0, (char *) unused, uncnt * sizeof(OffsetNumber)); + XLogRegisterBufData(0, (char *) unused, nunused * sizeof(OffsetNumber)); recptr = XLogInsert(RM_HEAP2_ID, XLOG_HEAP2_VACUUM); @@ -2554,31 +2565,21 @@ 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) + { + Assert(!TransactionIdIsValid(visibility_cutoff_xid)); 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 1d1ca423a..74ff01bb1 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 clear 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); @@ -256,8 +258,11 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, #endif Assert(InRecovery || XLogRecPtrIsInvalid(recptr)); - Assert(InRecovery || BufferIsValid(heapBuf)); + Assert(InRecovery || PageIsAllVisible((Page) BufferGetPage(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) @@ -299,8 +304,6 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, { Page heapPage = BufferGetPage(heapBuf); - /* caller is expected to set PD_ALL_VISIBLE first */ - Assert(PageIsAllVisible(heapPage)); PageSetLSN(heapPage, recptr); } } -- 2.38.1