Hi Melanie, I resisted this patch again today. I reviewed 0001-0004, and got a few more comments:
> On Dec 4, 2025, at 07:07, Melanie Plageman <[email protected]> wrote: > > <v23-0001-Simplify-vacuum-visibility-assertion.patch><v23-0002-Refactor-lazy_scan_prune-VM-set-logic-into-helpe.patch><v23-0003-Set-the-VM-in-prune-code.patch><v23-0004-Move-VM-assert-into-prune-freeze-code.patch><v23-0005-Eliminate-XLOG_HEAP2_VISIBLE-from-vacuum-phase-I.patch><v23-0006-Eliminate-XLOG_HEAP2_VISIBLE-from-empty-page-vac.patch><v23-0007-Remove-XLOG_HEAP2_VISIBLE-entirely.patch><v23-0008-Rename-GlobalVisTestIsRemovableXid-to-GlobalVisX.patch><v23-0009-Use-GlobalVisState-in-vacuum-to-determine-page-l.patch><v23-0010-Unset-all_visible-sooner-if-not-freezing.patch><v23-0011-Track-which-relations-are-modified-by-a-query.patch><v23-0012-Pass-down-information-on-table-modification-to-s.patch><v23-0013-Allow-on-access-pruning-to-set-pages-all-visible.patch><v23-0014-Set-pd_prune_xid-on-insert.patch> 1 - 0002 ``` +static bool +heap_page_will_set_vis(Relation relation, + BlockNumber heap_blk, + Buffer heap_buf, + Buffer vmbuffer, + bool all_visible_according_to_vm, + const PruneFreezeResult *presult, + uint8 *new_vmbits, + bool *do_set_pd_vis) ``` Actually, I wanted to comment on the new function name in last round of review, but I guess I missed that. I was very confused what “set_vis” means, and finally figured out “vis” should stand for “visibility”. Here “vis” actually means “visibility map bits”. There is the other “vis” in the last parameter’s name “do_set_pd_vis” where the “vis” should be mean “PD_ALL_VISIBLE” bit. So the two “vis” feels making things confusing. How about rename the function to “heap_page_will_set_vm_bits”, and rename the last parameter to “do_set_all_visible”? 2 - 0002 ``` + * Decide whether to set the visibility map bits for heap_blk, using + * information from PruneFreezeResult and all_visible_according_to_vm. This + * function does not actually set the VM bit or page-level hint, + * PD_ALL_VISIBLE. + * + * If it finds that the page-level visibility hint or VM is corrupted, it will + * fix them by clearing the VM bit and page hint. This does not need to be + * done in a critical section. + * + * Returns true if one or both VM bits should be set, along with the desired + * flags in *new_vmbits. Also indicates via do_set_pd_vis whether + * PD_ALL_VISIBLE should be set on the heap page. + */ ``` This function comment mentions PD_ALL_VISIBLE twice, but never mentions ALL_FROZEN. So “Returns true if one or both VM bits should be set” fells unclear. How about rephrase like "Returns true if the all-visible and/or all-frozen VM bits should be set.” 3 - 0002 ``` + /* + * Now handle two potential corruption cases: + * + * These do not need to happen in a critical section and are not + * WAL-logged. + * + * As of PostgreSQL 9.2, the visibility map bit should never be set if the + * page-level bit is clear. However, it's possible that the bit got + * cleared after heap_vac_scan_next_block() was called, so we must recheck + * with buffer lock before concluding that the VM is corrupt. + */ + else if (all_visible_according_to_vm && !PageIsAllVisible(heap_page) && + visibilitymap_get_status(relation, heap_blk, &vmbuffer) != 0) + { + ereport(WARNING, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg("page is not marked all-visible but visibility map bit is set in relation \"%s\" page %u", + RelationGetRelationName(relation), heap_blk))); + + visibilitymap_clear(relation, heap_blk, vmbuffer, + VISIBILITYMAP_VALID_BITS); + } ``` Here in the comment and error message, I guess “visibility map bit” refers to “all visible bit”, can we be explicit? Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
