Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

Revamp the WAL record format.
- 2c03216d8311 9.5.0 cited
Allow I/O reliability checks using 16-bit checksums
- 96ef3b8ff1cf 9.3.0 cited
Make the visibility map crash-safe.
- 503c7305a1e3 9.2.0 cited

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

Melanie Plageman <melanieplageman@gmail.com> — 2026-03-05T19:56:13Z

Today Andres and I noticed that heap_{update,insert,delete}() don't
register the VM buffer when they are clearing the VM. I was under the
impression that any buffers modified needed to be registered in the
WAL record. Without which, you'll never do an FPI. It seems like this
could cause checksum failures. What are we missing?

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

Andres Freund <andres@anarazel.de> — 2026-03-05T20:16:28Z

Hi,

On 2026-03-05 14:56:13 -0500, Melanie Plageman wrote:
> Today Andres and I noticed that heap_{update,insert,delete}() don't
> register the VM buffer when they are clearing the VM. I was under the
> impression that any buffers modified needed to be registered in the
> WAL record. Without which, you'll never do an FPI. It seems like this
> could cause checksum failures. What are we missing?

Besides checksum errors, I was also worried that this would break
pg_rewind. But it seems we're saved by pg_rewind only parsing the WAL for
changes to the main fork.

But it does seem like it could be a problem for incremental backup /
walsummarizer?

Greetings,

Andres Freund

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

Matthias van de Meent <boekewurm+postgres@gmail.com> — 2026-03-05T20:23:12Z

On Thu, 5 Mar 2026 at 20:56, Melanie Plageman <melanieplageman@gmail.com> wrote:
>
> Today Andres and I noticed that heap_{update,insert,delete}() don't
> register the VM buffer when they are clearing the VM. I was under the
> impression that any buffers modified needed to be registered in the
> WAL record. Without which, you'll never do an FPI. It seems like this
> could cause checksum failures. What are we missing?

Might it be, because zeroing a VM page (which you would do when you
encounter checksum failures) is an MVCC-safe operation?

I agree with you that we probably _should_ register the VM (and
possibly FSM) buffer, but that's a bit of a different story. Right
now, the VM does not use the standard page format (nor does the FSM),
and therefore every FPI would be the full 8KB, even when just a few
bits of the VM page are in use; which would be a rather large waste of
space.

On Thu, 5 Mar 2026 at 21:16, Andres Freund <andres@anarazel.de> wrote:
>
> But it does seem like it could be a problem for incremental backup /
> walsummarizer?

I don't think it is, because that doesn't do calculations for non-main
forks, it considers those forks always changed and includes them in
full. Or at least, that was the response I got when I raised concerns
about the FSM back when the incremental backup feature was being
developed [0].

Kind regards,

Matthias van de Meent
Databricks (https://databricks.com)

[0] https://postgr.es/m/CA%2BTgmoaR8o%2BPBeWc_2Ge0XVgoM7xWKNyDmqXoTov%3DS6_J1gecQ%40mail.gmail.com

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

Andres Freund <andres@anarazel.de> — 2026-03-05T20:38:24Z

Hi,

On 2026-03-05 21:23:12 +0100, Matthias van de Meent wrote:
> On Thu, 5 Mar 2026 at 20:56, Melanie Plageman <melanieplageman@gmail.com> wrote:
> >
> > Today Andres and I noticed that heap_{update,insert,delete}() don't
> > register the VM buffer when they are clearing the VM. I was under the
> > impression that any buffers modified needed to be registered in the
> > WAL record. Without which, you'll never do an FPI. It seems like this
> > could cause checksum failures. What are we missing?
> 
> Might it be, because zeroing a VM page (which you would do when you
> encounter checksum failures) is an MVCC-safe operation?

We are afaict not automatically zeroing corrupted VM pages (we are doing that
for FSM though). IMO it's a bug if a user ever has to turn on
zero_damaged_pages outside of the storage actually corrupting data.

> I agree with you that we probably _should_ register the VM (and possibly
> FSM) buffer, but that's a bit of a different story. Right now, the VM does
> not use the standard page format (nor does the FSM), and therefore every FPI
> would be the full 8KB, even when just a few bits of the VM page are in use;
> which would be a rather large waste of space.

If checksums are enabled, we are already emitting FPIs for the VM when
*setting* bits in the VM (c.f. log_heap_visible()). I don't see why the story
for clearing it should be different.  And because there are so few VM pages
compared to heap pages, I wouldn't expect there to be a meaningful amount of
VM FPIs outside of very contrived workloads.

> On Thu, 5 Mar 2026 at 21:16, Andres Freund <andres@anarazel.de> wrote:
> >
> > But it does seem like it could be a problem for incremental backup /
> > walsummarizer?
> 
> I don't think it is, because that doesn't do calculations for non-main
> forks, it considers those forks always changed and includes them in
> full. Or at least, that was the response I got when I raised concerns
> about the FSM back when the incremental backup feature was being
> developed [0].

There's explicit code for ignoring the FSM, but I don't see the same for the
VM. And that makes sense: VM changes are mostly WAL logged, just not
completely / generically (i.e. this complaint), whereas FSM changes are not
WAL logged at all.

Greetings,

Andres Freund

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

Andres Freund <andres@anarazel.de> — 2026-03-05T21:01:07Z

Hi,

On 2026-03-05 15:38:24 -0500, Andres Freund wrote:
> > On Thu, 5 Mar 2026 at 21:16, Andres Freund <andres@anarazel.de> wrote:
> > >
> > > But it does seem like it could be a problem for incremental backup /
> > > walsummarizer?
> >
> > I don't think it is, because that doesn't do calculations for non-main
> > forks, it considers those forks always changed and includes them in
> > full. Or at least, that was the response I got when I raised concerns
> > about the FSM back when the incremental backup feature was being
> > developed [0].
>
> There's explicit code for ignoring the FSM, but I don't see the same for the
> VM. And that makes sense: VM changes are mostly WAL logged, just not
> completely / generically (i.e. this complaint), whereas FSM changes are not
> WAL logged at all.

Unfortunately I can confirm that incremental backups end up with an outdated
VM.

An unfortunate kicker: It looks like verify_heapam() doesn't even notice :(.


But the next vacuum does complain, if we end up processing the page for some
reason:

2026-03-05 15:56:23.018 EST [2073843][client backend][0/5:0][psql] WARNING:  page is not marked all-visible but visibility map bit is set in relation "s" page 0
2026-03-05 15:56:23.018 EST [2073843][client backend][0/5:0][psql] CONTEXT:  while scanning block 0 of relation "public.s"


Repro:

1) CREATE TABLE s(id int); INSERT INTO s DEFAULT VALUES; SELECT pg_current_wal_insert_lsn(); VACUUM s;
2) create full base backup
   rm -rf /tmp/backup-a && mkdir /tmp/backup-a && ./src/bin/pg_basebackup/pg_basebackup -c fast -D /tmp/backup-a

3) UPDATE s SET id = -id;
4) create incremental backup
   rm -rf /tmp/backup-a-i && mkdir /tmp/backup-a-i && ./src/bin/pg_basebackup/pg_basebackup --incremental=/tmp/backup-a/backup_manifest -c fast -D /tmp/backup-a-i

5) combine full and incremental backup
   rm -rf /tmp/backup-a-i-c && mkdir /tmp/backup-a-i-c && ./src/bin/pg_combinebackup/pg_combinebackup -d -o /tmp/backup-a-i-c --copy-file-range /tmp/backup-a /tmp/backup-a-i/

6) start PG on combined backup and use
   SELECT * FROM pg_visibility_map_summary('s');
   to see things are wrong

