Re: Why clearing the VM doesn't require registering vm buffer in wal record - Mailing list pgsql-hackers

On Tue, Sep 22, 2026 at 2:54 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
> On 21 Sep 2026, Melanie Plageman wrote:
> > That wouldn't help in this case because it was already clear on the
> > primary.
>
> Yes, my suggestion was too vague. I meant a check during redo that
> would fail an assert-enabled buildfarm run and preserve enough state
> to investigate. A WARNING can go unnoticed in a passing TAP test, as
> David recently pointed out [0]. Could we make this fatal in assert
> builds, once we have a condition that excludes pages legitimately
> ahead of replay?

I don't think it makes sense to implement that here. There are no
other places in the code base where we error out in an assert build
and warn in a non-assert build. So, we'd be starting a new precedent.
And having it error out in non-assert builds for VM corruption will
probably make people mad.

I do think it makes sense to do something like what David is
suggesting in general, though.

One thing we could do here is make sure there is test coverage. I
committed a patch yesterday 5d84c76021c that changes verify_heapam()
to report corruption when PD_ALL_VISIBLE is clear and the VM is set.
We could add a test that uses pg_amcheck to detect this kind of
corruption. AI drafted one for me that used a lot of fancy perl that I
didn't yet evaluate, but I could look into it more.

While revisiting the warning I wrote in the patch you reviewed, I
realized I don't think it's the right thing to do. The warning as I
wrote it would warn whenever a WAL record is clearing the VM and the
primary's PD_ALL_VISIBLE was set and its visibility map was already
clear and the standby's visibility map is set. This doesn't seem right
because it will warn even if the standby doesn't actually have data
corruption. That is, if the standby has both PD_ALL_VISIBLE set and
visibility map set, it would still warn even though that is not
corruption. The warning is then basically about the primary being out
of sync with the standby. But there are many other combinations of the
primary and standby being out of sync (e.g. we try to set the
visibility map on the standby and it is already set). And, I don't
think we want to warn in all of these cases. Every combination of
PD_ALL_VISIBLE and VM set/clear differing between primary and standby
is a lot to warn on. And if you lost the whole VM on one node, for
example, you would get this warning for every page. And it's not
obvious why just this one case of divergence between primary and
standby is most important to warn on.

Instead, I suggest that, starting on master, we warn whenever there is
corruption on either node. On the primary it already prints a warning
when PD_ALL_VISIBLE is clear and the VM is set. Then it fixes it. We
should expand this on master and also wal-log it. We should also then
add a warning to the standby when it has PD_ALL_VISIBLE clear and the
VM set. Once we WAL-log fixing corruption on the primary, it should
only warn during recovery when the standby is corrupt and out of sync
with the primary. These changes are larger, so I am planning to only
propose it in a separate thread to master only. I would couple it with
hardening that makes it harder to get into these situations so the
warnings are less likely to be produced.

I plan to start a new thread with the patches proposed to master only
that adds a lot more corruption protection to the VM.

> In heap_xlog_vm_clear_unregistered(), couldn't the VM page already have
> a newer LSN? Consider a heap record R whose VM clear was a no-op,
> followed by VACUUM setting that bit at S. If the VM page at S reaches
> disk and crash recovery starts before R, the new helper would call this
> corruption and move the page LSN backwards to R. Unlike
> XLogReadBufferForRedo(), it does not check the page LSN.  I did not
> reproduce this, just seems possible.

The ironic thing about this is that in the patch version you reviewed,
this behavior leaves things in a more correct state than prior to it.
However, I know it's wrong to decrease the LSN. Also it is wrong to
stamp a page with an LSN after making changes that weren't registered
in that WAL record. That's why we don't set the page LSN when clearing
the VM when the VM block is not registered in backbranches.

Just for fun, I'll describe why, in this particular case, my patch you
reviewed leaves things in a more correct state, though. VM page LSN
starts at 0, replay R and it does nothing to it. Replay S and it
advances the LSN from 0 -> 3. Then the VM page is persisted. Then we
crash. We replay R. It clears the VM and sets the LSN from 3 -> 2. We
replay S. It sets the VM and advances the LSN from 2 -> 3. On versions
18 and lower after the crash, R will clear the VM and not set the LSN.
S will see the LSN is already 3 and not do anything so the VM ends up
clear. That's not wrong, but it does create divergent state on primary
and standby. (19 without this patch wouldn't clear the VM at all,
which is wrong).

