From d2190abf366f148bae5307442e8a6245c6922e78 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Mon, 21 Feb 2022 12:46:44 -0800 Subject: [PATCH v9 3/4] Remove aggressive VACUUM skipping special case. Since it's simply never okay to miss out on advancing relfrozenxid during an aggressive VACUUM (that's the whole point), the aggressive case treated any page from a next_unskippable_block-wise skippable block range as an all-frozen page (not a merely all-visible page) during skipping. Such a page might not be all-visible/all-frozen at the point that it actually gets skipped, but it could nevertheless be safely skipped, and then counted in frozenskipped_pages (the page must have been all-frozen back when we determined the extent of the range of blocks to skip, since aggressive VACUUMs _must_ scan all-visible pages). This is necessary to ensure that aggressive VACUUMs are always capable of advancing relfrozenxid. The non-aggressive case behaved slightly differently: it rechecked the visibility map for each page at the point of skipping, and only counted pages in frozenskipped_pages when they were still all-frozen at that time. But it skipped the page either way (since we already committed to skipping the page at the point of the recheck). This was correct, but sometimes resulted in non-aggressive VACUUMs needlessly wasting an opportunity to advance relfrozenxid (when a page was modified in just the wrong way, at just the wrong time). It also resulted in a needless recheck of the visibility map for each and every page skipped during non-aggressive VACUUMs. Avoid these problems by conditioning the "skippable page was definitely all-frozen when range of skippable pages was first determined" behavior on what the visibility map _actually said_ about the range as a whole back when we first determined the extent of the range (don't deduce what must have happened at that time on the basis of aggressive-ness). This allows us to reliably count skipped pages in frozenskipped_pages when they were initially all-frozen. In particular, when a page's visibility map bit is unset after the point where a skippable range of pages is initially determined, but before the point where the page is actually skipped, non-aggressive VACUUMs now count it in frozenskipped_pages, just like aggressive VACUUMs always have [1]. It's not critical for the non-aggressive case to get this right, but there is no reason not to. [1] Actually, it might not work that way when there happens to be a mix of all-visible and all-frozen pages in a range of skippable pages. There is no chance of VACUUM advancing relfrozenxid in this scenario either way, though, so it doesn't matter. --- src/backend/access/heap/vacuumlazy.c | 59 +++++++++++++++++++--------- 1 file changed, 40 insertions(+), 19 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index f14b64dfc..b2d3b039d 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -542,7 +542,14 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, */ if (vacrel->scanned_pages + vacrel->frozenskipped_pages < orig_rel_pages) { - /* Cannot advance relfrozenxid/relminmxid */ + /* + * Skipped some all-visible pages, so definitely cannot advance + * relfrozenxid. This is generally only expected in pg_upgrade + * scenarios, since VACUUM now avoids setting a page to all-visible + * but not all-frozen. However, it's also possible (though quite + * unlikely) that we ended up here because somebody else cleared some + * page's all-frozen flag (without clearing its all-visible flag). + */ Assert(!aggressive); frozenxid_updated = minmulti_updated = false; vac_update_relstats(rel, new_rel_pages, new_live_tuples, @@ -810,7 +817,8 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers) next_failsafe_block, next_fsm_block_to_vacuum; Buffer vmbuffer = InvalidBuffer; - bool skipping_blocks; + bool skipping_blocks, + skipping_allfrozen_blocks; const int initprog_index[] = { PROGRESS_VACUUM_PHASE, PROGRESS_VACUUM_TOTAL_HEAP_BLKS, @@ -905,27 +913,31 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers) * computed, so they'll have no effect on the value to which we can safely * set relfrozenxid. A similar argument applies for MXIDs and relminmxid. */ + skipping_allfrozen_blocks = true; /* iff skipping_blocks */ next_unskippable_block = 0; if (vacrel->skipwithvm) { while (next_unskippable_block < nblocks) { - uint8 vmstatus; + uint8 vmskipflags; - vmstatus = visibilitymap_get_status(vacrel->rel, - next_unskippable_block, - &vmbuffer); + vmskipflags = visibilitymap_get_status(vacrel->rel, + next_unskippable_block, + &vmbuffer); if (vacrel->aggressive) { - if ((vmstatus & VISIBILITYMAP_ALL_FROZEN) == 0) + if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0) break; } else { - if ((vmstatus & VISIBILITYMAP_ALL_VISIBLE) == 0) + if ((vmskipflags & VISIBILITYMAP_ALL_VISIBLE) == 0) break; } vacuum_delay_point(); + + if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0) + skipping_allfrozen_blocks = false; next_unskippable_block++; } } @@ -949,6 +961,8 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers) if (blkno == next_unskippable_block) { + skipping_allfrozen_blocks = true; /* iff skipping_blocks */ + /* Time to advance next_unskippable_block */ next_unskippable_block++; if (vacrel->skipwithvm) @@ -971,6 +985,9 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers) break; } vacuum_delay_point(); + + if ((vmskipflags & VISIBILITYMAP_ALL_FROZEN) == 0) + skipping_allfrozen_blocks = false; next_unskippable_block++; } } @@ -997,8 +1014,11 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers) { /* * The current page can be skipped if we've seen a long enough run - * of skippable blocks to justify skipping it -- provided it's not - * the last page in the relation (according to rel_pages/nblocks). + * of skippable blocks to justify skipping it. An aggressive + * VACUUM can only skip a range of blocks that were determined to + * be all-frozen (not just all-visible) as a group back when the + * next_unskippable_block-wise extent of the range was determined. + * Assert that we got this right in passing. * * We always scan the table's last page to determine whether it * has tuples or not, even if it would otherwise be skipped. This @@ -1006,19 +1026,20 @@ lazy_scan_heap(LVRelState *vacrel, int nworkers) * on the table to attempt a truncation that just fails * immediately because there are tuples on the last page. */ + Assert(!vacrel->aggressive || !skipping_blocks || + skipping_allfrozen_blocks); if (skipping_blocks && blkno < nblocks - 1) { /* - * Tricky, tricky. If this is in aggressive vacuum, the page - * must have been all-frozen at the time we checked whether it - * was skippable, but it might not be any more. We must be - * careful to count it as a skipped all-frozen page in that - * case, or else we'll think we can't update relfrozenxid and - * relminmxid. If it's not an aggressive vacuum, we don't - * know whether it was initially all-frozen, so we have to - * recheck. + * When skipping a range of blocks with one or more blocks + * that are not all-frozen (expected during a non-aggressive + * VACUUM following pg_upgrade), we need to recheck if this + * block is all-frozen to maintain frozenskipped_pages. The + * block might not even be all-visible by now, but it's always + * okay to skip (see note above about visibilitymap_get_status + * return value being out-of-date). */ - if (vacrel->aggressive || + if (skipping_allfrozen_blocks || VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer)) vacrel->frozenskipped_pages++; continue; -- 2.30.2