7) vacuum and see it complain (page is processed because we always look at the
   last page, and there's just one)


Greetings,

Andres Freund

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

Robert Haas <robertmhaas@gmail.com> — 2026-03-05T23:11:43Z

On Thu, Mar 5, 2026 at 3:38 PM Andres Freund <andres@anarazel.de> wrote:
> If checksums are enabled, we are already emitting FPIs for the VM when
> *setting* bits in the VM (c.f. log_heap_visible()). I don't see why the story
> for clearing it should be different.  And because there are so few VM pages
> compared to heap pages, I wouldn't expect there to be a meaningful amount of
> VM FPIs outside of very contrived workloads.

Yeah, that's how it seems to me, too, at least as of this moment.

Is there any indication in the code or comments that this was an
intentional omission because somebody thought we could get away
without doing it? Or is just a straight-up goof?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

Andres Freund <andres@anarazel.de> — 2026-03-06T00:43:51Z

Hi,

On 2026-03-05 18:11:43 -0500, Robert Haas wrote:
> On Thu, Mar 5, 2026 at 3:38 PM Andres Freund <andres@anarazel.de> wrote:
> > If checksums are enabled, we are already emitting FPIs for the VM when
> > *setting* bits in the VM (c.f. log_heap_visible()). I don't see why the story
> > for clearing it should be different.  And because there are so few VM pages
> > compared to heap pages, I wouldn't expect there to be a meaningful amount of
> > VM FPIs outside of very contrived workloads.
>
> Yeah, that's how it seems to me, too, at least as of this moment.
>
> Is there any indication in the code or comments that this was an
> intentional omission because somebody thought we could get away
> without doing it?

I couldn't find any evidence of that, which of course doesn't mean it doesn't
exist.

> Or is just a straight-up goof?

It kinda looks like a victim of multiple subsequent changes that each arguably
should have done something different:

commit 2c03216d831
Author: Heikki Linnakangas <heikki.linnakangas@iki.fi>
Date:   2014-11-20 17:56:26 +0200

    Revamp the WAL record format.

Which should have included a reference to the VM page in the WAL record for
insert/update/delete.

and

commit 96ef3b8ff1c
Author: Simon Riggs <simon@2ndQuadrant.com>
Date:   2013-03-22 13:54:07 +0000

    Allow I/O reliability checks using 16-bit checksums

Which should have triggered an FPI when clearing the VM, as afaict there
otherwise is no guarantee the checksum will be correct after a crash.

and, although I am not sure it's true:

commit 503c7305a1e
Author: Robert Haas <rhaas@postgresql.org>
Date:   2011-06-21 23:04:40 -0400

    Make the visibility map crash-safe.

Which perhaps also should have emitted an FPI when clearing a bit? But I'm
unsure that that was required at the time. OTOH, it did seem to generate an
FPI for setting a VM bit, so ...

Greetings,

Andres Freund

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

Robert Haas <robertmhaas@gmail.com> — 2026-03-06T16:49:20Z

On Thu, Mar 5, 2026 at 7:43 PM Andres Freund <andres@anarazel.de> wrote:
> Which perhaps also should have emitted an FPI when clearing a bit? But I'm
> unsure that that was required at the time. OTOH, it did seem to generate an
> FPI for setting a VM bit, so ...

After booting up the part of my brain that worked on this back in
2011, I think I reasoned that clearing a bit was idempotent and didn't
depend on the prior page state, so that we would be adequate protected
without an FPI. I had to add a WAL record to *set* the VM bit, because
that wasn't logged at all, but the actual of clearing the VM bit
piggybacked on the heap change that necessitated doing so, and having
that record also emit an FPI for the VM page didn't seem to me to add
anything.

So I think technically it's the addition of checksums that turns this
into a real bug, because now torn pages are a checksum-failure hazard.
However, that would have been extremely hard to notice given that I
seem never to have actually documented that reasoning in a comment
anywhere.

I don't think we should mess around with trying to make the behavior
here conditional on wal_level, checksums, etc. We should just do it
all the time to keep the logic simple.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

Yura Sokolov <y.sokolov@postgrespro.ru> — 2026-03-12T11:25:17Z

06.03.2026 00:01, Andres Freund пишет:
> Hi,
> 
> On 2026-03-05 15:38:24 -0500, Andres Freund wrote:
>>> On Thu, 5 Mar 2026 at 21:16, Andres Freund <andres@anarazel.de> wrote:
>>>>
>>>> But it does seem like it could be a problem for incremental backup /
>>>> walsummarizer?
>>>
>>> I don't think it is, because that doesn't do calculations for non-main
>>> forks, it considers those forks always changed and includes them in
>>> full. Or at least, that was the response I got when I raised concerns
>>> about the FSM back when the incremental backup feature was being
>>> developed [0].
>>
>> There's explicit code for ignoring the FSM, but I don't see the same for the
>> VM. And that makes sense: VM changes are mostly WAL logged, just not
>> completely / generically (i.e. this complaint), whereas FSM changes are not
>> WAL logged at all.
> 
> Unfortunately I can confirm that incremental backups end up with an outdated
> VM.

That is why pg_probackup still archive VM at whole in incremental (WAL
parsing) backup.
That is why WAL-G's incremental backup in WAL-parsing mode is (was?)
considered unstable.

I know the problem for couple of years. Excuse me I didn't write about.

I didn't recognize fix could be as simple as registering VM buffers.
My bad. I fill so stupid :-(

It would be great if it will be fixed in all supported versions.

-- 
regards
Yura Sokolov aka funny-falcon

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

Yura Sokolov <y.sokolov@postgrespro.ru> — 2026-03-12T12:22:35Z

12.03.2026 14:25, Yura Sokolov пишет:
> 06.03.2026 00:01, Andres Freund пишет:
>> Hi,
>>
>> On 2026-03-05 15:38:24 -0500, Andres Freund wrote:
>>>> On Thu, 5 Mar 2026 at 21:16, Andres Freund <andres@anarazel.de> wrote:
>>>>>
>>>>> But it does seem like it could be a problem for incremental backup /
>>>>> walsummarizer?
>>>>
>>>> I don't think it is, because that doesn't do calculations for non-main
>>>> forks, it considers those forks always changed and includes them in
>>>> full. Or at least, that was the response I got when I raised concerns
>>>> about the FSM back when the incremental backup feature was being
>>>> developed [0].
>>>
>>> There's explicit code for ignoring the FSM, but I don't see the same for the
>>> VM. And that makes sense: VM changes are mostly WAL logged, just not
>>> completely / generically (i.e. this complaint), whereas FSM changes are not
>>> WAL logged at all.
>>
>> Unfortunately I can confirm that incremental backups end up with an outdated
>> VM.
> 
> That is why pg_probackup still archive VM at whole in incremental (WAL
> parsing) backup.
> That is why WAL-G's incremental backup in WAL-parsing mode is (was?)
> considered unstable.
> 
> I know the problem for couple of years. Excuse me I didn't write about.
> 
> I didn't recognize fix could be as simple as registering VM buffers.
> My bad. I fill so stupid :-(
Should LSN properly set as well on vm page? To maximum of concurrent
updates, of course.

-- 
regards
Yura Sokolov aka funny-falcon

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

Andres Freund <andres@anarazel.de> — 2026-03-12T17:05:20Z

Hi,

On 2026-03-06 11:49:20 -0500, Robert Haas wrote:
> On Thu, Mar 5, 2026 at 7:43 PM Andres Freund <andres@anarazel.de> wrote:
> > Which perhaps also should have emitted an FPI when clearing a bit? But I'm
> > unsure that that was required at the time. OTOH, it did seem to generate an
> > FPI for setting a VM bit, so ...
> 
> After booting up the part of my brain that worked on this back in
> 2011, I think I reasoned that clearing a bit was idempotent and didn't
> depend on the prior page state, so that we would be adequate protected
> without an FPI. I had to add a WAL record to *set* the VM bit, because
> that wasn't logged at all, but the actual of clearing the VM bit
> piggybacked on the heap change that necessitated doing so, and having
> that record also emit an FPI for the VM page didn't seem to me to add
> anything.
> 
> So I think technically it's the addition of checksums that turns this
> into a real bug, because now torn pages are a checksum-failure hazard.
> However, that would have been extremely hard to notice given that I
> seem never to have actually documented that reasoning in a comment
> anywhere.

And then it got a lot worse with incremental backup support...

> I don't think we should mess around with trying to make the behavior
> here conditional on wal_level, checksums, etc. We should just do it
> all the time to keep the logic simple.

Agreed.

I've been working on a fix for this (still pretty raw though).  What a mess.

As part of validating that I added error checks that disallow reads in the
startup process for anything other than the FSM fork.  Which triggered, due to
visibilitymap_prepare_truncate() reading a VM page.

Which in turn made me wonder: Is it actually OK for
visibilitymap_prepare_truncate() to do separate WAL logging from
XLOG_SMGR_TRUNCATE?

I suspect not, consider this scenario:

1) primary does XLOG_FPI for the VM page
2) checkpoint
3) primary does XLOG_SMGR_TRUNCATE
4) standby replays XLOG_FPI
5) primary performs a restartpoint
6) primary replays XLOG_SMGR_TRUNCATE, as part of that it again does
   visibilitymap_prepare_truncate, which dirties the VM page
7) standby crashes while writing out the VM page
8) standby does recovery and now finds a torn VM page, triggering a checksum
   failure

Perhaps that's kinda ok, because vm_readbuf() uses ZERO_ON_ERROR, so we'd just
wipe out that region of the VM?

Separately, is it actually sufficient that visibilitymap_prepare_truncate()
only WAL logs the changes if XLogHintBitIsNeeded()?  I'm going back and forth
in my head about danger scenarios involving PITR or crashes inbetween
prepare_truncate() and the actual truncation.

ISTM that we should not do visibilitymap_prepare_truncate() during recovery
and always WAL log the prepare_truncate() on the primary.  Does that sound
sane?

Greetings,

Andres

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

Tomas Vondra <tomas@vondra.me> — 2026-03-12T19:27:32Z

On 3/12/26 18:05, Andres Freund wrote:
> Hi,
> 
> On 2026-03-06 11:49:20 -0500, Robert Haas wrote:
>> On Thu, Mar 5, 2026 at 7:43 PM Andres Freund <andres@anarazel.de> wrote:
>>> Which perhaps also should have emitted an FPI when clearing a bit? But I'm
>>> unsure that that was required at the time. OTOH, it did seem to generate an
>>> FPI for setting a VM bit, so ...
>>
>> After booting up the part of my brain that worked on this back in
>> 2011, I think I reasoned that clearing a bit was idempotent and didn't
>> depend on the prior page state, so that we would be adequate protected
>> without an FPI. I had to add a WAL record to *set* the VM bit, because
>> that wasn't logged at all, but the actual of clearing the VM bit
>> piggybacked on the heap change that necessitated doing so, and having
>> that record also emit an FPI for the VM page didn't seem to me to add
>> anything.
>>
>> So I think technically it's the addition of checksums that turns this
>> into a real bug, because now torn pages are a checksum-failure hazard.
>> However, that would have been extremely hard to notice given that I
>> seem never to have actually documented that reasoning in a comment
>> anywhere.
> 
> And then it got a lot worse with incremental backup support...
> 
> 
>> I don't think we should mess around with trying to make the behavior
>> here conditional on wal_level, checksums, etc. We should just do it
>> all the time to keep the logic simple.
> 
> Agreed.
> 
> 
> I've been working on a fix for this (still pretty raw though).  What a mess.
> 
> 
> As part of validating that I added error checks that disallow reads in the
> startup process for anything other than the FSM fork.  Which triggered, due to
> visibilitymap_prepare_truncate() reading a VM page.
> 
> Which in turn made me wonder: Is it actually OK for
> visibilitymap_prepare_truncate() to do separate WAL logging from
> XLOG_SMGR_TRUNCATE?
> 
> I suspect not, consider this scenario:
> 
> 1) primary does XLOG_FPI for the VM page
> 2) checkpoint
> 3) primary does XLOG_SMGR_TRUNCATE
> 4) standby replays XLOG_FPI
> 5) primary performs a restartpoint
> 6) primary replays XLOG_SMGR_TRUNCATE, as part of that it again does
>    visibilitymap_prepare_truncate, which dirties the VM page
> 7) standby crashes while writing out the VM page
> 8) standby does recovery and now finds a torn VM page, triggering a checksum
>    failure
> 
> Perhaps that's kinda ok, because vm_readbuf() uses ZERO_ON_ERROR, so we'd just
> wipe out that region of the VM?
> 

Shouldn't it be possible to test this using injection points?

> 
> Separately, is it actually sufficient that visibilitymap_prepare_truncate()
> only WAL logs the changes if XLogHintBitIsNeeded()?  I'm going back and forth
> in my head about danger scenarios involving PITR or crashes inbetween
> prepare_truncate() and the actual truncation.
> 
> 
> ISTM that we should not do visibilitymap_prepare_truncate() during recovery
> and always WAL log the prepare_truncate() on the primary.  Does that sound
> sane?
> 

I'm considering writing a stress test to try to shake out issues in
this. That strategy was pretty successful for the online checksums
patch, where we had a hunch it might not be quite correct- but it was
hard to come up with a reproducer. The stress test failures were very
useful in that they gave us proof of issues and some examples.

But the crucial part is an ability to verify correctness. With the
checksums it's easy enough - just verify checksums / look for checksum
failures in the server log. But what would that be here?

regards

-- 
Tomas Vondra

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

Andres Freund <andres@anarazel.de> — 2026-03-12T21:26:59Z

Hi,

On 2026-03-12 20:27:32 +0100, Tomas Vondra wrote:
> > I've been working on a fix for this (still pretty raw though).  What a mess.
> >
> >
> > As part of validating that I added error checks that disallow reads in the
> > startup process for anything other than the FSM fork.  Which triggered, due to
> > visibilitymap_prepare_truncate() reading a VM page.
> >
> > Which in turn made me wonder: Is it actually OK for
> > visibilitymap_prepare_truncate() to do separate WAL logging from
> > XLOG_SMGR_TRUNCATE?
> >
> > I suspect not, consider this scenario:
> >
> > 1) primary does XLOG_FPI for the VM page
> > 2) checkpoint
> > 3) primary does XLOG_SMGR_TRUNCATE
> > 4) standby replays XLOG_FPI
> > 5) primary performs a restartpoint
> > 6) primary replays XLOG_SMGR_TRUNCATE, as part of that it again does
> >    visibilitymap_prepare_truncate, which dirties the VM page
> > 7) standby crashes while writing out the VM page
> > 8) standby does recovery and now finds a torn VM page, triggering a checksum
> >    failure
> >
> > Perhaps that's kinda ok, because vm_readbuf() uses ZERO_ON_ERROR, so we'd just
> > wipe out that region of the VM?
> >
>
> Shouldn't it be possible to test this using injection points?

Not trivially, I think?  For one, you're not normally going to get a torn
page, even if you kill the server in the right moment, so you're rarely going
to be able to reproduce checksum errors without further infrastructure.  But
also, since the failure behaviour is going to be to clear the VM, you'd not
really see bad consequences, I think?

I think we really need some logic during WAL replay that does more
verification of pages that are read in (i.e. do not have an FPI applied) by
the startup process.

- Direct uses XLogReadBufferExtended() should only be allowed with
  ZERO_ON_ERROR, as it has no provisions for applying FPIs.

- Unless FPWs are disabled or ZERO_ON_ERROR is used:

   - the read in page must have an LSN newer than the last REDO LSN, otherwise
     there should have been an FPI

   - the page LSN may not have an LSN from the future (i.e BLK_DONE
     should never happen with FPWs)

