Content-Type: text/plain;
	charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3826.700.81\))
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM
 on-access)
From: Chao Li <li.evan.chao@gmail.com>
In-Reply-To: 
 <CAAKRu_amF00f2T_H8N6pbbe75C22EeX1OqA=svpj8LNO1sdUuw@mail.gmail.com>
Date: Fri, 21 Nov 2025 09:09:21 +0800
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-Transfer-Encoding: quoted-printable
Message-Id: <0AC177F5-5E26-45EE-B273-357C51212AC5@gmail.com>
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>
To: Melanie Plageman <melanieplageman@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/0AC177F5-5E26-45EE-B273-357C51212AC5%40gmail.com>
Precedence: bulk


> On Nov 21, 2025, at 01:19, Melanie Plageman =
<melanieplageman@gmail.com> wrote:
>=20
> On Wed, Nov 19, 2025 at 6:13=E2=80=AFPM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>>=20
>> Since it is passed into one of the helpers, I think I agree. Attached
>> v21 has this change.
>=20
> 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.
>=20

I just started reviewing 0001 yesterday and got a few comments. However, =
it was late, I didn=E2=80=99t 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=E2=80=99d 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 =E2=80=A6. Required if =
HEAP_PRUNE_FREEZE option is set.=E2=80=9D, 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 =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_options,
							   =
&vacrel->cutoffs, &presult, PRUNE_VACUUM_SCAN,
							   =
&vacrel->offnum,
							   =
&vacrel->NewRelfrozenXid, &vacrel->NewRelminMxid);
```

So, the change doesn=E2=80=99t 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 !=3D=
 NULL);
```

2 - pushed 0001
```
+	PruneFreezeParams params =3D {.relation =3D rel,.buffer =3D buf,
+		.reason =3D PRUNE_VACUUM_SCAN,.options =3D =
HEAP_PAGE_PRUNE_FREEZE,
+		.cutoffs =3D &vacrel->cutoffs,.vistest =3D =
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. =46rom my impression, I don=E2=80=99=
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 =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 =
maintainability.

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.

I=E2=80=99d stop here today, and continue reviewing rest commits in next =
week.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/