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

Thanks for the review!

On Thu, Nov 20, 2025 at 8:10 PM Chao Li <li.evan.chao@gmail.com> 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_FREEZEis 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
assertin 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,
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

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