- all FPIs in the record have been applied

A slightly different, but related, thing that we are missing is infrastructure
to detect pages that have been dirtied without associated WAL logging.  I
wonder if we could use a bit in the buffer descriptor to track if a page has
been dirtied by anything other than a hint bit write.  Or maybe we could
detect it when unlocking the page.

> > Separately, is it actually sufficient that visibilitymap_prepare_truncate()
> > only WAL logs the changes if XLogHintBitIsNeeded()?  I'm going back and forth
> > in my head about danger scenarios involving PITR or crashes inbetween
> > prepare_truncate() and the actual truncation.
> >
> >
> > ISTM that we should not do visibilitymap_prepare_truncate() during recovery
> > and always WAL log the prepare_truncate() on the primary.  Does that sound
> > sane?
> >
>
> I'm considering writing a stress test to try to shake out issues in
> this.

We could certainly use more of that.

> That strategy was pretty successful for the online checksums patch, where we
> had a hunch it might not be quite correct- but it was hard to come up with a
> reproducer. The stress test failures were very useful in that they gave us
> proof of issues and some examples.

> But the crucial part is an ability to verify correctness.

Unfortunately I don't think we have a particularly good infrastructure for
detecting problems right now :(

The verification ideas I posted above would, I think, help to detect some
issues, but I don't think they'd catch more complicated things.

We have wal_consistency_checking, but as it runs just after the redo routine,
it doesn't catch problems like not including things in the WAL record or
covering related actions in two WAL records that could be separated by a
checkpoint.  We really should have a tool that compares a primary and a
replica after doing recovery using the masking infrastructure from
wal_consistency_checking.

> With the checksums it's easy enough - just verify checksums / look for
> checksum failures in the server log. But what would that be here?

Unfortunately that's not even something you can really can rely on, it's
entirely possible to see checksum errors for the FSM without it being a bug,
as it's not WAL logged.

Greetings,

Andres Freund

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

Tomas Vondra <tomas@vondra.me> — 2026-03-12T21:54:25Z


On 3/12/26 22:26, Andres Freund wrote:
> Hi,
> 
> On 2026-03-12 20:27:32 +0100, Tomas Vondra wrote:
>>> I've been working on a fix for this (still pretty raw though).  What a mess.
>>>
>>>
>>> As part of validating that I added error checks that disallow reads in the
>>> startup process for anything other than the FSM fork.  Which triggered, due to
>>> visibilitymap_prepare_truncate() reading a VM page.
>>>
>>> Which in turn made me wonder: Is it actually OK for
>>> visibilitymap_prepare_truncate() to do separate WAL logging from
>>> XLOG_SMGR_TRUNCATE?
>>>
>>> I suspect not, consider this scenario:
>>>
>>> 1) primary does XLOG_FPI for the VM page
>>> 2) checkpoint
>>> 3) primary does XLOG_SMGR_TRUNCATE
>>> 4) standby replays XLOG_FPI
>>> 5) primary performs a restartpoint
>>> 6) primary replays XLOG_SMGR_TRUNCATE, as part of that it again does
>>>    visibilitymap_prepare_truncate, which dirties the VM page
>>> 7) standby crashes while writing out the VM page
>>> 8) standby does recovery and now finds a torn VM page, triggering a checksum
>>>    failure
>>>
>>> Perhaps that's kinda ok, because vm_readbuf() uses ZERO_ON_ERROR, so we'd just
>>> wipe out that region of the VM?
>>>
>>
>> Shouldn't it be possible to test this using injection points?
> 
> Not trivially, I think?  For one, you're not normally going to get a torn
> page, even if you kill the server in the right moment, so you're rarely going
> to be able to reproduce checksum errors without further infrastructure.  But
> also, since the failure behaviour is going to be to clear the VM, you'd not
> really see bad consequences, I think?
> 
> 
> I think we really need some logic during WAL replay that does more
> verification of pages that are read in (i.e. do not have an FPI applied) by
> the startup process.
> 
> - Direct uses XLogReadBufferExtended() should only be allowed with
>   ZERO_ON_ERROR, as it has no provisions for applying FPIs.
> 
> - Unless FPWs are disabled or ZERO_ON_ERROR is used:
> 
>    - the read in page must have an LSN newer than the last REDO LSN, otherwise
>      there should have been an FPI
> 
>    - the page LSN may not have an LSN from the future (i.e BLK_DONE
>      should never happen with FPWs)
> 
> - all FPIs in the record have been applied
> 
> 
> 
> A slightly different, but related, thing that we are missing is infrastructure
> to detect pages that have been dirtied without associated WAL logging.  I
> wonder if we could use a bit in the buffer descriptor to track if a page has
> been dirtied by anything other than a hint bit write.  Or maybe we could
> detect it when unlocking the page.
> 
> 
>>> Separately, is it actually sufficient that visibilitymap_prepare_truncate()
>>> only WAL logs the changes if XLogHintBitIsNeeded()?  I'm going back and forth
>>> in my head about danger scenarios involving PITR or crashes inbetween
>>> prepare_truncate() and the actual truncation.
>>>
>>>
>>> ISTM that we should not do visibilitymap_prepare_truncate() during recovery
>>> and always WAL log the prepare_truncate() on the primary.  Does that sound
>>> sane?
>>>
>>
>> I'm considering writing a stress test to try to shake out issues in
>> this.
> 
> We could certainly use more of that.
> 
> 
>> That strategy was pretty successful for the online checksums patch, where we
>> had a hunch it might not be quite correct- but it was hard to come up with a
>> reproducer. The stress test failures were very useful in that they gave us
>> proof of issues and some examples.
> 
> 
>> But the crucial part is an ability to verify correctness.
> 
> Unfortunately I don't think we have a particularly good infrastructure for
> detecting problems right now :(
> 
> 
> The verification ideas I posted above would, I think, help to detect some
> issues, but I don't think they'd catch more complicated things.
> 
> 
> We have wal_consistency_checking, but as it runs just after the redo routine,
> it doesn't catch problems like not including things in the WAL record or
> covering related actions in two WAL records that could be separated by a
> checkpoint.  We really should have a tool that compares a primary and a
> replica after doing recovery using the masking infrastructure from
> wal_consistency_checking.
> 

Silly idea - wouldn't it be possible to detect this particular issue
solely based on WAL? We could even have an off-line tool that reads WAL
and checks that we have all FPIs for all the changes.

> 
>> With the checksums it's easy enough - just verify checksums / look for
>> checksum failures in the server log. But what would that be here?
> 
> Unfortunately that's not even something you can really can rely on, it's
> entirely possible to see checksum errors for the FSM without it being a bug,
> as it's not WAL logged.
> 

I did not mean to imply that any checksum failure is necessarily an
outright bug. But I think this kind of issues being "normal" is a hint
maybe not WAL-logging FSM is not such a good design choice anymore.

regards

-- 
Tomas Vondra

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

Andres Freund <andres@anarazel.de> — 2026-03-12T23:34:37Z

Hi,

On 2026-03-12 22:54:25 +0100, Tomas Vondra wrote:
> >> But the crucial part is an ability to verify correctness.
> > 
> > Unfortunately I don't think we have a particularly good infrastructure for
> > detecting problems right now :(
> > 
> > 
> > The verification ideas I posted above would, I think, help to detect some
> > issues, but I don't think they'd catch more complicated things.
> > 
> > 
> > We have wal_consistency_checking, but as it runs just after the redo routine,
> > it doesn't catch problems like not including things in the WAL record or
> > covering related actions in two WAL records that could be separated by a
> > checkpoint.  We really should have a tool that compares a primary and a
> > replica after doing recovery using the masking infrastructure from
> > wal_consistency_checking.
> > 
> 
> Silly idea - wouldn't it be possible to detect this particular issue
> solely based on WAL? We could even have an off-line tool that reads WAL
> and checks that we have all FPIs for all the changes.

Afaict the problem is that you can't easily see that from the WAL in case of
bugs. E.g. for the case at hand, we didn't include block references for the
cleared VM pages in the WAL record. So such a tool would need to know about
the XLH_INSERT_ALL_VISIBLE_CLEARED flag etc.


> >> With the checksums it's easy enough - just verify checksums / look for
> >> checksum failures in the server log. But what would that be here?
> > 
> > Unfortunately that's not even something you can really can rely on, it's
> > entirely possible to see checksum errors for the FSM without it being a bug,
> > as it's not WAL logged.
> > 
> 
> I did not mean to imply that any checksum failure is necessarily an
> outright bug. But I think this kind of issues being "normal" is a hint
> maybe not WAL-logging FSM is not such a good design choice anymore.

I agree we'll eventually have to fix this, I just don't entirely know how.
Perhaps it'd be good enough to essentially treat the FSM as hint bits,
i.e. WAL log the first modification of an FSM page in a checkpoint, if
checksums/log-hints is on.

Greetings,

Andres Freund

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

Robert Haas <robertmhaas@gmail.com> — 2026-03-17T16:06:27Z

On Thu, Mar 12, 2026 at 1:05 PM Andres Freund <andres@anarazel.de> wrote:
> Which in turn made me wonder: Is it actually OK for
> visibilitymap_prepare_truncate() to do separate WAL logging from
> XLOG_SMGR_TRUNCATE?

I mean, I think this is just a blatant violation of the usual practice
around write-ahead logging. We're hoping that the change we're doing
here will be atomic with a later change in a separate critical
section. I don't see how that's ever going to work out. Functions with
"prepare" in the name should be limited to doing things that can be
done ahead of the operation for which they are preparing. The actual
operation needs to all happen in one critical section after all
preparation is complete.

> Separately, is it actually sufficient that visibilitymap_prepare_truncate()
> only WAL logs the changes if XLogHintBitIsNeeded()?  I'm going back and forth
> in my head about danger scenarios involving PITR or crashes inbetween
> prepare_truncate() and the actual truncation.

Since we don't set the page LSN, the changes made in that critical
section can make it to disk even if we don't manage to replay the
truncate record and fail to make it to disk even if we do. That can
only be OK if those changes are never critical. The only thing we're
doing here is clearing VM bits, so if that happens when it shouldn't,
nothing should break, except maybe synchronization between primaries
and standbys. But if it does happen when it shouldn't, that's
potentially data-corrupting: the relation can be re-extended, and now
you've got pages where the VM bit in the page is clear and the VM bit
in the map is set, which is never OK.

> ISTM that we should not do visibilitymap_prepare_truncate() during recovery
> and always WAL log the prepare_truncate() on the primary.  Does that sound
> sane?

I think the real need here is, um, to just following the regular
WAL-logging rules. All the things that need to happen as part of a
single operation need to be done in one critical section while holding
one set of buffer locks all at the same time, and the WAL record needs
to be written in that same critical section. Or alternatively several
operations that each follow these rules and together add up to the
same thing. For example, it would be fine to treat clearing the
tailing bits of the last partial page as a separate operation that
happens before the actual truncation and that therefore has its own
WAL record, buffer locks, and critical section, provided that you can
keep any of those bits from being set again before the actual
truncation happens, e.g. by hanging onto the buffer lock. During
replay, there can't be any concurrent writers, so replay can be fully
consecutive.

I feel like what's basically happened here is that an attempt was made
to hide too many of the details under the visibilitymap_* layer.
That's nice for abstraction, but it's not so great for correctness.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

Melanie Plageman <melanieplageman@gmail.com> — 2026-04-30T21:44:28Z

On Thu, Mar 5, 2026 at 4:01 PM Andres Freund <andres@anarazel.de> wrote:
>
> On 2026-03-05 15:38:24 -0500, Andres Freund wrote:
>
> > There's explicit code for ignoring the FSM, but I don't see the same for the
> > VM. And that makes sense: VM changes are mostly WAL logged, just not
> > completely / generically (i.e. this complaint), whereas FSM changes are not
> > WAL logged at all.
>
> Unfortunately I can confirm that incremental backups end up with an outdated
> VM.
>
> An unfortunate kicker: It looks like verify_heapam() doesn't even notice :(.

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.

I added a TAP test that does something like Andres' incremental backup
repro but tests more variations. Because init_from_backup() runs
pg_combinebackup with the --debug flag, the log output is very verbose
(i.e. every single copied file is logged). I suggest we turn it off by
default in tests. I included a patch that does that. It isn't required
to commit my test, but my test does produce 5000 lines of regression
log output which seems...not ideal.

The patch set targeting master includes a few more changes aimed at
catching bugs like this in the future. 0005 adds a check to
verify_heapam() that PD_ALL_VISIBLE is never clear when the VM is set.
0006 stops masking PD_ALL_VISIBLE during WAL consistency checking. And
0007 is a version of a patch Andres started which validates that every
block registered with a WAL record is read during recovery.

- Melanie

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

Robert Haas <robertmhaas@gmail.com> — 2026-05-01T16:41:49Z

On Thu, Apr 30, 2026 at 5:44 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> I added a TAP test that does something like Andres' incremental backup
> repro but tests more variations. Because init_from_backup() runs
> pg_combinebackup with the --debug flag, the log output is very verbose
> (i.e. every single copied file is logged). I suggest we turn it off by
> default in tests. I included a patch that does that. It isn't required
> to commit my test, but my test does produce 5000 lines of regression
> log output which seems...not ideal.

In my experience, it's pretty much impossible to debug a pg_basebackup
failure with incremental backup without the --debug option. Like,
you'll just have no idea where things went wrong. So I am cautious
about this, but I also understand that we run the tests a lot of times
on a lot of branches on a lot of machines, and so 5000 lines of output
that seems like no big deal in isolation may start to add up to a big
deal when you multiply it out.

So I feel like if your reasoning here is "this is using too many
resources and we can't afford it," that's fair enough, and we'll just
have to accept that if it starts failing we'll have to turn this back
on to have any idea what the problem is. If your reasoning is more
like "well I don't want all this output," I don't really think that's
a great reason -- if it ever fails, you will.

Another possible approach could be to direct the output to a temporary
file. At the end of the test run, if no tests failed, remove the file.
If there were any failures, copy that file's contents into the log.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

Andres Freund <andres@anarazel.de> — 2026-05-01T16:58:41Z

Hi,

On 2026-05-01 12:41:49 -0400, Robert Haas wrote:
> On Thu, Apr 30, 2026 at 5:44 PM Melanie Plageman
> <melanieplageman@gmail.com> wrote:
> > I added a TAP test that does something like Andres' incremental backup
> > repro but tests more variations. Because init_from_backup() runs
> > pg_combinebackup with the --debug flag, the log output is very verbose
> > (i.e. every single copied file is logged). I suggest we turn it off by
> > default in tests. I included a patch that does that. It isn't required
> > to commit my test, but my test does produce 5000 lines of regression
> > log output which seems...not ideal.
> 
> In my experience, it's pretty much impossible to debug a pg_basebackup
> failure with incremental backup without the --debug option. Like,
> you'll just have no idea where things went wrong. So I am cautious
> about this, but I also understand that we run the tests a lot of times
> on a lot of branches on a lot of machines, and so 5000 lines of output
> that seems like no big deal in isolation may start to add up to a big
> deal when you multiply it out.
> 
> So I feel like if your reasoning here is "this is using too many
> resources and we can't afford it," that's fair enough, and we'll just
> have to accept that if it starts failing we'll have to turn this back
> on to have any idea what the problem is. If your reasoning is more
> like "well I don't want all this output," I don't really think that's
> a great reason -- if it ever fails, you will.

Well, right now the regress_log* files that involve combining backups are so
long that it's hard to find the actual stuff that's happening - and it's worse
with the test Melanie is proposing because it runs a few iterations of
combining backups. So I think it's not universal that you'd want it.


> Another possible approach could be to direct the output to a temporary
> file. At the end of the test run, if no tests failed, remove the file.
> If there were any failures, copy that file's contents into the log.

I think that's probably the better direction.  I'd not even include it
directly into regress_log, just put it alongside it, like we have the server
logs etc.

Greetings,

Andres Freund

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

Robert Haas <robertmhaas@gmail.com> — 2026-05-01T17:30:38Z

On Fri, May 1, 2026 at 12:58 PM Andres Freund <andres@anarazel.de> wrote:
> Well, right now the regress_log* files that involve combining backups are so
> long that it's hard to find the actual stuff that's happening - and it's worse
> with the test Melanie is proposing because it runs a few iterations of
> combining backups. So I think it's not universal that you'd want it.

Fair enough.

> > Another possible approach could be to direct the output to a temporary
> > file. At the end of the test run, if no tests failed, remove the file.
> > If there were any failures, copy that file's contents into the log.
>
> I think that's probably the better direction.  I'd not even include it
> directly into regress_log, just put it alongside it, like we have the server
> logs etc.

Oh, yeah, OK, as long as CI will capture it, that WFM.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

Andres Freund <andres@anarazel.de> — 2026-05-01T19:14:48Z

Hi,

On 2026-04-30 17:44:28 -0400, Melanie Plageman wrote:
> On Thu, Mar 5, 2026 at 4:01 PM Andres Freund <andres@anarazel.de> wrote:
> >
> > On 2026-03-05 15:38:24 -0500, Andres Freund wrote:
> >
> > > There's explicit code for ignoring the FSM, but I don't see the same for the
> > > VM. And that makes sense: VM changes are mostly WAL logged, just not
> > > completely / generically (i.e. this complaint), whereas FSM changes are not
> > > WAL logged at all.
> >
> > Unfortunately I can confirm that incremental backups end up with an outdated
> > VM.
> >
> > An unfortunate kicker: It looks like verify_heapam() doesn't even notice :(.
> 
> 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.

Makes sense.

I think it's worth explicitly bringing up that this means that we leave an
arguable corruption case in the earlier branches.

I think it's the right call: The changes are non-trivial and there are
plenty-inter branch changes. And I can't come up with realistic scenarios
where it'd be a problem without something like incremental backup (as
pg_rewind just always copies the VM).

> I added a TAP test that does something like Andres' incremental backup
> repro but tests more variations. Because init_from_backup() runs
> pg_combinebackup with the --debug flag, the log output is very verbose
> (i.e. every single copied file is logged). I suggest we turn it off by
> default in tests. I included a patch that does that. It isn't required
> to commit my test, but my test does produce 5000 lines of regression
> log output which seems...not ideal.
> 
> The patch set targeting master includes a few more changes aimed at
> catching bugs like this in the future. 0005 adds a check to
> verify_heapam() that PD_ALL_VISIBLE is never clear when the VM is set.
> 0006 stops masking PD_ALL_VISIBLE during WAL consistency checking. And
> 0007 is a version of a patch Andres started which validates that every
> block registered with a WAL record is read during recovery.


I thought I'd start looking at the 18 patches.

> From 8ef81094e39a34d845b1a40cad7de469c8501bee Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Wed, 29 Apr 2026 12:53:49 -0400
> Subject: [PATCH v1_PG18 1/3] Fix WAL logging of VM clears
> 
> Previously, we failed to register the visibility map buffer when
> emitting WAL after clearing the VM bits for heap pages (recovery code
> read the VM page normally). This meant that we couldn't emit FPIs of VM
> pages when clearing VM bits. While this would not result in a checksum
> error because the visibility map is read with RBM_ZERO_ON_ERROR, WAL
> summarizer only includes pages which were registered, so a restore from
> an incremental backup would not include the modified VM pages -- leading
> to data corruption.
> 
> Fix this by registering the VM buffer in the WAL record when clearing VM
> bits. This also means the VM buffer must be locked throughout the
> duration of the critical section where we make changes to the VM and
> heap page and emit WAL.


> diff --git a/contrib/pg_surgery/heap_surgery.c b/contrib/pg_surgery/heap_surgery.c
> index 3e86283beb7..d7e4e051fff 100644
> --- a/contrib/pg_surgery/heap_surgery.c
> +++ b/contrib/pg_surgery/heap_surgery.c
> @@ -266,9 +266,10 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
>                */
>               if (PageIsAllVisible(page))
>               {
> +                 LockBuffer(vmbuf, BUFFER_LOCK_EXCLUSIVE);
>                   PageClearAllVisible(page);
> -                 visibilitymap_clear(rel, blkno, vmbuf,
> -                                     VISIBILITYMAP_VALID_BITS);
> +                 visibilitymap_clear_locked(rel, blkno, vmbuf,
> +                                            VISIBILITYMAP_VALID_BITS);
>                   did_modify_vm = true;
>               }
>           }
> @@ -331,7 +332,10 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
>  
>       UnlockReleaseBuffer(buf);
>  
> -     if (vmbuf != InvalidBuffer)
> +
> +     if (did_modify_vm)
> +         UnlockReleaseBuffer(vmbuf);
> +     else if (BufferIsValid(vmbuf))
>           ReleaseBuffer(vmbuf);
>  
>       /* Update the current_start_ptr before moving to the next page. */

