MIME-Version: 1.0
References: 
 <CAAKRu_YX0NP_yhXvPnvDRjVxxprsRBM-_MZzAJskfMydMQ=ETA@mail.gmail.com>
 <CA+TgmoZef8XqRujP1NN=wJdV4SxOtu7rxRozsyAtaEvuVMZhEw@mail.gmail.com>
 <CAAKRu_YxD3UC3BXxS55jPjBC_yj_vn3FVoLvBMwQuHXGDXacGg@mail.gmail.com>
 <CA+TgmobYY2URHKBMh1NHo1zF3Z28TiS_+0aSyRYyBfvauCPZzA@mail.gmail.com>
 <CAAKRu_YOJ3VTKo4Z9vB2hGeTnwVWsL39gXH09vyBUQ7bGtDnKA@mail.gmail.com>
 <yn4zp35kkdsjx6wf47zcfmxgexxt4h2og47pvnw2x5ifyrs3qc@7uw6jyyxuyf7>
 <CAAKRu_ZiuR+YcUc7=TrgANbRakpzCu8X9zqR=Tf0fE6uDbfP1g@mail.gmail.com>
 <CA+TgmoYgCs=SEsohP6Z6R3KKsGaqFqvqxH8vQ_-nY4t+7rK8jg@mail.gmail.com>
 <CAAKRu_ZP-3=SaZykpwDBMJOdUKyW3Wm5JZfPFRR3L5Ac8ouq4w@mail.gmail.com>
 <CAAKRu_bgkOQqu3K5n4YLRsNBZqJ9Rjg80ROqgKSr2UGz4b5hUg@mail.gmail.com>
 <2wk7jo4m4qwh5sn33pfgerdjfujebbccsmmlownybddbh6nawl@mdyyqpqzxjek>
 <CAAKRu_YR-COJ9aGnMUQqt5yoWmUBjikqrd4jGNZYouHaXpis9g@mail.gmail.com>
 <CALdSSPhiCwJwWwgJP1NmqRmnp9RS2tGOBY0gQrfLCbB+OS5_KQ@mail.gmail.com>
 <CAAKRu_YS+Ocm=OzMaZnG4egFiE9v4VYfZ25DXd6jbwegqmGYbQ@mail.gmail.com>
 <CAAKRu_ZGZSqhGt-RcmmfiSheC+1fjQdxy6_+oM-1jMn8hyVptQ@mail.gmail.com>
 <CALdSSPg+B8RTzTXhJvCcKJBqgzhPZkq0E2oqxQdv74ZNZOMVzg@mail.gmail.com>
 <CAAKRu_Zha7HcdQBv8tTtQrcry5332J6kHnOc1X=TT03LzUXDow@mail.gmail.com>
 <CAAKRu_amF00f2T_H8N6pbbe75C22EeX1OqA=svpj8LNO1sdUuw@mail.gmail.com>
 <0AC177F5-5E26-45EE-B273-357C51212AC5@gmail.com>
In-Reply-To: <0AC177F5-5E26-45EE-B273-357C51212AC5@gmail.com>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Tue, 25 Nov 2025 16:43:58 -0500
Message-ID: 
 <CAAKRu_YQ2P0HdL4+JXhM2EN6gTXf=6ma7ZaALrsAY0=q3QgaGQ@mail.gmail.com>
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM
 on-access)
To: Chao Li <li.evan.chao@gmail.com>
Cc: Kirill Reshke <reshkekirill@gmail.com>,
 Andres Freund <andres@anarazel.de>,
	Robert Haas <robertmhaas@gmail.com>, Andrey Borodin <x4mmm@yandex-team.ru>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Heikki Linnakangas <hlinnaka@iki.fi>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAAKRu_YQ2P0HdL4%2BJXhM2EN6gTXf%3D6ma7ZaALrsAY0%3Dq3QgaGQ%40mail.gmail.com>
Precedence: bulk

Thanks for the review!

