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

> 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 raise my 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_FREEZE is 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 assert in 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, this initializer will leave the new field uninitiated. From my impression, I don’t remember I ever see a designated initializer in 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/