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_bvQiesumRhgvbcym1T9Z9=ddGfUbi-dSNxLRc6JvL1-w@mail.gmail.com>
Date: Thu, 11 Dec 2025 12:06:57 +0800
Cc: Andres Freund <andres@anarazel.de>,
 Kirill Reshke <reshkekirill@gmail.com>,
 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: <5CEAA162-67B1-44DA-B60D-8B65717E8B05@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>
 <9B2211B5-14B4-4C22-96BD-F7B32E577DB6@gmail.com>
 <CAAKRu_bvkCZ+xBzBjujJMkA5STU+b6AtrUUTjcvAH=ZnnpTtzA@mail.gmail.com>
 <CAAKRu_bvQiesumRhgvbcym1T9Z9=ddGfUbi-dSNxLRc6JvL1-w@mail.gmail.com>
To: Melanie Plageman <melanieplageman@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/5CEAA162-67B1-44DA-B60D-8B65717E8B05%40gmail.com>
Precedence: bulk


> On Dec 11, 2025, at 07:35, Melanie Plageman =
<melanieplageman@gmail.com> wrote:
>=20
> On Tue, Dec 9, 2025 at 12:48=E2=80=AFPM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
>>=20
>> In this set 0001 and 0002 are independent. 0003-0007 are all small
>> steps toward the single change in 0007 which combines the VM updates
>> into the same WAL record as pruning and freezing. 0008 and 0009 are
>> removing the rest of XLOG_HEAP2_VISIBLE. 0010 - 0012 are refactoring
>> needed to set the VM during on-access pruning. 0013 - 0015 are small
>> steps toward setting the VM on-access. And 0016 sets the prune xid on
>> insert so we may set the VM on-access for pages that have only new
>> data.
>=20
> I committed 0001 and 0002. attached v25 reflects that.
> 0001-0004 refactoring steps for eliminate visible record from phase I
> (not probably independent commits in the end)
> 0005 eliminate XLOG_HEAP2_VISIBLE from phase I vac
> 0006-0007 removing the rest of XLOG_HEAP2_VISIBLE
> 0008-0010 refactoring for setting VM on-access
> 0011-0013 setting the VM on-access
> 0014 - setting pd_prune_xid on insert
>=20
> - Melanie
> =
<v25-0001-Combine-visibilitymap_set-cases-in-lazy_scan_pru.patch><v25-0002=
-Refactor-lazy_scan_prune-VM-set-logic-into-helpe.patch><v25-0003-Set-the-=
VM-in-heap_page_prune_and_freeze.patch><v25-0004-Move-VM-assert-into-prune=
-freeze-code.patch><v25-0005-Eliminate-XLOG_HEAP2_VISIBLE-from-vacuum-phas=
e-I.patch><v25-0006-Eliminate-XLOG_HEAP2_VISIBLE-from-empty-page-vac.patch=
><v25-0007-Remove-XLOG_HEAP2_VISIBLE-entirely.patch><v25-0008-Rename-Globa=
lVisTestIsRemovableXid-to-GlobalVisX.patch><v25-0009-Use-GlobalVisState-in=
-vacuum-to-determine-page-l.patch><v25-0011-Track-which-relations-are-modi=
fied-by-a-query.patch><v25-0012-Pass-down-information-on-table-modificatio=
n-to-s.patch><v25-0013-Allow-on-access-pruning-to-set-pages-all-visible.pa=
tch><v25-0014-Set-pd_prune_xid-on-insert.patch>

A few more small comments. Sorry for keeping come out new comments. =
Actually I learned a lot about vacuum from reviewing this patch.

1 - 0001
```
+-- the checkpoint cleans the buffer dirtied by freezing the sole tuple
+checkpoint;
+-- truncating the VM ensures that the next vacuum will need to set it
+select pg_truncate_visibility_map('test_vac_unmodified_heap');
+-- vacuum sets the VM but does not need to set PD_ALL_VISIBLE so no =
heap page
+-- modification
+vacuum test_vac_unmodified_heap;
```

The last vacuum is expected to set vm bits, but the test doesn=E2=80=99t =
verify that. Should we verify that like:
```
evantest=3D# SELECT blkno, all_visible, all_frozen FROM =
pg_visibility_map('test_vac_unmodified_heap');
 blkno | all_visible | all_frozen
-------+-------------+------------
     0 | t           | t
(1 row)
```

As you have been using the extension pg_visibility, adding the =
verification with pg_visibility_map() should not be a burden.

2 - 0001
```
 		if (presult.all_frozen)
 		{
+			/*
+			 * We can pass InvalidTransactionId as our =
cutoff_xid, since a
+			 * snapshotConflictHorizon sufficient to make =
everything safe for
+			 * REDO was logged when the page's tuples were =
frozen.
+			 */
 			=
Assert(!TransactionIdIsValid(presult.vm_conflict_horizon));
-			flags |=3D VISIBILITYMAP_ALL_FROZEN;
+			new_vmbits |=3D VISIBILITYMAP_ALL_FROZEN;
 		}
```