Why did you touch heap_surgery in 18?  That shouldn't be necessary, right?


> @@ -2135,10 +2136,12 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
>   if (PageIsAllVisible(BufferGetPage(buffer)))
>   {
>       all_visible_cleared = true;
> +
> +     LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE);
>       PageClearAllVisible(BufferGetPage(buffer));
> -     visibilitymap_clear(relation,
> -                         ItemPointerGetBlockNumber(&(heaptup->t_self)),
> -                         vmbuffer, VISIBILITYMAP_VALID_BITS);
> +     visibilitymap_clear_locked(relation,
> +                                ItemPointerGetBlockNumber(&(heaptup->t_self)),
> +                                vmbuffer, VISIBILITYMAP_VALID_BITS);
>   }
>  
>   /*

It probably doesn't matter, but it seems like it'd be ever so slightly more
correct to first do visibilitymap_clear_locked() and then
PageClearAllVisible(), given
a) the rule that there should never be a VM bit set when !PD_ALL_VISIBLE
b) VM reads happening without page locks

I don't think it can ever lead to a wrong result, but still seems better to do
it the "right" way round.


> @@ -2228,13 +2231,22 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
>       /* filtering by origin on a row level is much more efficient */
>       XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
>  
> +     if (all_visible_cleared)
> +         XLogRegisterBuffer(1, vmbuffer, 0);
> +

For update you created HEAP_UPDATE_BLKREF_* to distinguish the different block
refernces.  I think that was a good call to make the code at least somewhat
understandable.  I think once we do it for heap_update(), we should just do it
for heap_{insert,multi_insert,delete,update} too.


> @@ -2503,10 +2515,11 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
>       if (PageIsAllVisible(page) && !(options & HEAP_INSERT_FROZEN))
>       {
>           all_visible_cleared = true;
> +         LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE);
>           PageClearAllVisible(page);
> -         visibilitymap_clear(relation,
> -                             BufferGetBlockNumber(buffer),
> -                             vmbuffer, VISIBILITYMAP_VALID_BITS);
> +         visibilitymap_clear_locked(relation,
> +                                    BufferGetBlockNumber(buffer),
> +                                    vmbuffer, VISIBILITYMAP_VALID_BITS);
>       }

Every time I see one of these new lock acquisitions I get worried about lock
ordering issues leading to deadlocks.  Wrongly here, because
visibilitymap_clear() would also have acquired the lock.

