> On Nov 21, 2025, at 01:19, Melanie Plageman <[email protected]> wrote:
>
> On Wed, Nov 19, 2025 at 6:13 PM Melanie Plageman
> <[email protected]> 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/
