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>
 <CABPTF7X1X810eT7hjRz9M69sjMv1Vui5gXQEGhqYXmkmywhHCQ@mail.gmail.com>
 <CAAKRu_ZcK+ez71W7j+QzMo0gSafTsbGA3TU-fVSptw7yh41BEQ@mail.gmail.com>
In-Reply-To: 
 <CAAKRu_ZcK+ez71W7j+QzMo0gSafTsbGA3TU-fVSptw7yh41BEQ@mail.gmail.com>
From: Kirill Reshke <reshkekirill@gmail.com>
Date: Sat, 20 Dec 2025 17:32:38 +0500
Message-ID: 
 <CALdSSPjvhGXihT_9f-GJabYU=_PjrFDUxYaURuTbfLyQM6TErg@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: Xuneng Zhou <xunengzhou@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>,
	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/CALdSSPjvhGXihT_9f-GJabYU%3D_PjrFDUxYaURuTbfLyQM6TErg%40mail.gmail.com>
Precedence: bulk

On Sat, 20 Dec 2025 at 02:10, Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> Attached v29 addresses some feedback and also corrects a small error
> with the assertion I had added in the previous version's 0009.
>
> On Thu, Dec 18, 2025 at 10:38=E2=80=AFPM Xuneng Zhou <xunengzhou@gmail.co=
m> wrote:
> >
> > I=E2=80=99ve done a basic review of patches 1 and 2. Here are some comm=
ents
> > 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
>
> Good catch! I've fixed this in attached v29.
>
> > 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?
>
> There are already assertions that will trip in various places -- most
> importantly in XLogRegisterBuffer(), which is the one that inspired
> this refactor.
>
> > The comment at lines:
> > > "The only scenario where it is not already dirty is if the VM was rem=
oved=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(=
), because it may WAL-log the buffer and XLogRegisterBuffer() requires it."
>
> I see your point about future refactors missing updating comments like
> this. But, I don't think we are going to refactor the code such that
> we can have PD_ALL_VISIBLE set without the VM bits set more often.
> Also, it is common practice in Postgres to describe very specific edge
> cases or odd scenarios in order to explain code that may seem
> confusing without the comment. It does risk that comment later
> becoming stale, but it is better that future developers understand why
> the code is there.
>
> That being said, I take your point that the comment is confusing. I
> have updated it in a different way.
>
> > > "Even if PD_ALL_VISIBLE is already set, we don't need to worry about =
unnecessarily dirtying the heap buffer, as it must be marked dirty before a=
dding it to the WAL chain. The only scenario where it is not already dirty =
is if 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=
 modify
> > the heap buffer=E2=80=9D/=E2=80=9Cno heap page modification=E2=80=9D mi=
ght 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.
>
> The point I'm trying to make is that we have to dirty the buffer even
> if we don't modify the page because of the XLOG sub-system
> requirements. And, it may seem like a waste to do that if not
> modifying the page, but the page will rarely be clean anyway. I've
> tried to make this more clear in attached v29.
>
> - Melanie


Hi! I checked v29-0009, about HeapTupleSatisfiesVacuumHorizon. Origins
of this code track down to fdf9e21196a6 which was committed as part of
[0], at which point
there was no HeapTupleSatisfiesVacuumHorizon function. I guess this is
the reason this optimization was not performed earlier.

I also think this patch is correct, because we do similar things for
HEAPTUPLE_DEAD & HEAPTUPLE_RECENTLY_DEAD, and
HeapTupleSatisfiesVacuumHorizon is just a proxy to
HeapTupleSatisfiesVacuumHorizon with only difference in DEAD VS
RECENTLY_DEAD handling.


Similar change could be done at heapam_scan_analyze_next_tuple

...
case HEAPTUPLE_DEAD:
case HEAPTUPLE_RECENTLY_DEAD:
/* Count dead and recently-dead rows */
*deadrows +=3D 1;
break;
...


[0] https://www.postgresql.org/message-id/CABOikdP0meGuXPPWuYrP%3DvDvoqUdsh=
F2xJAzZHWSKg03Rz_%2B9Q%40mail.gmail.com


--=20
Best regards,
Kirill Reshke