Thanks for the review!
On Thu, Nov 20, 2025 at 8:10 PM Chao Li <[email protected]> wrote:
>
> * 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.
I did move that comment into the PruneFreezeParams struct definition.
> 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);
That's fair. I've gone ahead and pushed a commit with your suggested assert.
> 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
Well, the main reason you don't see them much in the code is that a
lot of the code is old and we didn't require a c99-compliant compiler
until fairly recently (okay like 2018/2019) -- and thus couldn't use
designated initializers.
I agree that they are rare for structs (they are quite commonly used
with arrays), but they are there -- for example these bufmgr init
macros
#define BMR_REL(p_rel) \
((BufferManagerRelation){.rel = p_rel})
#define BMR_SMGR(p_smgr, p_relpersistence) \
((BufferManagerRelation){.smgr = p_smgr, .relpersistence =
p_relpersistence})
#define BMR_GET_SMGR(bmr) \
(RelationIsValid((bmr).rel) ? RelationGetSmgr((bmr).rel) : (bmr).smgr)
I don't see how it would be harder to remember to initialize a field
with a designated initializer vs if you have to just remember to add a
line initializing that field in the code. And using a designated
initializer ensures all unspecified fields will be zeroed out.
In general, I have seen threads [1] encouraging the use of designated
initializers, so I'm inclined to leave it as is since it is committed,
and I haven't heard other pushback.
> 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.
I see your point. However, I don't think it would necessarily be an
improvement. This function already has a lot of helpers you have to
jump to to understand what's going on. And in the place where they are
cleared most often, heap_prune_record_unchanged_lp_normal(), we set
other fields of the prstate directly, so it is nice visual symmetry in
my opinion to set them inline.
I did want to use chained assignment (all_visible = all_frozen =
false), but I have had people complain about that in my code before
more than once, so I refrained.
> 4 - pushed 0003
> ```
> + * opporunistically freeze, to indicate if the VM bits can be set. They are
> ```
>
> Typo: opporunistically, missed a “t”.
Fixed in same commit that added the assert.
- Melanie
[1]
https://www.postgresql.org/message-id/flat/5B873BED.9080501%40anastigmatix.net#4a067c1314783f0e171b4e1be76f7574
