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_YQd=2KvomM+RHcpeDKj0bq+peJ=3W-fip+pkvzA-Jq9w@mail.gmail.com>
Date: Wed, 7 Jan 2026 16:14:38 +0800
Cc: Andrey Borodin <x4mmm@yandex-team.ru>,
 Kirill Reshke <reshkekirill@gmail.com>,
 Xuneng Zhou <xunengzhou@gmail.com>,
 Andres Freund <andres@anarazel.de>,
 Robert Haas <robertmhaas@gmail.com>,
 PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Heikki Linnakangas <hlinnaka@iki.fi>
Content-Transfer-Encoding: quoted-printable
Message-Id: <EEA21113-8A6B-4CB3-BE49-6299F513469A@gmail.com>
References: <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>
 <mhf4vkmh3j57zx7vuxp4jagtdzwhu3573pgfpmnjwqa6i6yj5y@sy4ymcdtdklo>
 <CAAKRu_agCLAQ9OjmrTdJe-X=Xr7QnU4d=cfxdQGwc9jNx9w31w@mail.gmail.com>
 <CAAKRu_ayWLg=WDGZZfSPWf0KjPM8u=LBb0D6XaEWyx2_YFFwAQ@mail.gmail.com>
 <CABPTF7X1X810eT7hjRz9M69sjMv1Vui5gXQEGhqYXmkmywhHCQ@mail.gmail.com>
 <CAAKRu_ZcK+ez71W7j+QzMo0gSafTsbGA3TU-fVSptw7yh41BEQ@mail.gmail.com>
 <6BC5DBAB-6084-4BB8-8450-52E9648AB021@gmail.com>
 <CAAKRu_ZCjHoRPfQ8AbMrFY8TOMCPAvZ0_m9SX7yg0edfTk45-g@mail.gmail.com>
 <7F5BCD7A-764D-4D8D-8E27-6F2CAAEA1CEE@gmail.com>
 <CAAKRu_a04jbDACwzRYwzDND31aPyf7Yvz9TAZrTr=+F5bK1aVA@mail.gmail.com>
 <CALdSSPjcv25jmXm29X-MRWZBae6+HwcWfVH1PE8NfD=EMTnkAg@mail.gmail.com>
 <CAAKRu_bwtBEzDwemyim1r6yYonw7FTyFr1HXG8vywCe-MdbPBQ@mail.gmail.com>
 <4379FDA3-9446-4E2C-9C15-32EFE8D4F31B@yandex-team.ru>
 <CAAKRu_YQd=2KvomM+RHcpeDKj0bq+peJ=3W-fip+pkvzA-Jq9w@mail.gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Archived-At: <https://www.postgresql.org/message-id/EEA21113-8A6B-4CB3-BE49-6299F513469A%40gmail.com>
Precedence: bulk


> On Jan 7, 2026, at 01:31, Melanie Plageman <melanieplageman@gmail.com> =
wrote:
>=20
> On Tue, Jan 6, 2026 at 4:40=E2=80=AFAM Andrey Borodin =
<x4mmm@yandex-team.ru> wrote:
>>=20
>>> <v32-0014-Pass-down-information-on-table-modification-to-s.patch>
>>=20
>> I've tried to take an attempt to review some patches of this =
patchset. It's huge and mostly polished.
>=20
> I've added attributed your review on the patches you specifically
> mention here (and from previous emails you sent). Let me know if there
> are other patches you reviewed that you did not mention.
>=20
>> In a step "Pass down information on table modification to scan node" =
you pass SO_HINT_REL_READ_ONLY flag in IndexNext() and =
BitmapTableScanSetup(), but not in IndexNextWithReorder() and =
IndexOnlyNext(). Is there a reason why index scans with ordering cannot =
use on-access VM setting?
>=20
> Great point, I simply hadn't tested those cases and didn't think to
> add them. I've added them in attached v33.
>=20
> While looking at other callers of index_beginscan(), I was wondering
> if systable_beginscan() and systable_beginscan_ordered() should ever
> pass SO_HINT_REL_READ_ONLY. I guess we would need to pass if the
> operation is read-only above the index_beginscan() -- I'm not sure if
> we always know in the caller of systable_beginscan() whether this
> operation will modify the catalog. That seems like it could be a
> separate project, though, so maybe it is better to say this feature is
> just for regular tables.
>=20
> As for the other cases: We don't have the relation range table index
> in check_exclusion_or_unique_constraints(), so I don't think we can do
> it there.
>=20
> And I think that the other index scan cases like in replication code
> or get_actual_variable_endpoint() are too small to be worth it, don't
> have the needed info, or don't do on-access pruning (bc of the
> snapshot type they use).
>=20
>> Also, comment about visibilitymap_set() says "Callers that log VM =
changes separately should use visibilitymap_set()" as if =
visibilitymap_set() is some other function.
>=20
> Ah, yes, I forgot to remove that when I removed the old
> visibilitymap_set() and made visibilitymap_set_vmbits() into
> visiblitymap_set(). Done in v33.
>=20
> - Melanie
> =
<v33-0001-Combine-visibilitymap_set-cases-in-lazy_scan_pru.patch><v33-0002=
-Eliminate-use-of-cached-VM-value-in-lazy_scan_pr.patch><v33-0003-Refactor=
-lazy_scan_prune-VM-clear-logic-into-hel.patch><v33-0004-Set-the-VM-in-hea=
p_page_prune_and_freeze.patch><v33-0005-Move-VM-assert-into-prune-freeze-c=
ode.patch><v33-0006-Eliminate-XLOG_HEAP2_VISIBLE-from-vacuum-phase-I.patch=
><v33-0007-Eliminate-XLOG_HEAP2_VISIBLE-from-empty-page-vac.patch><v33-000=
8-Remove-XLOG_HEAP2_VISIBLE-entirely.patch><v33-0009-Simplify-heap_page_wo=
uld_be_all_visible-visibili.patch><v33-0010-Remove-table_scan_analyze_next=
_tuple-unneeded-pa.patch><v33-0011-Use-GlobalVisState-in-vacuum-to-determi=
ne-page-l.patch><v33-0012-Unset-all_visible-sooner-if-not-freezing.patch><=
v33-0013-Track-which-relations-are-modified-by-a-query.patch><v33-0014-Pas=
s-down-information-on-table-modification-to-s.patch><v33-0015-Allow-on-acc=
ess-pruning-to-set-pages-all-visible.patch><v33-0016-Set-pd_prune_xid-on-i=
nsert.patch>

