Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access)

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 <melanieplageman(at)gmail(dot)com> 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/