Am I blind, or have not documented the lock ordering rules for heap vs VM
anywhere?


> @@ -3133,13 +3154,23 @@ l1:
>       /* filtering by origin on a row level is much more efficient */
>       XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
>  
> +     if (all_visible_cleared)
> +         XLogRegisterBuffer(1, vmbuffer, 0);
> +
>       recptr = XLogInsert(RM_HEAP_ID, XLOG_HEAP_DELETE);

On the topic of complaining about things you have no responsibility for
whatsoever: It's so annoying that git picks up goto labels as diff "subjects"
(the "l1:" above) ...



> + /*
> +  * Clear PD_ALL_VISIBLE flags and reset visibility map bits for any heap
> +  * pages that were all-visible. If there are two heap pages, we may need
> +  * to clear VM bits for both.
> +  */
> + if (PageIsAllVisible(page) &&
> +     newbuf != buffer && PageIsAllVisible(newpage) &&
> +     vmbuffer_new == vmbuffer)
>   {
> +     /*
> +      * This is the more complicated case: both the new and old heap pages
> +      * are all-visible and both their VM bits are on the same page of the
> +      * VM, so we register a single VM buffer as HEAP_UPDATE_BLKREF_VM_NEW
> +      * in the WAL record. We must be careful to only lock and register one
> +      * buffer, even though we modify it twice -- once for each heap
> +      * block's VM bits.
> +      */

> - if (newbuf != buffer && PageIsAllVisible(BufferGetPage(newbuf)))
> + else
>   {
> -     all_visible_cleared_new = true;
> -     PageClearAllVisible(BufferGetPage(newbuf));
> -     visibilitymap_clear(relation, BufferGetBlockNumber(newbuf),
> -                         vmbuffer_new, VISIBILITYMAP_VALID_BITS);
> +     /*
> +      * In all the remaining cases, we will clear at most one heap block's
> +      * VM bits per VM page, so we don't have to worry about locking or
> +      * registering the same buffer twice.
> +      */
> +     if (PageIsAllVisible(page))
> +     {
> +         PageClearAllVisible(page);
> +         LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE);
> +         visibilitymap_clear_locked(relation, block,
> +                                    vmbuffer, VISIBILITYMAP_VALID_BITS);
> +         modified_vmbuffer = true;
> +         all_visible_cleared = true;
> +     }
> +     if (newbuf != buffer && PageIsAllVisible(newpage))
> +     {
> +         PageClearAllVisible(newpage);
> +         LockBuffer(vmbuffer_new, BUFFER_LOCK_EXCLUSIVE);
> +         visibilitymap_clear_locked(relation, BufferGetBlockNumber(newbuf),
> +                                    vmbuffer_new, VISIBILITYMAP_VALID_BITS);
> +         modified_vmbuffer_new = true;
> +         all_visible_cleared_new = true;
> +     }

I think here we actually have a lock ordering issue:

Assuming that we have four all-visible heap pages, Block HA1, HA2, which are
covered by VM1 and HB1 and HB2 which are covered by VM2.  You could get into
the unlucky situation that you have one backend B1 updating a tuple from HA1
and placing it on HB1, and a backend B2 updating a tuple on HB2 and updating
it to HA2.

In that case you could have the following lock acquisition order:

B1: lock HA1
B1: lock HB1
B2: lock HA2
B2: lock HB2
(hio.c will make sure to acquire the locks in ascending page order)

B1: lock VM1
B2: lock VM2
B1: lock VM2 <waits>
B2: lock VM1 <undetected deadlock>

It might not be possible to reach this, due to the newtupsize > pagefree case
clearing the VM bit.  But I think you could get unlucky and have it get set
again. For that I think the target page would have to be empty, otherwise we'd
not have set it as all-visible with a pin, but that seems possible.

Coming back later: The newtupsize > pagefree case doesn't even help, because
inexplicably we are just unsetting all frozen, even though we're about to
update the page! WHYYY!?!


So I think we need to be careful and acquire the exlusive locks on the two vm
pages in ascending block number order.  That'd turn the above into:

B1: lock VM1
B2: lock VM1 <waits>
B1: lock VM2
B1: xloginsert etc
B1: release locks
B2: lock VM1 <completes>
B2: lock VM2


We just need to do the lock acquisition that way, not the setting and clearing
of the bits. So it's perhaps going to not be *too* ugly.





> @@ -363,9 +363,24 @@ heap_xlog_delete(XLogReaderState *record)
>       Relation    reln = CreateFakeRelcacheEntry(target_locator);
>       Buffer      vmbuffer = InvalidBuffer;
>  
> -     visibilitymap_pin(reln, blkno, &vmbuffer);
> -     visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS);
> -     ReleaseBuffer(vmbuffer);
> +     if (XLogRecHasBlockRef(record, 1))
> +     {
> +         if (XLogReadBufferForRedo(record, 1, &vmbuffer) == BLK_NEEDS_REDO)
> +         {
> +             visibilitymap_clear_locked(reln,
> +                                        blkno, vmbuffer,
> +                                        VISIBILITYMAP_VALID_BITS);
> +             PageSetLSN(BufferGetPage(vmbuffer), lsn);
> +         }
> +         if (BufferIsValid(vmbuffer))
> +             UnlockReleaseBuffer(vmbuffer);
> +     }
> +     else
> +     {
> +         visibilitymap_pin(reln, blkno, &vmbuffer);
> +         visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS);
> +         ReleaseBuffer(vmbuffer);
> +     }
>       FreeFakeRelcacheEntry(reln);
>   }

I think we need a comment explaining why we need this if
(XLogRecHasBlockRef()) dance.  Otherwise whoever has to look at this for some
bug in the future will be rather confused.

Perhaps just have one comment, with the other places referring to it?  Might
also be sane to leave a breadcrumb on the WAL logging side, that it used to
look different in the same major version.



Wonder if we could package this into some kind of helper, there are a bunch of
copies of this now. But there's enough small differences that it's not
trivial.


>  
> @@ -691,6 +742,8 @@ heap_xlog_update(XLogReaderState *record, bool hot_update)
>   Buffer      obuffer,
>               nbuffer;
>   Page        page;
> + bool        new_cleared,
> +             old_cleared;
>   OffsetNumber offnum;
>   ItemId      lp = NULL;
>   HeapTupleData oldtup;
> @@ -728,14 +781,107 @@ heap_xlog_update(XLogReaderState *record, bool hot_update)


There's this comment nearby:

    /*
     * In normal operation, it is important to lock the two pages in
     * page-number order, to avoid possible deadlocks against other update
     * operations going the other way.  However, during WAL replay there can
     * be no other update happening, so we don't need to worry about that. But
     * we *do* need to worry that we don't expose an inconsistent state to Hot
     * Standby queries --- so the original page can't be unlocked before we've
     * added the new tuple to the new page.
     */

I think it's not wrong, but man, that seems like a fragile assumption.



>    * The visibility map may need to be fixed even if the heap page is
>    * already up-to-date.
>    */
> - if (xlrec->flags & XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED)
> + new_cleared = (xlrec->flags & XLH_UPDATE_NEW_ALL_VISIBLE_CLEARED) != 0;
> + old_cleared = (xlrec->flags & XLH_UPDATE_OLD_ALL_VISIBLE_CLEARED) != 0;
> + if (new_cleared || old_cleared)
>   {
>       Relation    reln = CreateFakeRelcacheEntry(rlocator);
> -     Buffer      vmbuffer = InvalidBuffer;
> +     bool        has_vm_old = XLogRecHasBlockRef(record, HEAP_UPDATE_BLKREF_VM_OLD);
> +     bool        has_vm_new = XLogRecHasBlockRef(record, HEAP_UPDATE_BLKREF_VM_NEW);
> +
> +     if (has_vm_new || has_vm_old)
> +     {
> +         /*
> +          * If both the old and new heap pages were all-visible and their
> +          * VM bits are on the same VM page, that single VM page is
> +          * registered as HEAP_UPDATE_BLKREF_VM_NEW. Clear both heap
> +          * blocks' VM bits from the single provided VM buffer.
> +          */
> +         bool        same_vm_page = new_cleared && old_cleared &&
> +             !has_vm_old && has_vm_new;
> +
> +         if (same_vm_page)
> +         {
> +             Buffer      vmbuffer = InvalidBuffer;
> +
> +             if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_NEW, &vmbuffer) ==
> +                 BLK_NEEDS_REDO)
> +             {
> +                 visibilitymap_clear_locked(reln, oldblk, vmbuffer,
> +                                            VISIBILITYMAP_VALID_BITS);
> +                 visibilitymap_clear_locked(reln, newblk, vmbuffer,
> +                                            VISIBILITYMAP_VALID_BITS);
> +                 PageSetLSN(BufferGetPage(vmbuffer), lsn);

> +             }
> +
> +             if (BufferIsValid(vmbuffer))
> +                 UnlockReleaseBuffer(vmbuffer);
> +         }
> +         else
> +         {
> +             /*
> +              * We can be sure that we need to clear at most one heap
> +              * page's VM bits in each registered VM buffer.
> +              */
> +             if (new_cleared)
> +             {
> +                 Buffer      vmbuffer = InvalidBuffer;
> +
> +                 if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_NEW, &vmbuffer) ==
> +                     BLK_NEEDS_REDO)
> +                 {
> +                     visibilitymap_clear_locked(reln, newblk, vmbuffer,
> +                                                VISIBILITYMAP_VALID_BITS);
> +                     PageSetLSN(BufferGetPage(vmbuffer), lsn);
> +                 }
> +
> +                 if (BufferIsValid(vmbuffer))
> +                     UnlockReleaseBuffer(vmbuffer);
> +             }
> +             if (old_cleared)
> +             {
> +                 Buffer      vmbuffer = InvalidBuffer;
> +
> +                 if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_OLD, &vmbuffer) ==
> +                     BLK_NEEDS_REDO)
> +                 {
> +                     visibilitymap_clear_locked(reln, oldblk, vmbuffer,
> +                                                VISIBILITYMAP_VALID_BITS);
> +                     PageSetLSN(BufferGetPage(vmbuffer), lsn);
> +                 }
> +
> +                 if (BufferIsValid(vmbuffer))
> +                     UnlockReleaseBuffer(vmbuffer);
> +             }
> +         }

