On Wed, Dec 11, 2024 at 2:18 PM Masahiko Sawada <sawada.m...@gmail.com> wrote: > > On Thu, Dec 5, 2024 at 4:32 PM Melanie Plageman > <melanieplage...@gmail.com> wrote: > > > > On Mon, Dec 2, 2024 at 9:28 AM Robert Haas <robertmh...@gmail.com> wrote: > > > > > > > > > All that said, if you really want this broken out into three > > > categories rather than two, I'm not overwhelmingly opposed to that. It > > > is always possible that more detail could be useful; it is just > > > difficult to decide where the likelihood is high enough to justify > > > adding more output. > > I agree that it will be unimportant from Melanie's work in this area. > Also, I agree that if semi-aggressive vacuum has its own new logging > message about what it's done, this new message doesn't necessarily > need to be detailed. But looking at the proposed patches, there seems > to be no such new logging message so far. Showing three categories > makes sense to me independent of semi-aggressive vacuum work. If we > figure out it's better to merge some parts of this new message to > semi-aggressive vacuum's logs, we can adjust them later.
Yes, actually after some review feedback on the eager vacuum patch set from Andres, I switched my approach and these counters are no longer a dependency. (I still require the second patch in this set). However, through this discussion, I've come to think that the VM logging is useful. As such I plan to commit attached v5 (which addresses review feedback and incorporates Tomas' wording suggestion). > > Here's an example to exercise the new log message: > > > > create table foo (a int, b int) with (autovacuum_enabled = false); > > insert into foo select generate_series(1,1000), 1; > > delete from foo where a > 500; > > vacuum (verbose) foo; > > -- visibility map: 5 pages newly set all-visible, of which 2 set > > all-frozen. 0 all-visible pages newly set all-frozen. > > insert into foo select generate_series(1,500), 1; > > vacuum (verbose, freeze) foo; > > --visibility map: 3 pages newly set all-visible, of which 3 set > > all-frozen. 2 all-visible pages newly set all-frozen. > > The output makes sense to me. The patch mostly looks good to me. Here > is one minor comment: > > if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0 && > (old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0) > > Maybe it would be better to rewrite it to: > > if ((old_vmbits & (VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN)) == > 0) Thanks for noticing. Actually Bilal pointed out downthread that I actually don't need to check if the old_vmbits were set neither all-visible nor all-frozen in the first branch -- if they were not all-visible then they would not have been all-frozen (barring some VM corruption -- but then we would have bigger problems). I've changed the code accordingly. - Melanie
From fe3f9372d801e25c55cca5733e9bfa290d0d52a8 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@gmail.com> Date: Thu, 21 Nov 2024 18:36:05 -0500 Subject: [PATCH v5 2/3] Make visibilitymap_set() return previous state of vmbits It can be useful to know the state of a relation page's VM bits before visibilitymap_set(). visibilitymap_set() has the old value on hand, so returning it is simple. This commit does not use visibilitymap_set()'s new return value. Author: Melanie Plageman Reviewed-by: Masahiko Sawada, Andres Freund, Nitin Jadhav, Bilal Yavuz Discussion: https://postgr.es/m/flat/CAAKRu_ZQe26xdvAqo4weHLR%3DivQ8J4xrSfDDD8uXnh-O-6P6Lg%40mail.gmail.com#6d8d2b4219394f774889509bf3bdc13d, https://postgr.es/m/ctdjzroezaxmiyah3gwbwm67defsrwj2b5fpfs4ku6msfpxeia%40mwjyqlhwr2wu --- src/backend/access/heap/visibilitymap.c | 9 +++++++-- src/include/access/visibilitymap.h | 9 ++++++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c index 8b24e7bc33c..5f71fafaa37 100644 --- a/src/backend/access/heap/visibilitymap.c +++ b/src/backend/access/heap/visibilitymap.c @@ -239,8 +239,10 @@ visibilitymap_pin_ok(BlockNumber heapBlk, Buffer vmbuf) * You must pass a buffer containing the correct map page to this function. * Call visibilitymap_pin first to pin the right one. This function doesn't do * any I/O. + * + * Returns the state of the page's VM bits before setting flags. */ -void +uint8 visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, XLogRecPtr recptr, Buffer vmBuf, TransactionId cutoff_xid, uint8 flags) @@ -250,6 +252,7 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, uint8 mapOffset = HEAPBLK_TO_OFFSET(heapBlk); Page page; uint8 *map; + uint8 status; #ifdef TRACE_VISIBILITYMAP elog(DEBUG1, "vm_set %s %d", RelationGetRelationName(rel), heapBlk); @@ -274,7 +277,8 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, map = (uint8 *) PageGetContents(page); LockBuffer(vmBuf, BUFFER_LOCK_EXCLUSIVE); - if (flags != (map[mapByte] >> mapOffset & VISIBILITYMAP_VALID_BITS)) + status = ((map[mapByte] >> mapOffset) & VISIBILITYMAP_VALID_BITS); + if (flags != status) { START_CRIT_SECTION(); @@ -311,6 +315,7 @@ visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, } LockBuffer(vmBuf, BUFFER_LOCK_UNLOCK); + return status; } /* diff --git a/src/include/access/visibilitymap.h b/src/include/access/visibilitymap.h index 1a4d467e6f0..f7779a0fe19 100644 --- a/src/include/access/visibilitymap.h +++ b/src/include/access/visibilitymap.h @@ -31,9 +31,12 @@ extern bool visibilitymap_clear(Relation rel, BlockNumber heapBlk, extern void visibilitymap_pin(Relation rel, BlockNumber heapBlk, Buffer *vmbuf); extern bool visibilitymap_pin_ok(BlockNumber heapBlk, Buffer vmbuf); -extern void visibilitymap_set(Relation rel, BlockNumber heapBlk, Buffer heapBuf, - XLogRecPtr recptr, Buffer vmBuf, TransactionId cutoff_xid, - uint8 flags); +extern uint8 visibilitymap_set(Relation rel, + BlockNumber heapBlk, Buffer heapBuf, + XLogRecPtr recptr, + Buffer vmBuf, + TransactionId cutoff_xid, + uint8 flags); extern uint8 visibilitymap_get_status(Relation rel, BlockNumber heapBlk, Buffer *vmbuf); extern void visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen); extern BlockNumber visibilitymap_prepare_truncate(Relation rel, -- 2.34.1
From 2d0c8c503da3fd53ccec431e5afd8d40e3b667b9 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@gmail.com> Date: Mon, 28 Oct 2024 10:53:37 -0400 Subject: [PATCH v5 1/3] Rename LVRelState->frozen_pages Rename frozen_pages to new_frozen_tuple_pages in LVRelState, the struct used for tracking state during vacuuming of a heap relation. frozen_pages sounds like it includes every all-frozen page. That is a misnomer. It does not include pages with already frozen tuples. It also includes pages that are not actually all-frozen. Author: Melanie Plageman Reviewed-by: Andres Freund, Masahiko Sawada, Nitin Jadhav, Bilal Yavuz Discussion: https://postgr.es.org/message-id/ctdjzroezaxmiyah3gwbwm67defsrwj2b5fpfs4ku6msfpxeia%40mwjyqlhwr2wu --- src/backend/access/heap/vacuumlazy.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 6a3588cf817..3077ee8ec32 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -188,7 +188,7 @@ typedef struct LVRelState BlockNumber rel_pages; /* total number of pages */ BlockNumber scanned_pages; /* # pages examined (not skipped via VM) */ BlockNumber removed_pages; /* # pages removed by relation truncation */ - BlockNumber frozen_pages; /* # pages with newly frozen tuples */ + BlockNumber new_frozen_tuple_pages; /* # pages with newly frozen tuples */ BlockNumber lpdead_item_pages; /* # pages with LP_DEAD items */ BlockNumber missed_dead_pages; /* # pages with missed dead tuples */ BlockNumber nonempty_pages; /* actually, last nonempty page + 1 */ @@ -407,7 +407,7 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, /* Initialize page counters explicitly (be tidy) */ vacrel->scanned_pages = 0; vacrel->removed_pages = 0; - vacrel->frozen_pages = 0; + vacrel->new_frozen_tuple_pages = 0; vacrel->lpdead_item_pages = 0; vacrel->missed_dead_pages = 0; vacrel->nonempty_pages = 0; @@ -696,9 +696,10 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, vacrel->NewRelminMxid, diff); } appendStringInfo(&buf, _("frozen: %u pages from table (%.2f%% of total) had %lld tuples frozen\n"), - vacrel->frozen_pages, + vacrel->new_frozen_tuple_pages, orig_rel_pages == 0 ? 100.0 : - 100.0 * vacrel->frozen_pages / orig_rel_pages, + 100.0 * vacrel->new_frozen_tuple_pages / + orig_rel_pages, (long long) vacrel->tuples_frozen); if (vacrel->do_index_vacuuming) { @@ -1453,11 +1454,12 @@ lazy_scan_prune(LVRelState *vacrel, if (presult.nfrozen > 0) { /* - * We don't increment the frozen_pages instrumentation counter when - * nfrozen == 0, since it only counts pages with newly frozen tuples - * (don't confuse that with pages newly set all-frozen in VM). + * We don't increment the new_frozen_tuple_pages instrumentation + * counter when nfrozen == 0, since it only counts pages with newly + * frozen tuples (don't confuse that with pages newly set all-frozen + * in VM). */ - vacrel->frozen_pages++; + vacrel->new_frozen_tuple_pages++; } /* -- 2.34.1
From 556838bb561b03f43a63070dc904af743d586238 Mon Sep 17 00:00:00 2001 From: Melanie Plageman <melanieplage...@gmail.com> Date: Thu, 31 Oct 2024 18:19:18 -0400 Subject: [PATCH v5 3/3] Count pages set all-visible and all-frozen in VM during vacuum Vacuum already counts and logs pages with newly frozen tuples. Now count and log the number of pages newly set all-visible and all-frozen in the visibility map. Pages that are all-visible but not all-frozen are debt for future aggressive vacuums. The counts of newly all-visible and all-frozen pages give us visibility into the rate at which this debt is being accrued and paid down. Author: Melanie Plageman Reviewed-by: Masahiko Sawada, Alastair Turner, Nitin Jadhav, Andres Freund, Bilal Yavuz, Tomas Vondra Discussion: https://postgr.es/m/flat/CAAKRu_ZQe26xdvAqo4weHLR%3DivQ8J4xrSfDDD8uXnh-O-6P6Lg%40mail.gmail.com#6d8d2b4219394f774889509bf3bdc13d, https://postgr.es/m/ctdjzroezaxmiyah3gwbwm67defsrwj2b5fpfs4ku6msfpxeia%40mwjyqlhwr2wu --- src/backend/access/heap/vacuumlazy.c | 123 ++++++++++++++++++++++++--- 1 file changed, 111 insertions(+), 12 deletions(-) diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c index 3077ee8ec32..68705ca0447 100644 --- a/src/backend/access/heap/vacuumlazy.c +++ b/src/backend/access/heap/vacuumlazy.c @@ -189,6 +189,21 @@ typedef struct LVRelState BlockNumber scanned_pages; /* # pages examined (not skipped via VM) */ BlockNumber removed_pages; /* # pages removed by relation truncation */ BlockNumber new_frozen_tuple_pages; /* # pages with newly frozen tuples */ + + /* # pages newly set all-visible in the VM */ + BlockNumber vm_new_visible_pages; + + /* + * # pages newly set both all-visible and all-frozen in the VM. This is a + * subset of vm_new_visible_pages. That is, vm_new_visible_frozen_pages + * includes only pages previously neither all-visible nor all-frozen in + * the VM but which this vacuum set all-visible and all-frozen. + */ + BlockNumber vm_new_visible_frozen_pages; + + /* # all-visible pages newly set all-frozen in the VM */ + BlockNumber vm_new_frozen_pages; + BlockNumber lpdead_item_pages; /* # pages with LP_DEAD items */ BlockNumber missed_dead_pages; /* # pages with missed dead tuples */ BlockNumber nonempty_pages; /* actually, last nonempty page + 1 */ @@ -428,6 +443,10 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, vacrel->recently_dead_tuples = 0; vacrel->missed_dead_tuples = 0; + vacrel->vm_new_visible_pages = 0; + vacrel->vm_new_visible_frozen_pages = 0; + vacrel->vm_new_frozen_pages = 0; + /* * Get cutoffs that determine which deleted tuples are considered DEAD, * not just RECENTLY_DEAD, and which XIDs/MXIDs to freeze. Then determine @@ -701,6 +720,12 @@ heap_vacuum_rel(Relation rel, VacuumParams *params, 100.0 * vacrel->new_frozen_tuple_pages / orig_rel_pages, (long long) vacrel->tuples_frozen); + + appendStringInfo(&buf, _("visibility map: %u pages set all-visible, %u pages set all-frozen. (%u were all-visible).\n"), + vacrel->vm_new_visible_pages, + vacrel->vm_new_visible_frozen_pages + + vacrel->vm_new_frozen_pages, + vacrel->vm_new_frozen_pages); if (vacrel->do_index_vacuuming) { if (vacrel->nindexes == 0 || vacrel->num_index_scans == 0) @@ -1354,6 +1379,8 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno, */ if (!PageIsAllVisible(page)) { + uint8 old_vmbits; + START_CRIT_SECTION(); /* mark buffer dirty before writing a WAL record */ @@ -1373,10 +1400,24 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno, log_newpage_buffer(buf, true); PageSetAllVisible(page); - visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr, - vmbuffer, InvalidTransactionId, - VISIBILITYMAP_ALL_VISIBLE | VISIBILITYMAP_ALL_FROZEN); + old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf, + InvalidXLogRecPtr, + vmbuffer, InvalidTransactionId, + VISIBILITYMAP_ALL_VISIBLE | + VISIBILITYMAP_ALL_FROZEN); END_CRIT_SECTION(); + + /* + * If the page wasn't already set all-visible and all-frozen in + * the VM, count it as newly set for logging. + */ + if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0) + { + vacrel->vm_new_visible_pages++; + vacrel->vm_new_visible_frozen_pages++; + } + else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0) + vacrel->vm_new_frozen_pages++; } freespace = PageGetHeapFreeSpace(page); @@ -1531,6 +1572,7 @@ lazy_scan_prune(LVRelState *vacrel, */ if (!all_visible_according_to_vm && presult.all_visible) { + uint8 old_vmbits; uint8 flags = VISIBILITYMAP_ALL_VISIBLE; if (presult.all_frozen) @@ -1554,9 +1596,24 @@ lazy_scan_prune(LVRelState *vacrel, */ PageSetAllVisible(page); MarkBufferDirty(buf); - visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr, - vmbuffer, presult.vm_conflict_horizon, - flags); + old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf, + InvalidXLogRecPtr, + vmbuffer, presult.vm_conflict_horizon, + flags); + + /* + * If the page wasn't already set all-visible and 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++; + } + else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 && + presult.all_frozen) + vacrel->vm_new_frozen_pages++; } /* @@ -1606,6 +1663,8 @@ lazy_scan_prune(LVRelState *vacrel, else if (all_visible_according_to_vm && presult.all_visible && presult.all_frozen && !VM_ALL_FROZEN(vacrel->rel, blkno, &vmbuffer)) { + uint8 old_vmbits; + /* * Avoid relying on all_visible_according_to_vm as a proxy for the * page-level PD_ALL_VISIBLE bit being set, since it might have become @@ -1625,10 +1684,32 @@ lazy_scan_prune(LVRelState *vacrel, * was logged when the page's tuples were frozen. */ Assert(!TransactionIdIsValid(presult.vm_conflict_horizon)); - visibilitymap_set(vacrel->rel, blkno, buf, InvalidXLogRecPtr, - vmbuffer, InvalidTransactionId, - VISIBILITYMAP_ALL_VISIBLE | - VISIBILITYMAP_ALL_FROZEN); + old_vmbits = visibilitymap_set(vacrel->rel, blkno, buf, + InvalidXLogRecPtr, + vmbuffer, InvalidTransactionId, + VISIBILITYMAP_ALL_VISIBLE | + VISIBILITYMAP_ALL_FROZEN); + + /* + * The page was likely already set all-visible in the VM. However, + * there is a small chance that it was modified sometime between + * setting all_visible_according_to_vm and checking the visibility + * during pruning. Check the return value of old_vmbits anyway to + * ensure the visibility map counters used for logging are accurate. + */ + if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) == 0) + { + vacrel->vm_new_visible_pages++; + vacrel->vm_new_visible_frozen_pages++; + } + + /* + * We already checked that the page was not set all-frozen in the VM + * above, so we don't need to test the value of old_vmbits. + */ + else + vacrel->vm_new_frozen_pages++; + } } @@ -2274,6 +2355,7 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer, if (heap_page_is_all_visible(vacrel, buffer, &visibility_cutoff_xid, &all_frozen)) { + uint8 old_vmbits; uint8 flags = VISIBILITYMAP_ALL_VISIBLE; if (all_frozen) @@ -2283,8 +2365,25 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer, } PageSetAllVisible(page); - visibilitymap_set(vacrel->rel, blkno, buffer, InvalidXLogRecPtr, - vmbuffer, visibility_cutoff_xid, flags); + old_vmbits = visibilitymap_set(vacrel->rel, blkno, buffer, + InvalidXLogRecPtr, + vmbuffer, visibility_cutoff_xid, + flags); + + /* + * If the page wasn't already set all-visible and 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 (all_frozen) + vacrel->vm_new_visible_frozen_pages++; + } + + else if ((old_vmbits & VISIBILITYMAP_ALL_FROZEN) == 0 && + all_frozen) + vacrel->vm_new_frozen_pages++; } /* Revert to the previous phase information for error traceback */ -- 2.34.1