The comment here is a little confusing. In the old code, the Assert() as =
immediately above the call visibilitymap_set(), and cutoff_xid is a =
parameter to the call. But the new code moves the Assert() as well as =
the comment far away from the call visibilitymap_set(), so I think the =
comment should stay together with the call of visibilitymap_set().

3 - 0002
```
 * If it finds that the page-level visibility hint or VM is corrupted, =
it will
* fix them by clearing the VM bits and visibility page hint. This does =
not
```

In the second line, =E2=80=9Cvisibility page hint=E2=80=9D is =
understandable but feels not quite good. I know it=E2=80=99s actually =
=E2=80=9Cpage-level visibility hint=E2=80=9D, so how about just =
=E2=80=9Cvisibility hint=E2=80=9D.

4 - 0002
```
 	/*
-	 * As of PostgreSQL 9.2, the visibility map bit should never be =
set if the
-	 * page-level bit is clear.  However, it's possible that the bit =
got
-	 * cleared after heap_vac_scan_next_block() was called, so we =
must recheck
-	 * with buffer lock before concluding that the VM is corrupt.
+	 * For the purposes of logging, count whether or not the page =
was newly
+	 * set all-visible and, potentially, all-frozen.
 	 */
-	else if (all_visible_according_to_vm && !PageIsAllVisible(page) =
&&
-			 visibilitymap_get_status(vacrel->rel, blkno, =
&vmbuffer) !=3D 0)
+	if ((old_vmbits & VISIBILITYMAP_ALL_VISIBLE) =3D=3D 0 &&
+		(new_vmbits & VISIBILITYMAP_ALL_VISIBLE) !=3D 0)
 	{
```

Without do_set_vm=3D=3Dtrue, old_vmbits will only be 0, thus this =
=E2=80=9Cif-elseif=E2=80=9D that uses old_vmbits should be moved into =
=E2=80=9Cif (do_set_vm)=E2=80=9D. =46rom this perspective, if not =
do_set_vm, this function can return early, like:

```
Do_set_vm =3D heap_page_will_set_vm(&new_vmbits)
If (!do_set_vm)
   Return presult.ndeleted;

PageSetAllVisible(page);
MarkBufferDirty(buf);
old_vmbits =3D visibilitymap_set(new_vmbits);
If (old_vmbits..)
{
..
}
Else if (old_vmbits=E2=80=A6)
{
=E2=80=A6
}

Return presult.ndeleted;
```

5 - 0003
```
 /*
  *	lazy_scan_prune() -- lazy_scan_heap() pruning and freezing.
  *
@@ -2076,15 +1979,14 @@ lazy_scan_prune(LVRelState *vacrel,
 				bool *vm_page_frozen)
 {
 	Relation	rel =3D vacrel->rel;
-	bool		do_set_vm =3D false;
-	uint8		new_vmbits =3D 0;
-	uint8		old_vmbits =3D 0;
 	PruneFreezeResult presult;
 	PruneFreezeParams params =3D {
 		.relation =3D rel,
 		.buffer =3D buf,
+		.vmbuffer =3D vmbuffer,
+		.blk_known_av =3D all_visible_according_to_vm,
 		.reason =3D PRUNE_VACUUM_SCAN,
-		.options =3D HEAP_PAGE_PRUNE_FREEZE,
+		.options =3D HEAP_PAGE_PRUNE_FREEZE | =
HEAP_PAGE_PRUNE_UPDATE_VM,
 		.vistest =3D vacrel->vistest,
 		.cutoffs =3D &vacrel->cutoffs,
 	};
```

This maybe a legacy bug. Here presult is not initialized, and it is =
immediately passed to heap_page_prune_and_freeze():

```
	heap_page_prune_and_freeze(&params,
							   &presult, =
<=3D=3D=3D here
							   =
&vacrel->offnum,
							   =
&vacrel->NewRelfrozenXid, &vacrel->NewRelminMxid);
```

Then heap_page_prune_and_freeze() immediately calls =
prune_freeze_setup():
```
	/* Initialize prstate */
	prune_freeze_setup(params,
					   new_relfrozen_xid, =
new_relmin_mxid,
					   presult, &prstate);
```

And prune_freeze_setup() takes presult as a const pointer:
```
static void
prune_freeze_setup(PruneFreezeParams *params,
				   TransactionId *new_relfrozen_xid,
				   MultiXactId *new_relmin_mxid,
				   const PruneFreezeResult *presult, =
<=3D=3D=3D here
				   PruneState *prstate)
{
    prstate->deadoffsets =3D (OffsetNumber *) presult->deadoffsets; <=3D=3D=
 here, presult->deadoffsets could be a random value
}
```

As this is a separate issue off the current patch, I just filed a new =
patch to fix it. Please take a look at:
=
https://www.postgresql.org/message-id/CAEoWx2%3DjiD1nqch4JQN%2BodAxZSD7mRv=
doHUGJYN2r6tQG_66yQ%40mail.gmail.com

6 - 0003
```
+ * Returns true if one or both VM bits should be set, along with =
returning the
+ * desired what bits should be set in the VM in *new_vmbits.
```

Looks like a typo: =E2=80=9Creturning the desired what bits should be =
set=E2=80=9D, maybe change to =E2=80=9Creturning the desired bits to be =
set=E2=80=9D.

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