Hm. Could this be simplified to something like:

		if (has_vm_new)
		{
			Buffer		vmbuffer = InvalidBuffer;

			if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_NEW, &vmbuffer) ==
				BLK_NEEDS_REDO)
			{
				if (new_cleared)
					visibilitymap_clear_locked(reln, newblk, vmbuffer,
											   VISIBILITYMAP_VALID_BITS);
				if (old_cleared && !has_vm_old)
					visibilitymap_clear_locked(reln, oldblk, vmbuffer,
											   VISIBILITYMAP_VALID_BITS);
				PageSetLSN(BufferGetPage(vmbuffer), lsn);
			}

			if (BufferIsValid(vmbuffer))
				UnlockReleaseBuffer(vmbuffer);
		}

		if (has_vm_old)
		{
			Buffer		vmbuffer = InvalidBuffer;

			Assert(old_cleared);

			if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_OLD, &vmbuffer) ==
				BLK_NEEDS_REDO)
			{
				visibilitymap_clear_locked(reln, oldblk, vmbuffer,
										   VISIBILITYMAP_VALID_BITS);
				PageSetLSN(BufferGetPage(vmbuffer), lsn);
			}

			if (BufferIsValid(vmbuffer))
				UnlockReleaseBuffer(vmbuffer);
		}

		if (!has_vm_new && !has_vm_old)
		{
...


Would it make sense to have an assert like

Assert(has_vm_new || !has_vm_old);  /* should never just have old VM buffer */


> +bool
> +visibilitymap_clear_locked(Relation rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags)

Given this is a new API, I wonder if we shouldn't already start using the new
signature. If there's any extensions using it (I think there is at least one),
this would just require them to have an ifdef for <17, 17-18, and >= 19.



> +# Set up primary with WAL summarization enabled.
> +my $primary = PostgreSQL::Test::Cluster->new('primary');
> +$primary->init(has_archiving => 1, allows_streaming => 1);
> +$primary->append_conf('postgresql.conf', <<EOF);
> +summarize_wal = on
> +autovacuum = off
> +EOF

A here doc with a closing paren is correct perl?  You learn something every
day.

Not particularly pretty imo, but whatever, it's perl.


> +##
> +## Test 1: INSERT clears all-visible and all-frozen bits
> +##
> +
> +$primary->safe_psql('postgres', q{VACUUM (FREEZE) vm_insert_test});
> +my ($pre_ins_vis, $pre_ins_frz) = get_vm_summary($primary, 'vm_insert_test');
> +cmp_ok($pre_ins_vis, '>', 0,
> + "INSERT test: pages are all-visible after vacuum");
> +cmp_ok($pre_ins_frz, '>', 0,
> + "INSERT test: pages are all-frozen after vacuum");
> +
> +my ($full1, $full1_name) = take_full_backup();
> +
> +$primary->safe_psql('postgres',
> + q{INSERT INTO vm_insert_test VALUES (1)});
> +
> +my ($post_ins_vis, $post_ins_frz) = get_vm_summary($primary, 'vm_insert_test');
> +cmp_ok($post_ins_vis, '<', $pre_ins_vis,
> + "INSERT test: all-visible cleared after insert");
> +cmp_ok($post_ins_frz, '<', $pre_ins_frz,
> + "INSERT test: all-frozen cleared after insert");
> +
> +my ($incr1, $incr1_name) = take_incr_backup($full1);
> +validate_restored_vm($full1, $full1_name, $incr1, $incr1_name,
> + 'vm_insert_test', 'INSERT test');
> +
> +##
> +## Test 2: DELETE clears all-visible and all-frozen bits
> +##
> +
> +$primary->safe_psql('postgres', q{VACUUM (FREEZE) vm_delete_test});
> +my ($pre_del_vis, $pre_del_frz) = get_vm_summary($primary, 'vm_delete_test');
> +cmp_ok($pre_del_vis, '>', 0,
> + "DELETE test: pages are all-visible after vacuum");
> +cmp_ok($pre_del_frz, '>', 0,
> + "DELETE test: pages are all-frozen after vacuum");
> +
> +my ($full2, $full2_name) = take_full_backup();
> +
> +$primary->safe_psql('postgres',
> + q{DELETE FROM vm_delete_test WHERE id = 1});
> +
> +my ($post_del_vis, $post_del_frz) = get_vm_summary($primary, 'vm_delete_test');
> +cmp_ok($post_del_vis, '<', $pre_del_vis,
> + "DELETE test: all-visible cleared after delete");
> +cmp_ok($post_del_frz, '<', $pre_del_frz,
> + "DELETE test: all-frozen cleared after delete");
> +
> +my ($incr2, $incr2_name) = take_incr_backup($full2);
> +validate_restored_vm($full2, $full2_name, $incr2, $incr2_name,
> + 'vm_delete_test', 'DELETE test');

Hm. Since these are looking at different tables, can't we do that with one
backup + incremental backup + combine + validate cycle for all of them?

Also seems like the modifications, tests and messages for all of them could be
moved to a helper?


I assume you verified that the tests catch the problem without the fix?



> From bb4c03a887f8ec5395ce37c09279a0786712b06c Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Wed, 29 Apr 2026 10:20:20 -0400
> Subject: [PATCH v1_PGMASTER 5/7] Add VM corruption check to verify_heapam()
> 
> pg_check_visible() can catch cases were tuples are not visible to all
> and the VM is set, but it doesn't specifically check page hints. This
> doesn't check, for example, an INSERT failing to clear PD_ALL_VISIBLE.
> 
> This also helps users who have access to amcheck but not pg_visibility
> to diagnose corruption.
> 
> Author: Melanie Plageman <melanieplageman@gmail.com>
> ---
>  contrib/amcheck/verify_heapam.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/contrib/amcheck/verify_heapam.c b/contrib/amcheck/verify_heapam.c
> index 20ff58aa782..0e023871566 100644
> --- a/contrib/amcheck/verify_heapam.c
> +++ b/contrib/amcheck/verify_heapam.c
> @@ -481,6 +481,7 @@ verify_heapam(PG_FUNCTION_ARGS)
>  
>   while ((ctx.buffer = read_stream_next_buffer(stream, NULL)) != InvalidBuffer)
>   {
> +     uint8       vmbits;
>       OffsetNumber maxoff;
>       OffsetNumber predecessor[MaxOffsetNumber];
>       OffsetNumber successor[MaxOffsetNumber];
> @@ -499,6 +500,21 @@ verify_heapam(PG_FUNCTION_ARGS)
>       ctx.blkno = BufferGetBlockNumber(ctx.buffer);
>       ctx.page = BufferGetPage(ctx.buffer);
>  
> +     /*
> +      * It is corruption of the page-level PD_ALL_VISIBLE flag is clear and
> +      * the visibility map bits corresponding to this heap page are set.
> +      */
> +     vmbits = visibilitymap_get_status(ctx.rel, ctx.blkno, &vmbuffer);

Maybe it's worth adding a comment explaining that visibilitymap_get_status()
won't fail, because the VM code uses ZERO_ON_ERROR and
visibilitymap_get_status() accepts there no being underlying page?

That'd basically address why this isn't meaningfully harming this goal:
 * This code goes to some trouble to avoid crashing the server even if the
 * table pages are badly corrupted, but it's probably not perfect.


> +     if (!PageIsAllVisible(ctx.page) &&
> +         (vmbits & VISIBILITYMAP_ALL_VISIBLE) != 0)
> +     {
> +         ctx.offnum = InvalidOffsetNumber;
> +         ctx.attnum = -1;
> +         report_corruption(&ctx,
> +                           psprintf("page is not marked all-visible in page header but visibility map bit is set"));

I'd just check for both bits, not just all visible.  Not that it's likely
you'd have just ALL_FROZEN set, but ...


> From 2be0dee6aa408752200ef9adfa9bf6a94716b4b2 Mon Sep 17 00:00:00 2001
> From: Melanie Plageman <melanieplageman@gmail.com>
> Date: Wed, 29 Apr 2026 10:33:46 -0400
> Subject: [PATCH v1_PGMASTER 6/7] Stop masking PD_ALL_VISIBLE when checking WAL
>  consistency
> 
> Setting and clearing PD_ALL_VISIBLE is WAL-logged, so there is no need
> to mask it for the purposes of WAL consistency checking.
> 
> There is one minor exception, which is that when fixing VM corruption,
> we clear PD_ALL_VISIBLE on the heap page without specifically emitting
> WAL. This is a situation where corruption has already happened, however,
> so it is okay if WAL consistency checking flags this as well.
> 
> Stop masking PD_ALL_VISIBLE, as doing so can only conceal real bugs.
> 
> Author: Melanie Plageman <melanieplageman@gmail.com>
> ---
>  src/backend/access/common/bufmask.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/src/backend/access/common/bufmask.c b/src/backend/access/common/bufmask.c
> index 5f63d04c9cb..93e34ea38b8 100644
> --- a/src/backend/access/common/bufmask.c
> +++ b/src/backend/access/common/bufmask.c
> @@ -53,12 +53,6 @@ mask_page_hint_bits(Page page)
>   /* Ignore PD_PAGE_FULL and PD_HAS_FREE_LINES flags, they are just hints. */
>   PageClearFull(page);
>   PageClearHasFreeLinePointers(page);
> -
> - /*
> -  * PD_ALL_VISIBLE is masked during WAL consistency checking. XXX: It is
> -  * worth investigating if we could stop doing this.
> -  */
> - PageClearAllVisible(page);

I'm still baffled we did this.  A real hear-no-evil-see-no-evil approach.



> diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
> index 8849610db00..63747aec566 100644
> --- a/src/backend/access/transam/xlogreader.c
> +++ b/src/backend/access/transam/xlogreader.c
> @@ -1794,6 +1794,10 @@ DecodeXLogRecord(XLogReaderState *state,
>  
>           blk = &decoded->blocks[block_id];
>           blk->in_use = true;
> +#ifdef USE_ASSERT_CHECKING
> +         blk->used_read = false;
> +#endif
> +
>           blk->apply_image = false;
>  
>           COPY_HEADER_FIELD(&fork_flags, sizeof(uint8));
> diff --git a/src/backend/access/transam/xlogrecovery.c b/src/backend/access/transam/xlogrecovery.c
> index c236e2b7969..4e1d50f642d 100644
> --- a/src/backend/access/transam/xlogrecovery.c
> +++ b/src/backend/access/transam/xlogrecovery.c
> @@ -1962,6 +1962,15 @@ ApplyWalRecord(XLogReaderState *xlogreader, XLogRecord *record, TimeLineID *repl
>   /* Now apply the WAL record itself */
>   GetRmgr(record->xl_rmid).rm_redo(xlogreader);
>  
> +#ifdef USE_ASSERT_CHECKING
> + for (int block_id = 0; block_id <= xlogreader->record->max_block_id; block_id++)
> + {
> +     DecodedBkpBlock *blk = &xlogreader->record->blocks[block_id];
> +
> +     Assert(!blk->in_use || blk->used_read);
> + }
> +#endif

Maybe deserves a comment like:

Verify that all block references were used during replay. This helps detect
bugs in redo routines. Every referenced block needs to be replayed with
XLogReadBufferForRedoExtended(), possibly via a helper, to ensure that we
replay FPIs, extend the relation if necessary and other similar protections.

Greetings,

Andres Freund

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

Melanie Plageman <melanieplageman@gmail.com> — 2026-05-06T21:46:49Z

Thanks for the review! I incorporated all your suggestions in attached
v2 unless otherwise noted below.

Based on an LLM-assisted self-review of this code, I think there is a
pre-existing bug in heap_lock_updated_tuple_rec() where we don't reset
cleared_all_frozen and may have a stale value if we iterate more than
once (new tuple versions) or use the l4 goto label (retries for the
same tuple version). This could mean we unnecessarily clear all-frozen
on the standby (and unnecessarily register the VM buffer after my
patch). I couldn't come up with a repro though, so I didn't include a
patch to fix it, as I wanted to confirm that someone else also thought
this is a bug.

On Fri, May 1, 2026 at 3:14 PM Andres Freund <andres@anarazel.de> wrote:
>
> On 2026-04-30 17:44:28 -0400, Melanie Plageman 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.
>
> Makes sense.
>
> I think it's worth explicitly bringing up that this means that we leave an
> arguable corruption case in the earlier branches.

But can there be such a case since we read the VM with ZERO_AND_LOCK?
Are there other cases that rely only on the buffers registered? Is it
possible it isn't reachable before 17?

> I thought I'd start looking at the 18 patches.

Attached v2_PG18 and v2_PG19 include all the updates from your
previous review applied to both branches.

> > @@ -331,7 +332,10 @@ heap_force_common(FunctionCallInfo fcinfo, HeapTupleForceOption heap_force_opt)
> >
> >       UnlockReleaseBuffer(buf);
> >
> > -     if (vmbuf != InvalidBuffer)
> > +
> > +     if (did_modify_vm)
> > +         UnlockReleaseBuffer(vmbuf);
> > +     else if (BufferIsValid(vmbuf))
> >           ReleaseBuffer(vmbuf);
> >
> >       /* Update the current_start_ptr before moving to the next page. */
>
> Why did you touch heap_surgery in 18?  That shouldn't be necessary, right?

It does visibilitymap_clear() and is also broken. Though, as you
pointed out off-list, my fix wasn't even far enough. It logs a WAL
record for the heap page and a separate one for the VM update which
isn't correct. v2 has a single WAL record with both registered.

> It probably doesn't matter, but it seems like it'd be ever so slightly more
> correct to first do visibilitymap_clear_locked() and then
> PageClearAllVisible(), given
> a) the rule that there should never be a VM bit set when !PD_ALL_VISIBLE
> b) VM reads happening without page locks
>
> I don't think it can ever lead to a wrong result, but still seems better to do
> it the "right" way round.

I've changed this throughout the patch set.

> > @@ -2228,13 +2231,22 @@ heap_insert(Relation relation, HeapTuple tup, CommandId cid,
> >       /* filtering by origin on a row level is much more efficient */
> >       XLogSetRecordFlags(XLOG_INCLUDE_ORIGIN);
> >
> > +     if (all_visible_cleared)
> > +         XLogRegisterBuffer(1, vmbuffer, 0);
> > +
>
> For update you created HEAP_UPDATE_BLKREF_* to distinguish the different block
> refernces.  I think that was a good call to make the code at least somewhat
> understandable.  I think once we do it for heap_update(), we should just do it
> for heap_{insert,multi_insert,delete,update} too.

I've done this in both versions as 0001.

> Am I blind, or have not documented the lock ordering rules for heap vs VM
> anywhere?

I don't see anywhere where something like this is documented. Though
I'm not entirely sure what you mean. In the cases with one VM block
and one heap block, you mean we should have documented that you should
acquire both locks before making changes to either page?

Thinking about this made me wonder if I should refactor the patches a
bit to acquire the vmbuffer lock before the critical section instead.
This would make the heap_insert() go from this

    START_CRIT_SECTION();

    RelationPutHeapTuple(relation, buffer, heaptup,
                         (options & HEAP_INSERT_SPECULATIVE) != 0);

    if (PageIsAllVisible(page))
    {
        all_visible_cleared = true;

        LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE);
        visibilitymap_clear(relation->rd_locator,
                            ItemPointerGetBlockNumber(&(heaptup->t_self)),
                            vmbuffer, VISIBILITYMAP_VALID_BITS,
                            InvalidXLogRecPtr);
        PageClearAllVisible(BufferGetPage(buffer));
    }

to something more like this:

    if (PageIsAllVisible(page))
    {
        clear_all_visible = true;
        LockBuffer(vmbuffer, BUFFER_LOCK_EXCLUSIVE);
    }

    START_CRIT_SECTION();

    RelationPutHeapTuple(relation, buffer, heaptup,
                         (options & HEAP_INSERT_SPECULATIVE) != 0);

    if (clear_all_visible)
    {
        visibilitymap_clear(relation->rd_locator,
                            ItemPointerGetBlockNumber(&(heaptup->t_self)),
                            vmbuffer, VISIBILITYMAP_VALID_BITS,
                            InvalidXLogRecPtr);
        PageClearAllVisible(BufferGetPage(buffer));
    }

> I think here we actually have a lock ordering issue:
<-- snip -->
> So I think we need to be careful and acquire the exlusive locks on the two vm
> pages in ascending block number order.  That'd turn the above into:
>
> B1: lock VM1
> B2: lock VM1 <waits>
> B1: lock VM2
> B1: xloginsert etc
> B1: release locks
> B2: lock VM1 <completes>
> B2: lock VM2

Right, good catch. I'd not thought of that at all. I've done this in
attached v2.

> > @@ -363,9 +363,24 @@ heap_xlog_delete(XLogReaderState *record)
> >       Relation    reln = CreateFakeRelcacheEntry(target_locator);
> >       Buffer      vmbuffer = InvalidBuffer;
> >
> > -     visibilitymap_pin(reln, blkno, &vmbuffer);
> > -     visibilitymap_clear(reln, blkno, vmbuffer, VISIBILITYMAP_VALID_BITS);
> > -     ReleaseBuffer(vmbuffer);
> > +     if (XLogRecHasBlockRef(record, 1))

> I think we need a comment explaining why we need this if
> (XLogRecHasBlockRef()) dance.  Otherwise whoever has to look at this for some
> bug in the future will be rather confused.

I've added a comment by the new helper function that I added.

> Perhaps just have one comment, with the other places referring to it?  Might
> also be sane to leave a breadcrumb on the WAL logging side, that it used to
> look different in the same major version.

What do you mean by the WAL logging side? This already is the redo
routine. Or do you mean something else by breadcrumb?

> Wonder if we could package this into some kind of helper, there are a bunch of
> copies of this now. But there's enough small differences that it's not
> trivial.

I did this.

Just to confirm, we do _not_ want to touch XLOG_PAGE_MAGIC on the back
branches because it specifically doesn't change the WAL format
compatibility.

> Hm. Could this be simplified to something like:
>
>                 if (has_vm_new)
>                 {
>                         Buffer          vmbuffer = InvalidBuffer;
>
>                         if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_NEW, &vmbuffer) ==
>                                 BLK_NEEDS_REDO)
>                         {
>                                 if (new_cleared)
>                                         visibilitymap_clear_locked(reln, newblk, vmbuffer,
>                                                                                            VISIBILITYMAP_VALID_BITS);
>                                 if (old_cleared && !has_vm_old)
>                                         visibilitymap_clear_locked(reln, oldblk, vmbuffer,
>                                                                                            VISIBILITYMAP_VALID_BITS);
>                                 PageSetLSN(BufferGetPage(vmbuffer), lsn);
>                         }
>
>                         if (BufferIsValid(vmbuffer))
>                                 UnlockReleaseBuffer(vmbuffer);
>                 }
>
>                 if (has_vm_old)
>                 {
>                         Buffer          vmbuffer = InvalidBuffer;
>
>                         Assert(old_cleared);
>
>                         if (XLogReadBufferForRedo(record, HEAP_UPDATE_BLKREF_VM_OLD, &vmbuffer) ==
>                                 BLK_NEEDS_REDO)
>                         {
>                                 visibilitymap_clear_locked(reln, oldblk, vmbuffer,
>                                                                                    VISIBILITYMAP_VALID_BITS);
>                                 PageSetLSN(BufferGetPage(vmbuffer), lsn);
>                         }
>
>                         if (BufferIsValid(vmbuffer))
>                                 UnlockReleaseBuffer(vmbuffer);
>                 }
>
>                 if (!has_vm_new && !has_vm_old)
>                 {

I tried this ordering. Personally, I find my current way more clear.
Then the different cases are self-contained. If both the vmbits for
two heap pages are set on the same VM page, you can understand what is
registered without looking across multiple if statements.

I also prefer to have the same structure in replay as in normal
operation. And in normal operation, because of the page lock ordering
concerns, the single VM page vs two VM page cases are even more
different.

> Would it make sense to have an assert like
>
> Assert(has_vm_new || !has_vm_old);  /* should never just have old VM buffer */

It is okay to have just VM old if only the VM bits on the old page are
set (because there are two VM buffers but the new one is not
all-visible).

> > +bool
> > +visibilitymap_clear_locked(Relation rel, BlockNumber heapBlk, Buffer vmbuf, uint8 flags)
>
> Given this is a new API, I wonder if we shouldn't already start using the new
> signature. If there's any extensions using it (I think there is at least one),
> this would just require them to have an ifdef for <17, 17-18, and >= 19.

So in 19, I just changed the function signature of
visibilitymap_clear() a bit and did not add a
visibilitymap_clear_locked(). This was doable because I felt more
comfortable changing 19 to not take a Relation and instead take a
RelFileLocator as a parameter because visibilitymap_set() already does
this on 19.

I am wondering in 19 if we should just set the page LSN in the call
sites and not try to have visibilitymap_clear() take an LSN. You had a
separate visibilitymap_clear_redo() function that took an LSN. I guess
I could make a separate function that just calls visibilitymap_clear()
and then does PageSetLSN(), but that seemed kind of silly. What do you
think?

Separately, in 18, I say visibilitymap_clear() is there for backwards
compatibility but then I use it in vacuumlazy.c where we clear the VM
without WAL logging when it is corrupted. Should I not use it there
because it is supposed ot be only for backwards compatibility? Or does
it not matter...

> > +my ($post_del_vis, $post_del_frz) = get_vm_summary($primary, 'vm_delete_test');
> > +cmp_ok($post_del_vis, '<', $pre_del_vis,
> > + "DELETE test: all-visible cleared after delete");
> > +cmp_ok($post_del_frz, '<', $pre_del_frz,
> > + "DELETE test: all-frozen cleared after delete");
> > +
> > +my ($incr2, $incr2_name) = take_incr_backup($full2);
> > +validate_restored_vm($full2, $full2_name, $incr2, $incr2_name,
> > + 'vm_delete_test', 'DELETE test');
>
> Hm. Since these are looking at different tables, can't we do that with one
> backup + incremental backup + combine + validate cycle for all of them?

I've done this

> Also seems like the modifications, tests and messages for all of them could be
> moved to a helper?

Also did this.

> I assume you verified that the tests catch the problem without the fix?

Yes, though the update case was doing lock first and thus not
exercising exactly what I intended. I think it is fixed now.

The Cluster.pm changes that pipes the pg_combinebackup output to a
temp file and then prints that to the regress log if a test fails
still need review. I had some AI-assistance writing it, and I'm pretty
sure it behaves as I intended but I don't know if it is idiomatic perl
or how we do things or whatever.

> > @@ -499,6 +500,21 @@ verify_heapam(PG_FUNCTION_ARGS)
> >       ctx.blkno = BufferGetBlockNumber(ctx.buffer);
> >       ctx.page = BufferGetPage(ctx.buffer);
> >
> > +     /*
> > +      * It is corruption of the page-level PD_ALL_VISIBLE flag is clear and
> > +      * the visibility map bits corresponding to this heap page are set.
> > +      */
> > +     vmbits = visibilitymap_get_status(ctx.rel, ctx.blkno, &vmbuffer);
>
> Maybe it's worth adding a comment explaining that visibilitymap_get_status()
> won't fail, because the VM code uses ZERO_ON_ERROR and
> visibilitymap_get_status() accepts there no being underlying page?
>
> That'd basically address why this isn't meaningfully harming this goal:
>  * This code goes to some trouble to avoid crashing the server even if the
>  * table pages are badly corrupted, but it's probably not perfect.

Well, I wouldn't necessarily expect visibilitymap_get_status() to fail
because of PD_ALL_VISIBLE being clear on the heap pages. And
ZERO_ON_ERROR isn't why that doesn't happen. But I think your point is
we should explain to the reader that visibilitymap_get_status() itself
won't assert/ERROR out when called on either a corrupted heap page or
corrupted VM page. Or is that not what you are saying?

- Melanie

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

Tomas Vondra <tomas@vondra.me> — 2026-05-07T12:54:18Z

On 5/6/26 23:46, Melanie Plageman wrote:
> Thanks for the review! I incorporated all your suggestions in attached
> v2 unless otherwise noted below.
> 
> Based on an LLM-assisted self-review of this code, I think there is a
> pre-existing bug in heap_lock_updated_tuple_rec() where we don't reset
> cleared_all_frozen and may have a stale value if we iterate more than
> once (new tuple versions) or use the l4 goto label (retries for the
> same tuple version). This could mean we unnecessarily clear all-frozen
> on the standby (and unnecessarily register the VM buffer after my
> patch). I couldn't come up with a repro though, so I didn't include a
> patch to fix it, as I wanted to confirm that someone else also thought
> this is a bug.
> 

Hmm, would this be a correctness or efficiency issue?

I'm not very familiar with this code, so I can't say if there is an
issue or not. Could you explain the sequence of steps leading to an
issue? Maybe that assumes something that's not quite possible, or it
might help constructing a reproducer.

I don't quite see how could it be related to the "l4" goto label, when
all the "goto l4" cases are before setting "cleared_all_frozen = true"
(in any given iteration).

regards

-- 
Tomas Vondra

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

Melanie Plageman <melanieplageman@gmail.com> — 2026-06-23T23:55:21Z

On Wed, May 6, 2026 at 5:46 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> I don't see anywhere where something like this is documented. Though
> I'm not entirely sure what you mean. In the cases with one VM block
> and one heap block, you mean we should have documented that you should
> acquire both locks before making changes to either page?
>
> Thinking about this made me wonder if I should refactor the patches a
> bit to acquire the vmbuffer lock before the critical section instead.

I've done this in attached v3. I've only included proposed changes to
18, as this ended up being a pretty invasive refactor that I'd like to
get feedback on first.

While working on v3, some new tests that were added (by another
commit) exposed a bug in this patch. Because it's possible and not
incorrect for the VM bits to already be clear in the VM while
PD_ALL_VISIBLE is set on the heap page and because
visibilitymap_clear() only marks the VM buffer dirty if we actually
had to clear the bits, we can end up needing to clear PD_ALL_VISIBLE
but not dirtying the VM buffer. This basically worked before we
started registering the VM buffer in the WAL record, but now that we
do that, we have to be sure we only do so if the buffer has been
modified (because the WAL machinery asserts that the registered buffer
is dirty). Tests passed before, so we must not have had coverage of
this scenario.

Unfortunately that requirement makes an already complicated patch more
complicated -- especially heap_update(). Fixing that for heap_update()
necessitated several new variables to represent the various states.
It also creates several complicated scenarios.

For example, it is possible that though both new and old pages need
PD_ALL_VISIBLE cleared and each of their VM bits were on different
pages but the old VM buffer is omitted because it was already clear.
This was previously indistinguishable from when old and new pages VM
bits are on the same VM buffer. I think this is fixed by checking if
old page's VM bits are on vmbuffer_new...but wowza, it's all pretty
complicated now.

- Melanie

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

Melanie Plageman <melanieplageman@gmail.com> — 2026-06-24T00:01:46Z

On Thu, May 7, 2026 at 8:54 AM Tomas Vondra <tomas@vondra.me> wrote:
>
> On 5/6/26 23:46, Melanie Plageman wrote:
>
> > Based on an LLM-assisted self-review of this code, I think there is a
> > pre-existing bug in heap_lock_updated_tuple_rec() where we don't reset
> > cleared_all_frozen and may have a stale value if we iterate more than
> > once (new tuple versions) or use the l4 goto label (retries for the
> > same tuple version). This could mean we unnecessarily clear all-frozen
> > on the standby (and unnecessarily register the VM buffer after my
> > patch). I couldn't come up with a repro though, so I didn't include a
> > patch to fix it, as I wanted to confirm that someone else also thought
> > this is a bug.
>
> Hmm, would this be a correctness or efficiency issue?
>
> I'm not very familiar with this code, so I can't say if there is an
> issue or not. Could you explain the sequence of steps leading to an
> issue? Maybe that assumes something that's not quite possible, or it
> might help constructing a reproducer.
>
> I don't quite see how could it be related to the "l4" goto label, when
> all the "goto l4" cases are before setting "cleared_all_frozen = true"
> (in any given iteration).

You're right that it couldn't happen with goto l4. However, it is
possible in the loop when walking an update chain, it can set
cleared_all_frozen to true, clear the page's all frozen bits and then
later, on the next iteration, clear it again (even though it's already
clear). It's only an efficiency issue in master, actually (as you
suggest). In my patch it would be a serious bug because it would try
to register the VM page and set an LSN on that VM page when it isn't
locked or marked dirty.

- Melanie

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

Robert Haas <robertmhaas@gmail.com> — 2026-06-26T20:05:55Z

On Tue, Jun 23, 2026 at 7:55 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:
> I've done this in attached v3. I've only included proposed changes to
> 18, as this ended up being a pretty invasive refactor that I'd like to
> get feedback on first.

I'm finding that these patches don't apply cleanly to the latest sources.

[robert.haas pgsql]$ git log -n1 --oneline
4df5fe3833a (HEAD -> master, origin/master, origin/HEAD) Make
crosstabview honor boolean/null display settings
[robert.haas pgsql]$ patch -p1 <
~/Downloads/v3-0001-Alias-WAL-block-references-for-some-record-types.patch
patching file 'src/backend/access/heap/heapam.c'
1 out of 10 hunks failed--saving rejects to
'src/backend/access/heap/heapam.c.rej'
patching file 'src/backend/access/heap/heapam_xlog.c'
1 out of 16 hunks failed--saving rejects to
'src/backend/access/heap/heapam_xlog.c.rej'
patching file 'src/include/access/heapam_xlog.h'

Apparently these patches are based on an old commit, or you have some
other local patches. Can you specify a commit to apply them to, and/or
rebase?

-- 
Robert Haas
EDB: http://www.enterprisedb.com

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

Andrey Borodin <x4mmm@yandex-team.ru> — 2026-06-27T10:41:25Z

> On 27 Jun 2026, at 01:05, Robert Haas <robertmhaas@gmail.com> wrote:
> 
> Apparently these patches are based on an old commit, or you have some
> other local patches. Can you specify a commit to apply them to, and/or
> rebase?

v3 set applies cleanly on REL_18_STABLE. +1 on backpatching this. I've done some
mechanical checks on the 18 variant: the new 012_vm_consistency test is green
(and goes red without the fix), and minor-version WAL compatibility holds:
WAL replays both ways between patched and unpatched 18.4 (patched replays
old-format records via the fallback path; unpatched harmlessly ignores the added
VM block refs).

Did I get it right that we will not have this

+ * Backwards compatibility path. Previously, the VM buffers were

on v19?

Best regards, Andrey Borodin.

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

Andrey Borodin <x4mmm@yandex-team.ru> — 2026-06-29T07:46:57Z


> On 27 Jun 2026, at 15:41, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> 
> I've done some
> mechanical checks on the 18 variant

A couple of minor things:

 - The new t/012_vm_consistency.pl needs pg_visibility, but
src/bin/pg_combinebackup/Makefile doesn't declare
EXTRA_INSTALL=contrib/pg_visibility, so a standalone
"make -C src/bin/pg_combinebackup check" fails with "extension pg_visibility
is not available". (meson test and make check-world are fine, since they install
the whole tree) (for example see how src/bin/pg_amcheck pulls amcheck/pageinspect)

 - A tiny wording nit: the comment on the retained visibilitymap_clear()