I see the same problem in 0009 and 0010:

0009
```
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -3570,6 +3570,7 @@ heap_page_would_be_all_visible(Relation rel, =
Buffer buf,
 	{
 		ItemId		itemid;
 		HeapTupleData tuple;
+		TransactionId dead_after;
=20
 		/*
 		 * Set the offset number so that we can display it along =
with any
@@ -3609,12 +3610,14 @@ heap_page_would_be_all_visible(Relation rel, =
Buffer buf,
=20
 		/* Visibility checks may do IO or allocate memory */
 		Assert(CritSectionCount =3D=3D 0);
-		switch (HeapTupleSatisfiesVacuum(&tuple, OldestXmin, =
buf))
+		switch (HeapTupleSatisfiesVacuumHorizon(&tuple, buf, =
&dead_after))
 		{
 			case HEAPTUPLE_LIVE:
 				{
 					TransactionId xmin;
=20
+					=
Assert(!TransactionIdIsValid(dead_after));
+
 					/* Check comments in =
lazy_scan_prune. */
 					if =
(!HeapTupleHeaderXminCommitted(tuple.t_data))
 					{
@@ -3647,8 +3650,10 @@ heap_page_would_be_all_visible(Relation rel, =
Buffer buf,
 				}
 				break;
=20
-			case HEAPTUPLE_DEAD:
 			case HEAPTUPLE_RECENTLY_DEAD:
+				=
Assert(TransactionIdIsValid(dead_after));
+				/* FALLTHROUGH */
```

0010:
```
 static bool
-heapam_scan_analyze_next_tuple(TableScanDesc scan, TransactionId =
OldestXmin,
+heapam_scan_analyze_next_tuple(TableScanDesc scan,
 							   double =
*liverows, double *deadrows,
 							   =
TupleTableSlot *slot)
 {
@@ -1047,6 +1047,7 @@ heapam_scan_analyze_next_tuple(TableScanDesc scan, =
TransactionId OldestXmin,
 		ItemId		itemid;
 		HeapTuple	targtuple =3D &hslot->base.tupdata;
 		bool		sample_it =3D false;
+		TransactionId dead_after;
=20
 		itemid =3D PageGetItemId(targpage, hscan->rs_cindex);
=20
@@ -1069,16 +1070,20 @@ heapam_scan_analyze_next_tuple(TableScanDesc =
scan, TransactionId OldestXmin,
 		targtuple->t_data =3D (HeapTupleHeader) =
PageGetItem(targpage, itemid);
 		targtuple->t_len =3D ItemIdGetLength(itemid);
=20
-		switch (HeapTupleSatisfiesVacuum(targtuple, OldestXmin,
-										=
 hscan->rs_cbuf))
+		switch (HeapTupleSatisfiesVacuumHorizon(targtuple,
+										=
		hscan->rs_cbuf,
+										=
		&dead_after))
 		{
 			case HEAPTUPLE_LIVE:
 				sample_it =3D true;
 				*liverows +=3D 1;
 				break;
=20
-			case HEAPTUPLE_DEAD:
 			case HEAPTUPLE_RECENTLY_DEAD:
+				=
Assert(TransactionIdIsValid(dead_after));
+				/* FALLTHROUGH */
```

I believe the reason why we add Assert(TransactionIdIsValid(dead_after)) =
under HEAPTUPLE_RECENTLY_DEAD is to ensure that when =
HeapTupleSatisfiesVacuumHorizon() returns HEAPTUPLE_RECENTLY_DEAD, =
dead_after must be set. So the goal of the assert is to catch bugs of =
HeapTupleSatisfiesVacuumHorizon().

=46rom this perspective, I now feel dead_after should be initialized to =
InvalidTransactionId. Otherwise, say HeapTupleSatisfiesVacuumHorizon() =
has a bug and miss to set dead_after, then the assert mostly like =
won=E2=80=99t be fired, because it holds a random value, most likely not =
be 0.

I know this comment conflicts to one of my previous comments, sorry =
about that. As I read this patch once and again, I am getting more =
understanding to it.=20

0014
```
+	/* set if the query doesn't modify the rel */
+	SO_HINT_REL_READ_ONLY =3D 1 << 10,
```

Nit: I think it=E2=80=99s better to replace =E2=80=9Crel=E2=80=9D to =
=E2=80=9Crelation=E2=80=9D. For a function comment, if there is a =
parameter named =E2=80=9Crel=E2=80=9D, then we can use it to refer to =
the parameter, without such a context, I guess here a while word is =
better.

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