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

> On Nov 21, 2025, at 01:19, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> On Wed, Nov 19, 2025 at 6:13 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>>
>> Since it is passed into one of the helpers, I think I agree. Attached
>> v21 has this change.
>
> I've committed the first three patches. Attached v22 is the remaining
> patches which set the VM in heap_page_prune_and_freeze() for vacuum
> and then allow on-access pruning to also set the VM.
>

I just started reviewing 0001 yesterday and got a few comments. However, it was late, I didn’t have enough time to wrap
up,so I decided to review a few more today and send the comments together. As you have pushed 0001-0003, I’d still
raisemy comment for them now, and I will review the rest of commits next week. 

1 - pushed 0001
```
             /*
              * Report the number of tuples reclaimed to pgstats.  This is
@@ -419,60 +425,44 @@ heap_page_will_freeze(Relation relation, Buffer buffer,
  * also need to account for a reduction in the length of the line pointer
  * array following array truncation by us.
  *
- * If the HEAP_PRUNE_FREEZE option is set, we will also freeze tuples if it's
- * required in order to advance relfrozenxid / relminmxid, or if it's
- * considered advantageous for overall system performance to do so now.  The
- * 'cutoffs', 'presult', 'new_relfrozen_xid' and 'new_relmin_mxid' arguments
- * are required when freezing.  When HEAP_PRUNE_FREEZE option is set, we also
- * set presult->all_visible and presult->all_frozen on exit, to indicate if
- * the VM bits can be set.  They are always set to false when the
- * HEAP_PRUNE_FREEZE option is not set, because at the moment only callers
- * that also freeze need that information.
- *
- * vistest is used to distinguish whether tuples are DEAD or RECENTLY_DEAD
- * (see heap_prune_satisfies_vacuum).
- *
- * options:
- *   MARK_UNUSED_NOW indicates that dead items can be set LP_UNUSED during
- *   pruning.
+ * params contains the input parameters used to control freezing and pruning
+ * behavior. See the definition of PruneFreezeParams for more on what each
+ * parameter does.
  *
- *   FREEZE indicates that we will also freeze tuples, and will return
- *   'all_visible', 'all_frozen' flags to the caller.
- *
- * cutoffs contains the freeze cutoffs, established by VACUUM at the beginning
- * of vacuuming the relation.  Required if HEAP_PRUNE_FREEZE option is set.
- * cutoffs->OldestXmin is also used to determine if dead tuples are
- * HEAPTUPLE_RECENTLY_DEAD or HEAPTUPLE_DEAD.
+ * If the HEAP_PAGE_PRUNE_FREEZE option is set in params, we will freeze
+ * tuples if it's required in order to advance relfrozenxid / relminmxid, or
+ * if it's considered advantageous for overall system performance to do so
+ * now.  The 'params.cutoffs', 'presult', 'new_relfrozen_xid' and
+ * 'new_relmin_mxid' arguments are required when freezing.  When
+ * HEAP_PAGE_PRUNE_FREEZE option is passed, we also set presult->all_visible
+ * and presult->all_frozen on exit, to indicate if the VM bits can be set.
+ * They are always set to false when the HEAP_PAGE_PRUNE_FREEZE option is not
+ * passed, because at the moment only callers that also freeze need that
+ * information.
  *
  * presult contains output parameters needed by callers, such as the number of
  * tuples removed and the offsets of dead items on the page after pruning.
  * heap_page_prune_and_freeze() is responsible for initializing it.  Required
  * by all callers.
  *
- * reason indicates why the pruning is performed.  It is included in the WAL
- * record for debugging and analysis purposes, but otherwise has no effect.
- *
  * off_loc is the offset location required by the caller to use in error
  * callback.
  *
  * new_relfrozen_xid and new_relmin_mxid must provided by the caller if the
- * HEAP_PRUNE_FREEZE option is set.  On entry, they contain the oldest XID and
- * multi-XID seen on the relation so far.  They will be updated with oldest
- * values present on the page after pruning.  After processing the whole
- * relation, VACUUM can use these values as the new relfrozenxid/relminmxid
- * for the relation.
+ * HEAP_PAGE_PRUNE_FREEZE option is set in params.  On entry, they contain the
+ * oldest XID and multi-XID seen on the relation so far.  They will be updated
+ * with oldest values present on the page after pruning.  After processing the
+ * whole relation, VACUUM can use these values as the new
+ * relfrozenxid/relminmxid for the relation.
  */
 void
-heap_page_prune_and_freeze(Relation relation, Buffer buffer,
-                           GlobalVisState *vistest,
-                           int options,
-                           struct VacuumCutoffs *cutoffs,
+heap_page_prune_and_freeze(PruneFreezeParams *params,
                            PruneFreezeResult *presult,
-                           PruneReason reason,
                            OffsetNumber *off_loc,
                            TransactionId *new_relfrozen_xid,
                            MultiXactId *new_relmin_mxid)
 {
```

For this function interface change, I got a concern. The old function comment says "cutoffs contains the freeze cutoffs
….Required if HEAP_PRUNE_FREEZE option is set.”, meaning that cutoffs is only useful and must be set when
HEAP_PRUNE_FREEZEis set. But the new comment seems to have lost this indication. 

And in the old function interface, cutoffs sat right next to options, readers are easy to notice:

* when options is 0, cutoffs is null
```
            heap_page_prune_and_freeze(relation, buffer, vistest, 0,
                                       NULL, &presult, PRUNE_ON_ACCESS, &dummy_off_loc, NULL, NULL);
```

* when options has HEAP_PAGE_PRUNE_FREEZE, cutoffs is passed in
```
    prune_options = HEAP_PAGE_PRUNE_FREEZE;
    if (vacrel->nindexes == 0)
        prune_options |= HEAP_PAGE_PRUNE_MARK_UNUSED_NOW;

    heap_page_prune_and_freeze(rel, buf, vacrel->vistest, prune_options,
                               &vacrel->cutoffs, &presult, PRUNE_VACUUM_SCAN,
                               &vacrel->offnum,
                               &vacrel->NewRelfrozenXid, &vacrel->NewRelminMxid);
```

So, the change doesn’t break anything, but makes code a little bit harder to read. So, my suggestion is to add an
assertin heap_page_prune_and_freeze, something like: 

```
Assert(!(params->options & HEAP_PAGE_PRUNE_FREEZE) || params->cutoffs != NULL);
```

2 - pushed 0001
```
+    PruneFreezeParams params = {.relation = rel,.buffer = buf,
+        .reason = PRUNE_VACUUM_SCAN,.options = HEAP_PAGE_PRUNE_FREEZE,
+        .cutoffs = &vacrel->cutoffs,.vistest = vacrel->vistest
+    };
```

Using a designated initializer is not wrong, but makes future maintenance harder, because when a new field is added,
thisinitializer will leave the new field uninitiated. From my impression, I don’t remember I ever see a designated
initializerin PG code. I only remember 3 ways I have seen: 

* use an initialize function to set every fields individually
* palloc0 to set all 0, then set non-zero fields individually
* {0} to set all 0, then set non-zero fields individually

3 - pushed 0002
```
                     prstate->all_visible = false;
+                    prstate->all_frozen = false;
```

Nit: Now setting the both fields to false repeat in 6 places. Maybe add a static inline function, say
PruneClearVisibilityFlags(),may improve maintainability. 

4 - pushed 0003
```
+ * opporunistically freeze, to indicate if the VM bits can be set.  They are
```

Typo: opporunistically, missed a “t”.

I’d stop here today, and continue reviewing rest commits in next week.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/