says it's kept "for backwards compatibility". Maybe spell out that it's kept for
ABI(prototype stability) on the back branches?


Best regards, Andrey Borodin.

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

Robert Haas <robertmhaas@gmail.com> — 2026-06-29T14:42:46Z

On Sat, Jun 27, 2026 at 6:41 AM Andrey Borodin <x4mmm@yandex-team.ru> wrote:
> v3 set applies cleanly on REL_18_STABLE.

Oh, I didn't realize this was based on the back-branch code. Thanks.

I have reviewed 0001 and 0002 and in general I think this looks good.
There is possibly room for some concern about performance regressions
with 0002 since, as you point out in some of the added comments, we
will now keep the VM buffer locked for longer. It might be worth
benchmarking that to see how bad it looks, but I don't know that we
really have any choice. Correctness bugs are bad.

One problem is that I think the newly-registered VM blocks will get
fed to heap_mask() if wal_consistency_checking is turned on, but it's
really only meant to work with heap pages.

Other comments follow.

0001. I personally don't find this to be an improvement. Even it is, I
feel like a back-patched commit is not a good place to invent a new
convention for identifying block reference indexes. However, it's
ultimately a matter of style, so if I lose this particular argument,
c'est la vie. I just feel like if we introduce this for every WAL
record, it's going to be a lot of new #defines that are going to be
long and clunky, and if we don't, then we have a weird mix of styles.