On Thu, Nov 20, 2025 at 8:10=E2=80=AFPM Chao Li <li.evan.chao@gmail.com> wr=
ote:
>
>   * 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 X=
ID and
> - * multi-XID seen on the relation so far.  They will be updated with old=
est
> - * values present on the page after pruning.  After processing the whole
> - * relation, VACUUM can use these values as the new relfrozenxid/relminm=
xid
> - * for the relation.
> + * HEAP_PAGE_PRUNE_FREEZE option is set in params.  On entry, they conta=
in the
> + * oldest XID and multi-XID seen on the relation so far.  They will be u=
pdated
> + * with oldest values present on the page after pruning.  After processi=
ng 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 *vistes=
t,
> -                                                  int options,
> -                                                  struct VacuumCutoffs *=
cutoffs,
> +heap_page_prune_and_freeze(PruneFreezeParams *params,
>                                                    PruneFreezeResult *pre=
sult,
> -                                                  PruneReason reason,
>                                                    OffsetNumber *off_loc,
>                                                    TransactionId *new_rel=
frozen_xid,
>                                                    MultiXactId *new_relmi=
n_mxid)
>  {
> ```
>
> For this function interface change, I got a concern. The old function com=
ment says "cutoffs contains the freeze cutoffs =E2=80=A6. Required if HEAP_=
PRUNE_FREEZE option is set.=E2=80=9D, meaning that cutoffs is only useful a=
nd 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, rea=
ders are easy to notice:
>
> * when options is 0, cutoffs is null
> ```
>                         heap_page_prune_and_freeze(relation, buffer, vist=
est, 0,
>                                                                          =
  NULL, &presult, PRUNE_ON_ACCESS, &dummy_off_loc, NULL, NULL);
> ```
>
> * when options has HEAP_PAGE_PRUNE_FREEZE, cutoffs is passed in
> ```
>         prune_options =3D HEAP_PAGE_PRUNE_FREEZE;
>         if (vacrel->nindexes =3D=3D 0)
>                 prune_options |=3D HEAP_PAGE_PRUNE_MARK_UNUSED_NOW;
>
>         heap_page_prune_and_freeze(rel, buf, vacrel->vistest, prune_optio=
ns,
>                                                            &vacrel->cutof=
fs, &presult, PRUNE_VACUUM_SCAN,
>                                                            &vacrel->offnu=
m,
>                                                            &vacrel->NewRe=
lfrozenXid, &vacrel->NewRelminMxid);
> ```
>
> So, the change doesn=E2=80=99t break anything, but makes code a little bi=
t 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 !=
=3D NULL);

That's fair. I've gone ahead and pushed a commit with your suggested assert=
.

> 2 - pushed 0001
> ```
> +       PruneFreezeParams params =3D {.relation =3D rel,.buffer =3D buf,
> +               .reason =3D PRUNE_VACUUM_SCAN,.options =3D HEAP_PAGE_PRUN=
E_FREEZE,
> +               .cutoffs =3D &vacrel->cutoffs,.vistest =3D vacrel->vistes=
t
> +       };
> ```
>
> 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=E2=80=99t remember I ever=
 see a designated initializer in PG code. I only remember 3 ways I have see=
n:
>
> * 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 =3D p_rel})
#define BMR_SMGR(p_smgr, p_relpersistence) \
    ((BufferManagerRelation){.smgr =3D p_smgr, .relpersistence =3D
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 =3D false;
> +                                       prstate->all_frozen =3D false;
> ```
>
> Nit: Now setting the both fields to false repeat in 6 places. Maybe add a=
 static inline function, say PruneClearVisibilityFlags(), may improve maint=
ainability.

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 =3D all_frozen =3D
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 =E2=80=9Ct=E2=80=9D.

Fixed in same commit that added the assert.

- Melanie

[1] https://www.postgresql.org/message-id/flat/5B873BED.9080501%40anastigma=
tix.net#4a067c1314783f0e171b4e1be76f7574