MIME-Version: 1.0
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>
In-Reply-To: 
 <CAAKRu_ayWLg=WDGZZfSPWf0KjPM8u=LBb0D6XaEWyx2_YFFwAQ@mail.gmail.com>
From: Xuneng Zhou <xunengzhou@gmail.com>
Date: Fri, 19 Dec 2025 11:38:24 +0800
Message-ID: 
 <CABPTF7X1X810eT7hjRz9M69sjMv1Vui5gXQEGhqYXmkmywhHCQ@mail.gmail.com>
Subject: Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM
 on-access)
To: Melanie Plageman <melanieplageman@gmail.com>
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>,
	Chao Li <li.evan.chao@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CABPTF7X1X810eT7hjRz9M69sjMv1Vui5gXQEGhqYXmkmywhHCQ%40mail.gmail.com>
Precedence: bulk

He Melanie,

Thanks for working on this.

On Wed, Dec 17, 2025 at 12:59=E2=80=AFAM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Wed, Dec 3, 2025 at 6:07=E2=80=AFPM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> >
> > If we're just talking about the renaming, looking at procarray.c, it
> > is full of the word "removable" because its functions were largely
> > used to examine and determine if everyone can see an xmax as committed
> > and thus if that tuple is removable from their perspective. But
> > nothing about the code that I can see means it has to be an xmax. We
> > could just as well use the functions to determine if everyone can see
> > an xmin as committed.
>
> In the attached v27, I've removed the commit that renamed functions in
> procarray.c. I've added a single wrapper GlobalVisTestXidNotRunning()
> that is used in my code where I am testing live tuples. I think you'll
> find that I've addressed all of your review comments now -- as I've
> also gotten rid of the confusing blk_known_av logic through a series
> of refactors.
>
> The one outstanding point is which commits should bump
> XLOG_PAGE_MAGIC. (also review of the reworked patches).
>
> - Melanie

I=E2=80=99ve done a basic review of patches 1 and 2. Here are some comments
which may be somewhat immature, as this is a fairly large change set
and I=E2=80=99m new to some parts of the code.

1) Potential stale old_vmbits after VM repair n v2

// Corruption check 1
if (!PageIsAllVisible(page) &&
(old_vmbits & VISIBILITYMAP_ALL_VISIBLE) !=3D 0)
{
visibilitymap_clear(...); // VM now cleared to 0
// but old_vmbits still holds ALL_VISIBLE
}

// ... later ...

if (!presult.all_visible)
return presult.ndeleted; // Not taken if presult.all_visible=3Dtrue

new_vmbits =3D VISIBILITYMAP_ALL_VISIBLE; // Want to set this

if (old_vmbits =3D=3D new_vmbits) // Stale old_vmbits=3DALL_VISIBLE,
new_vmbits=3DALL_VISIBLE
  return presult.ndeleted; // issue: early return

After corruption repair clears the VM, old_vmbits is stale. The early
return can fire unexpectedly, leaving the VM cleared when it should be
re-set. Should we reset old_vmbits =3D 0 after the visibilitymap_clear?

2) Add Assert(BufferIsDirty(buf))

Since the patch's core claim is "buffer must be dirty before WAL
registration", an assertion encodes this invariant. Should we add:

Assert(BufferIsValid(buf));
Assert(BufferIsDirty(buf));

right before the visibilitymap_set() call?

3) Comment about "only scenario"

The comment at lines:
> "The only scenario where it is not already dirty is if the VM was removed=
=E2=80=A6"

This phrasing could become misleading after future refactors. Can we
make it more direct like:

> "We must mark the heap buffer dirty before calling visibilitymap_set(), b=
ecause it may WAL-log the buffer and XLogRegisterBuffer() requires it."

4) Comment clarity

Current comment:

> "Even if PD_ALL_VISIBLE is already set, we don't need to worry about unne=
cessarily dirtying the heap buffer, as it must be marked dirty before addin=
g it to the WAL chain. The only scenario where it is not already dirty is i=
f the VM was removed..."

In this test we now call MarkBufferDirty() on the heap page even when
only setting the VM, so the comments claiming =E2=80=9Cdoes not need to mod=
ify
the heap buffer=E2=80=9D/=E2=80=9Cno heap page modification=E2=80=9D might =
be misleading. It
might be better to say the test doesn=E2=80=99t need to modify heap
tuples/page contents or doesn=E2=80=99t need to prune/freeze.

--
Best,
Xuneng