Re: Why clearing the VM doesn't require registering vm buffer in wal record

On Thu, Apr 30, 2026 at 5:44 PM Melanie Plageman
<melanieplageman(at)gmail(dot)com> wrote:
>
> Attached is a patch set to fix the issue based largely on the work you
> started on your branch. I attached the version targeting master/19
> which is prefixed with v1_PGMASTER and the version targeting 18,
> prefixed v1_PG18. The pg 18 changes aren't a straight cherry-pick to
> 17 (the earliest I'll backpatch because that was when incremental
> backup was introduced) because the redo functions live in a different
> file in 18 than in 17, but I want to avoid discussing three different
> versions of this patch set on this thread.
>
> The backpatched changes are different for a few reasons, but the
> biggest difference from a review standpoint is that in pg18, the redo
> routines can read WAL in the old format or the new format, so that
> people can reasonably upgrade to the new minor version.

There is an interaction between CREATE DATABASE STRATEGY WAL_LOG and
this fix that causes data corruption (identified while investigating
[1]). The scenario is you have a heap page set PD_ALL_VISIBLE and a
corresponding VM page set all-visible, you do a CREATE DATABASE
STRATEGY WAL_LOG using that database as a template. The primary has
the heap page set PDV (PD_ALL_VISIBLE) set and VM page AV set but the
standby does NOT have the page set all-visible in the VM (it does have
the heap page set PDV). After failover you have a primary with heap
page PDV set and VM page not set. You modify a tuple on the page.
Because the VM was already clear, we do not register the VM block in
the WAL record. The standby replays this and clears the heap page
PD_ALL_VISIBLE but leaves the VM set. This is data corruption leading
to wrong results from index scans and worse if that standby is later
promoted back to primary.

We didn't have this problem before ed62d26caca because it cleared the
VM during replay as long as PD_ALL_VISIBLE needed clearing. I mention
in the thread [2] that I think we have to fix
RelationCopyStorageUsingBuffer() to avoid producing this scenario
because tuple locking already caused data corruption before
ed62d26caca. However, for update/delete, etc, I'm wondering if we
should harden this a bit and go back to registering a vm clear
operation whenever PD_ALL_VISIBLE was set on the heap page. If this is
a no-op on the primary for the VM, we register it with
REBGUF_NO_CHANGE. Vacuum works very hard to never end up with
PD_ALL_VISIBLE cleared and the VM set. Perhaps we should do everything
we can to avoid this happening with page modifications as well.

It's a pretty small change except for annoyingly making
log_heap_update() even more complicated. Note that this does not fix
tuple locking since that would be prohibitively expensive as described
in [2].

AI-generated repro and proposed fix attached.

- Melanie

[1] https://www.postgresql.org/message-id/tencent_2E870046716FD94285045E96505A2D4E2908%40qq.com
[2] https://www.postgresql.org/message-id/flat/tencent_2E870046716FD94285045E96505A2D4E2908%40qq.com#2b10a62acd511839653ee32693538993