0002. In heap_insert, the local variable called 'all_visible_cleared'
seems like it needs a bit of work. You'd expect such a variable to be
set at the point where we clear the all-visible bit, but it's actually
set before that, when we lock the buffer. Later, you write in the
comments "We locked vmbuffer if all_visible_cleared was true
regardless of whether or not we ended up modifying it," which
similarly implies that the variable is now misnamed. However,
PageClearAllVisible() is called if and only if all_visible_cleared is
true, so that comment doesn't seem to be entirely accurate. My first
instinct is to rename the variable to clear_all_visible and delete the
comment, but there might be something better.

Looks like heap_delete has the same issue, and heap_update has a
similar issue multiplied by two (all_visible_cleared vs.
all_visible_cleared_new).

In heap_update, the comment /* clear PD_ALL_VISIBLE flags, reset all
visibilitymap bits */ has been lost, and the code has been reordered
slightly in that area. This is fine, but keeping the existing comment
and/or code ordering could be worth considering.

In heap_lock_tuple, I think it might be a good idea to set
unlock_vmbuffer = false when we release the lock, and then just after
the out_unlocked: label, Assert(!unlock_vmbuffer). That way, if
someone adds an improvident goto in a future patch, it has a chance of
tripping an Assert.

I think the comment for heap_xlog_vm_clear could be better: instead of
saying that it "handles" something, say what it actually does.
Likewise "use it for redo" inside the function is a little imprecise.
If I'm understanding the purpose of this machinery correctly, the
point here is that any FPIs included for the VM block references need
to be applied, and otherwise (if there's no FPI or the block reference
is not included at all) we need to just clear the relevant bit. That
seems correct, but a little more work on the comments would make it
clearer to future readers, I feel.

In heap_xlog_multi_insert, you delete the comment "The visibility map
may need to be fixed even if the heap page is already up-to-date," and
replace it with a completely new comment that seems to be addresing
someone different concerns, but the identical comments in other
functions in the file are retained. Consider adding to the comment
instead of replacing it. Also, I am not sure I understand the point of
the new comment. Perhaps try to make it clearer.

I find the phrasing of the comment addition at the top of
visibilitymap_clear to be a little harder to understand than
necessary. Maybe: "Most callers should use visibilitymap_clear_locked
rather than this function. It is usually necessary to register the VM
buffer in the WAL record, and this necessitates holding the lock for
longer than it is held here. However, we retain this function for
backward compatibility, and for a few callers that don't have this
requirement."

The redo routines ignore the return value of
visibilitymap_clear_locked, which means they set the page LSN even if
it isn't modified. Apparently there is other precedent for that
treatment, but maybe it's best avoided anyway.

I think that the commit messages could use some polishing. For 0001,
if we keep it, I'd suggest that the subject line should use language
like "introduce macros for" instead of "alias" because nobody's going
to know what "alias" means without reading the commit itself. For
0002, I'd suggest that the subject line should be something like "Fix
heap WAL records to register VM buffers when clearing VM bits". I
think your goals here should be (1) to maximize the chance that
somebody can understand what the commit does if they just read the
one-line summary, or at most the whole commit message, and (2) to
minimize the amount of work it will take to turn your commit message
into a release note entry. I'm not saying your existing commit
messages are bad, but they read a bit as if they were written to
summarize work that the reader has already seen.

-- 
Robert Haas
EDB: http://www.enterprisedb.com