That being said, attached v2 is the correct approach for 19 and master
(for now). It does not stamp the LSN on the page when clearing the VM
when the VM block was not registered.

> Could we retain the original VM bits and page LSN in the diagnostic?
> The WAL redo CONTEXT already identifies the record.

I've removed the warning for now, but in the new more invasive patch
set I plan for master which will warn, I can include more details like
these. We can do things on master like have visibilitymap_clear()
return the original bits instead of just a boolean. That will make it
much easier to include details like this in the log message.

> A page-LSN check
> also needs care: another heap block's bit can advance the same VM page's
> LSN without repairing the bit we are interested in.

Yea, we don't want to gate the VM clear on the page LSN. This made me
think about how the VM can end up in an inconsistent state temporarily
during crash recovery. So, when I do add the warning to master, I'll
make sure it is only when we've reached a consistent state (I think
you mention that in your earlier comment).

> Also, if we accept a possible torn page from this repair, what happens
> on the next recovery? The new reader uses RBM_NORMAL_NO_LOG, not the
> VM's usual RBM_ZERO_ON_ERROR, so a checksum failure could stop recovery
> before it gets to the repair.

Yea, I did this on purpose because since ed62d26caca, I didn't read
the VM with ZERO_ON_ERROR, which I thought was okay since we were
always registering the VM blocks in the wal record for both setting
and clearing.

However, this isn't a good idea when 17 and 18 have a fallback path
that lets us read the VM in recovery (when clearing it) in a way that
could result in a torn page. Then we've created a way for the VM to
get corrupt and made it error out reading it during recovery.

> For this repair, shouldn't we either use RBM_ZERO_ON_ERROR on every
> relevant recovery read path, or require an FPI for the VM page since
> the last checkpoint's redo point before allowing the modification?

Yea, I've thought about this a lot the last few days. On back branches
(and as a stop-gap in master), I think we have to use
RBM_ZERO_ON_ERROR everywhere and be okay with tearing pages when
reading VM pages during recovery.

On master, going forward, we should register the VM whenever
PD_ALL_VISIBLE is being cleared. And then we should not use
RBM_ZERO_ON_ERROR for setting or clearing. For setting, that means
adding a new mode that will extend the VM if the page doesn't exist
but error out if the page is corrupt (RBM_ZERO_ON_MISSING). Then we
can make the zero_damaged_pages GUC per fork. I have four or five
ideas for making master more robust to VM corruption. And there are
still some open questions, like what to do about tuple locking, but
that can be discussed in more depth there.

But I think as long as we have a code path that we know leads to torn
pages, we have to zero those pages on error when reading them.

> There are quite a few moving parts in VM, and I don't yet have a clear
> picture of how they all fit together. Sorry if some of these questions
> are a distraction. I have a handful of open tickets about VM corruption,
> and I hope we can track down and fix all possible issues.

I certainly think we can harden master by making some bigger changes.
I have a draft of the changes and hope to post by mid next week.

For now, I've attached four patches that are much narrower fixes and
are backpatchable:

v2-0001 is 19-only and fixes the divergent VM bits on the standby by
reading the VM when the VM block wasn't registered
master-v2-0001 is the stop-gap for master that does the same thing but
in a different way because it doesn't have a fake relcache entry

v2-0002 fixes VM clear to read the VM with RBM_ZERO_ON_ERROR. This
should apply master -> 17

v2-0003 is a fix for 19 and master for an issue I found while working
on this where the VM wasn't guaranteed to be dirty by the time we
logged setting it in heap_page_prune_and_freeze(). This can happen if
it was already set all-frozen and all-visible but it contained any
non-frozen tuples (i.e. VM is corrupt with incorrect all-frozen bit).
The VM set operation would be a no-op and no one would fix the corrupt
all-frozen bit. This corruption was never fixed, but in 19 the
vm-setting code was rearranged to expect that if we try and set the
VM, we will dirty it. For that to be true, we have to fix this
corruption. And, since the VM corruption detection function is new in
19, it is easiest to just backpatch it that far.

- Melanie