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_agCLAQ9OjmrTdJe-X=Xr7QnU4d=cfxdQGwc9jNx9w31w@mail.gmail.com>
Date: Thu, 4 Dec 2025 13:10:33 +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: <9B2211B5-14B4-4C22-96BD-F7B32E577DB6@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>
To: Melanie Plageman <melanieplageman@gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/9B2211B5-14B4-4C22-96BD-F7B32E577DB6%40gmail.com>
Precedence: bulk

Hi Melanie,

I resisted this patch again today. I reviewed 0001-0004, and got a few =
more comments:

> On Dec 4, 2025, at 07:07, Melanie Plageman <melanieplageman@gmail.com> =
wrote:
>=20
> =
<v23-0001-Simplify-vacuum-visibility-assertion.patch><v23-0002-Refactor-la=
zy_scan_prune-VM-set-logic-into-helpe.patch><v23-0003-Set-the-VM-in-prune-=
code.patch><v23-0004-Move-VM-assert-into-prune-freeze-code.patch><v23-0005=
-Eliminate-XLOG_HEAP2_VISIBLE-from-vacuum-phase-I.patch><v23-0006-Eliminat=
e-XLOG_HEAP2_VISIBLE-from-empty-page-vac.patch><v23-0007-Remove-XLOG_HEAP2=
_VISIBLE-entirely.patch><v23-0008-Rename-GlobalVisTestIsRemovableXid-to-Gl=
obalVisX.patch><v23-0009-Use-GlobalVisState-in-vacuum-to-determine-page-l.=
patch><v23-0010-Unset-all_visible-sooner-if-not-freezing.patch><v23-0011-T=
rack-which-relations-are-modified-by-a-query.patch><v23-0012-Pass-down-inf=
ormation-on-table-modification-to-s.patch><v23-0013-Allow-on-access-prunin=
g-to-set-pages-all-visible.patch><v23-0014-Set-pd_prune_xid-on-insert.patc=
h>

1 - 0002
```
+static bool
+heap_page_will_set_vis(Relation relation,
+					   BlockNumber heap_blk,
+					   Buffer heap_buf,
+					   Buffer vmbuffer,
+					   bool =
all_visible_according_to_vm,
+					   const PruneFreezeResult =
*presult,
+					   uint8 *new_vmbits,
+					   bool *do_set_pd_vis)
```

Actually, I wanted to comment on the new function name in last round of =
review, but I guess I missed that.

I was very confused what =E2=80=9Cset_vis=E2=80=9D means, and finally =
figured out =E2=80=9Cvis=E2=80=9D should stand for =E2=80=9Cvisibility=E2=80=
=9D. Here =E2=80=9Cvis=E2=80=9D actually means =E2=80=9Cvisibility map =
bits=E2=80=9D. There is the other =E2=80=9Cvis=E2=80=9D in the last =
parameter=E2=80=99s name =E2=80=9Cdo_set_pd_vis=E2=80=9D where the =
=E2=80=9Cvis=E2=80=9D should be mean =E2=80=9CPD_ALL_VISIBLE=E2=80=9D =
bit. So the two =E2=80=9Cvis=E2=80=9D feels making things confusing.

How about rename the function to =E2=80=9Cheap_page_will_set_vm_bits=E2=80=
=9D, and rename the last parameter to =E2=80=9Cdo_set_all_visible=E2=80=9D=
?=20

2 - 0002
```
+ * Decide whether to set the visibility map bits for heap_blk, using
+ * information from PruneFreezeResult and all_visible_according_to_vm. =
This
+ * function does not actually set the VM bit or page-level hint,
+ * PD_ALL_VISIBLE.
+ *
+ * If it finds that the page-level visibility hint or VM is corrupted, =
it will
+ * fix them by clearing the VM bit and page hint. This does not need to =
be
+ * done in a critical section.
+ *
+ * Returns true if one or both VM bits should be set, along with the =
desired
+ * flags in *new_vmbits. Also indicates via do_set_pd_vis whether
+ * PD_ALL_VISIBLE should be set on the heap page.
+ */
```

This function comment mentions PD_ALL_VISIBLE twice, but never mentions =
ALL_FROZEN. So =E2=80=9CReturns true if one or both VM bits should be =
set=E2=80=9D fells unclear. How about rephrase like "Returns true if the =
all-visible and/or all-frozen VM bits should be set.=E2=80=9D

3 - 0002
```
+	/*
+	 * Now handle two potential corruption cases:
+	 *
+	 * These do not need to happen in a critical section and are not
+	 * WAL-logged.
+	 *
+	 * 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.
+	 */
+	else if (all_visible_according_to_vm && =
!PageIsAllVisible(heap_page) &&
+			 visibilitymap_get_status(relation, heap_blk, =
&vmbuffer) !=3D 0)
+	{
+		ereport(WARNING,
+				(errcode(ERRCODE_DATA_CORRUPTED),
+				 errmsg("page is not marked all-visible =
but visibility map bit is set in relation \"%s\" page %u",
+						=
RelationGetRelationName(relation), heap_blk)));
+
+		visibilitymap_clear(relation, heap_blk, vmbuffer,
+							=
VISIBILITYMAP_VALID_BITS);
+	}
```

Here in the comment and error message, I guess =E2=80=9Cvisibility map =
bit=E2=80=9D refers to =E2=80=9Call visible bit=E2=80=9D, can we be =
explicit?

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