WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts

Matthias van de Meent <boekewurm+postgres@gmail.com> writes:

PFA a patch that fixes this issue, by assuming that all pages in the
source database utilize a non-standard page layout.

Surely that cure is worse than the disease?

regards, tom lane

On Mon, 13 May 2024 at 16:13, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Matthias van de Meent <boekewurm+postgres@gmail.com> writes:

PFA a patch that fixes this issue, by assuming that all pages in the
source database utilize a non-standard page layout.

Surely that cure is worse than the disease?

I don't know where we would get the information whether the selected
relation fork's pages are standard-compliant. We could base it off of
the fork number (that info is available locally) but that doesn't
guarantee much.
For VM and FSM-pages we know they're essentially never
standard-compliant (hence this thread), but for the main fork it is
anyone's guess once the user has installed an additional AM - which we
don't detect nor pass through to the offending
RelationCopyStorageUsingBuffer.

As for "worse", the default template database is still much smaller
than the working set of most databases. This will indeed regress the
workload a bit, but only by the fraction of holes in the page + all
FSM/VM data.
I think the additional WAL volume during CREATE DATABASE is worth it
when the alternative is losing that data with physical
replication/secondary instances. Note that this does not disable page
compression, it just stops the logging of holes in pages; holes which
generally are only a fraction of the whole database.

It's not inconceivable that this will significantly increase WAL
volume, but I think we should go for correctness rather than fastest
copy. If we went with fastest copy, we'd better just skip logging the
FSM and VM forks because we're already ignoring the data of the pages,
so why not ignore the pages themselves, too? I don't think that holds
water when we want to be crash-proof in CREATE DATABASE, with a full
data copy of the template database.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

On Mon, May 13, 2024 at 10:53 AM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:

It's not inconceivable that this will significantly increase WAL
volume, but I think we should go for correctness rather than fastest
copy.

I don't think we can afford to just do this blindly for the sake of a
hypothetical non-core AM that uses nonstandard pages. There must be
lots of cases where the holes are large, and where the WAL volume
would be a multiple of what it is currently. That's a *big*
regression.

If we went with fastest copy, we'd better just skip logging the
FSM and VM forks because we're already ignoring the data of the pages,
so why not ignore the pages themselves, too? I don't think that holds
water when we want to be crash-proof in CREATE DATABASE, with a full
data copy of the template database.

This seems like a red herring. Either assuming standard pages is a
good idea or it isn't, and either logging the FSM and VM forks is a
good idea or it isn't, but those are two separate questions.

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

Hi,

Quick question, are there any more revisions left to be done on this patch from the previous feedback?
Or should I continue with reviewing the current patch?

Regards,
Akshat Jaimini

On Sat, Sep 14, 2024 at 06:57:21PM +0000, Akshat Jaimini wrote:

Quick question, are there any more revisions left to be done on this
patch from the previous feedback?

This patch is still listed in the CF app waiting on author with what
looks like Robert and Tom objecting to it, because it makes all
callers of CreateAndCopyRelationData() much more expensive in terms of
WAL generated when copying these files as the holes of the pages would
be included. This would also make the compression of the FPWs more
expensive.

One potential way to move forward would be to spread the knowledge of
page_std when logging a new page higher in the callers, then allow
dbcommands.c to be smarter about that?
--
Michael

On 2024-Dec-10, Michael Paquier wrote:

One potential way to move forward would be to spread the knowledge of
page_std when logging a new page higher in the callers, then allow
dbcommands.c to be smarter about that?

Matthias, is this patch still under consideration?

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Update: super-fast reaction on the Postgres bugs mailing list. The report
was acknowledged [...], and a fix is under discussion.
The wonders of open-source !"
https://twitter.com/gunnarmorling/status/1596080409259003906

Hi,

Reviving this thread: the bug reported here in 2024 is still present
on master, and has become more harmful since.

To recap the root cause: RelationCopyStorageUsingBuffer() copies every
fork page by page and WAL-logs each copied page with
log_newpage_buffer(buf, page_std = true).  page_std=true punches the
[pd_lower, pd_upper) range out of the full-page image as a hole, to
save WAL.  That works for heap and index pages, but VM and FSM pages
never maintain pd_lower/pd_upper - they keep whatever PageInit() left
(pd_lower = SizeOfPageHeaderData, pd_upper = BLCKSZ).  So for those
forks the "hole" covers the whole content area, the FPI carries
nothing but the header, and replay zero-fills it.  The primary lands
the real VM/FSM contents on disk, while the standby replays all-zero
pages.  As far as I can see, this call site is the only one that logs
VM/FSM pages in standard mode - everywhere else they are logged with
page_std=false.

What changed since the last discussion
--------------------------------------
The divergence was assessed back then as "not actively harmful". That
no longer holds in v19: commit add323da40a added
Assert(BufferIsDirty(vmbuffer)) to heap_xlog_prune_freeze(), which
makes the divergence fatal on assert builds after a switchover:

1. The promoted standby has a zeroed VM, so VACUUM FREEZE sets the
   bits again, emitting PRUNE records with no FPI for the VM buffer.
2. The old primary, rejoined as standby, still has the bits set with
   an older page LSN, so replay takes the redo branch - but
   visibilitymap_set() is a no-op there (bits already set) and the
   buffer stays clean.
3. Assert(BufferIsDirty(vmbuffer)) fires, and the node crash-loops on
   that record.

And even without assertions, the new database on the standby silently
loses all all-visible/all-frozen bits and all free space data of every
copied relation.

Repro
-----
With a streaming standby attached:

    -- on the primary
    CREATE DATABASE src_db;
    \c src_db
    CREATE EXTENSION pageinspect;
    CREATE TABLE t(a int);
    INSERT INTO t SELECT generate_series(1, 10000);
    VACUUM FREEZE t;
    -- back in postgres
    CREATE DATABASE dst_db TEMPLATE src_db STRATEGY WAL_LOG;

    -- then on both nodes, in dst_db:
    SELECT encode(get_raw_page('t', 'vm', 0), 'hex');
    SELECT encode(get_raw_page('t', 'fsm', 0), 'hex');
    -- primary shows real content, standby is all zeros

For the assert crash: run with full_page_writes=off so the PRUNE
record carries no FPI for the VM page, promote the standby, rejoin the
old primary as its standby, then VACUUM (FREEZE, DISABLE_PAGE_SKIPPING)
t on the new primary.  The old primary dies on the assert while
replaying the PRUNE records.

0002 adds a TAP test to src/test/recovery/t/034_create_database.pl
which automates the scenario above: all new checks fail without 0001
and pass with it (verified on REL_19_STABLE with --enable-cassert).

Fix
---
The v1 patch logged all copied pages with page_std=false, which was
objected to because of the WAL volume increase on the main fork: holes
would no longer be punched from heap/index pages.  Attached v2 instead
makes the decision per fork, along the lines Michael suggested. The
VM and FSM forks are the only ones whose pages never maintain a
standard page layout, so only their pages are logged whole:

-        /* WAL-log the copied page. */
+        /*
+         * WAL-log the copied page.  VM/FSM pages never maintain
+         * pd_lower/pd_upper, so hole punching would omit their entire
+         * content from the FPI.
+         */
          if (use_wal)
-            log_newpage_buffer(dstBuf, true);
+            log_newpage_buffer(dstBuf,
+                               forkNum != VISIBILITYMAP_FORKNUM &&
+                               forkNum != FSM_FORKNUM);

Main and init forks keep hole punching, so their WAL volume is
unchanged; the extra WAL is limited to the VM/FSM forks, which are
small.  The fork number is available locally in
RelationCopyStorageUsingBuffer() (it already branches on it for
use_wal), so no plumbing through dbcommands.c is needed.

The bug goes back to v15, where STRATEGY WAL_LOG was added
(9c08aea6a30); the assertion crash additionally requires v19
(add323da40a), so older branches see only the VM/FSM divergence.
This should be backpatched to all supported versions.

Regards,
Rogers Wang

On Mon, Aug 24, 2026 at 01:04:44AM +0800, Rogers Wang wrote:

What changed since the last discussion
--------------------------------------
The divergence was assessed back then as "not actively harmful". That
no longer holds in v19: commit add323da40a added
Assert(BufferIsDirty(vmbuffer)) to heap_xlog_prune_freeze(), which
makes the divergence fatal on assert builds after a switchover:

1. The promoted standby has a zeroed VM, so VACUUM FREEZE sets the
   bits again, emitting PRUNE records with no FPI for the VM buffer.
2. The old primary, rejoined as standby, still has the bits set with
   an older page LSN, so replay takes the redo branch - but
   visibilitymap_set() is a no-op there (bits already set) and the
   buffer stays clean.
3. Assert(BufferIsDirty(vmbuffer)) fires, and the node crash-loops on
   that record.

And even without assertions, the new database on the standby silently
loses all all-visible/all-frozen bits and all free space data of every
copied relation.

Hmm. This warrants an open item for v19, at least to be discussed.
Adding Melanie in CC as the committer of add323da40a.
--
Michael

On Mon, Aug 31, 2026 at 1:37 AM Michael Paquier <michael@paquier.xyz> wrote:

On Mon, Aug 24, 2026 at 01:04:44AM +0800, Rogers Wang wrote:

What changed since the last discussion
--------------------------------------
The divergence was assessed back then as "not actively harmful". That
no longer holds in v19: commit add323da40a added
Assert(BufferIsDirty(vmbuffer)) to heap_xlog_prune_freeze(), which
makes the divergence fatal on assert builds after a switchover:

1. The promoted standby has a zeroed VM, so VACUUM FREEZE sets the
bits again, emitting PRUNE records with no FPI for the VM buffer.
2. The old primary, rejoined as standby, still has the bits set with
an older page LSN, so replay takes the redo branch - but
visibilitymap_set() is a no-op there (bits already set) and the
buffer stays clean.
3. Assert(BufferIsDirty(vmbuffer)) fires, and the node crash-loops on
that record.

And even without assertions, the new database on the standby silently
loses all all-visible/all-frozen bits and all free space data of every
copied relation.

Hmm. This warrants an open item for v19, at least to be discussed.
Adding Melanie in CC as the committer of add323da40a.

This part

And even without assertions, the new database on the standby silently
loses all all-visible/all-frozen bits and all free space data of every
copied relation.

is true completely independently of my commit. Only the assert is new
and would require changes to v19. I didn't think of this way of ending
up with a discrepancy between primary and standby VM. I assumed that
if the VM bit was not set on the primary it would not be set on the
standby. Even if we commit something to eliminate this corner case
(with RelationCopyStorageUsingBuffer()), you could probably end up
with a lost or truncated VM on the primary and the VM being set on the
standby in some other way, so we shouldn't assume that the
visibilitymap_set() will dirty the buffer. We can't set the page LSN
unless we marked the buffer dirty, so for 19, I think I could just do
if (BufferIsDirty()) -> PageSetLSN()
In master, it probably makes sense to change back the signature of
visibilitymap_set() to report if it did actually set the bit(s) and
then use that to decide whether to set the page LSN. Unless we don't
think it is too late in 19 to change the visibilitymap_set() API. It
does mean all the other callers of visibilitymap_set() are going to
discard the return value, but maybe that's okay?

As for whether we should log full page images of the VM and FSM in
RelationCopyStorageUsingBuffer(), that kind of seems like the right
direction to go, but that doesn't feel backpatchable to me. I
understand this doesn't address Matthias' concern about certain AMs,
but it seems like a good idea to have the VM and FSM up-to-date after
promotion and FPIs of just those forks doesn't seem like an
unacceptable increase in WAL volume. I don't know if doing this will
have any side effects or causes bugs in other situations, though. I'd
have to think more about it...

- Melanie

On Mon, 31 Aug 2026 at 17:46, Melanie Plageman
<melanieplageman@gmail.com> wrote:

On Mon, Aug 31, 2026 at 1:37 AM Michael Paquier <michael@paquier.xyz> wrote:

Hmm. This warrants an open item for v19, at least to be discussed.
Adding Melanie in CC as the committer of add323da40a.

This part

I think a message got cut off.

As for whether we should log full page images of the VM and FSM in
RelationCopyStorageUsingBuffer(), that kind of seems like the right
direction to go, but that doesn't feel backpatchable to me. I
understand this doesn't address Matthias' concern about certain AMs,
but it seems like a good idea to have the VM and FSM up-to-date after
promotion and FPIs of just those forks doesn't seem like an
unacceptable increase in WAL volume. I don't know if doing this will
have any side effects or causes bugs in other situations, though. I'd
have to think more about it...

IIUC, CREATE DATABASE doesn't allow concurrent R/RW connections to the
template database, for neither WAL_LOG nor FILE_COPY. In this case
there shouldn't be any issues with copying VM pages that aren't
already present in the file-copy approach. The only difference here
would be that VM/FSM pages now get a more recent LSN on their pages,
and I think this should be fine because I can't think of a mechanism
that checks for (the consistency of) VM/FSM page LSNs.

Kind regards,

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

On Mon, Aug 31, 2026 at 11:46 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:

This part

And even without assertions, the new database on the standby silently
loses all all-visible/all-frozen bits and all free space data of every
copied relation.

is true completely independently of my commit. Only the assert is new
and would require changes to v19. I didn't think of this way of ending
up with a discrepancy between primary and standby VM. I assumed that
if the VM bit was not set on the primary it would not be set on the
standby. Even if we commit something to eliminate this corner case
(with RelationCopyStorageUsingBuffer()), you could probably end up
with a lost or truncated VM on the primary and the VM being set on the
standby in some other way, so we shouldn't assume that the
visibilitymap_set() will dirty the buffer. We can't set the page LSN
unless we marked the buffer dirty, so for 19, I think I could just do
if (BufferIsDirty()) -> PageSetLSN()
In master, it probably makes sense to change back the signature of
visibilitymap_set() to report if it did actually set the bit(s) and
then use that to decide whether to set the page LSN. Unless we don't
think it is too late in 19 to change the visibilitymap_set() API. It
does mean all the other callers of visibilitymap_set() are going to
discard the return value, but maybe that's okay?

Since we are putting out a beta 4, I feel fine about changing the
visiblitymap_set() API back. Here is a patch that does that.

- Melanie

On Fri, Sep 11, 2026 at 4:44 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:

In master, it probably makes sense to change back the signature of
visibilitymap_set() to report if it did actually set the bit(s) and
then use that to decide whether to set the page LSN. Unless we don't
think it is too late in 19 to change the visibilitymap_set() API. It
does mean all the other callers of visibilitymap_set() are going to
discard the return value, but maybe that's okay?

Since we are putting out a beta 4, I feel fine about changing the
visiblitymap_set() API back. Here is a patch that does that.

I've pushed a fix for this and marked the open item as resolved.

- Melanie

On Mon, Aug 31, 2026 at 12:08 PM Matthias van de Meent
<boekewurm+postgres@gmail.com> wrote:

On Mon, 31 Aug 2026 at 17:46, Melanie Plageman
<melanieplageman@gmail.com> wrote:

As for whether we should log full page images of the VM and FSM in
RelationCopyStorageUsingBuffer(), that kind of seems like the right
direction to go, but that doesn't feel backpatchable to me. I
understand this doesn't address Matthias' concern about certain AMs,
but it seems like a good idea to have the VM and FSM up-to-date after
promotion and FPIs of just those forks doesn't seem like an
unacceptable increase in WAL volume. I don't know if doing this will
have any side effects or causes bugs in other situations, though. I'd
have to think more about it...

IIUC, CREATE DATABASE doesn't allow concurrent R/RW connections to the
template database, for neither WAL_LOG nor FILE_COPY. In this case
there shouldn't be any issues with copying VM pages that aren't
already present in the file-copy approach. The only difference here
would be that VM/FSM pages now get a more recent LSN on their pages,
and I think this should be fine because I can't think of a mechanism
that checks for (the consistency of) VM/FSM page LSNs.

I now think we have to do something. While exploring another adjacent
topic, I realized that tuple locking has a seemingly unsolvable
problem with this.

If you have a page set all-frozen in the VM and then do CREATE
DATABASE STRATEGY WAL_LOG, your new database will have that page set
all-frozen in the VM on the primary and set neither all-visible nor
all-frozen on the standby. When that standby is promoted, you now have
a primary with the page NOT set all-frozen and a standby with the page
set all-frozen. If you then lock the tuple on the primary, it will
emit a WAL record with no XLH_LOCK_ALL_FROZEN_CLEARED flag because the
primary did not need to clear all-frozen. The standby will update the
xmax but not clear the all-frozen bit in the VM. Now you have a heap
page with a live xmax that is set all-frozen in the VM.

Basically, if you have some all-frozen data and do CREATE DATABASE
STRATEGY WAL_LOG and then failover and then do a SELECT FOR UPDATE,
you'll have data corruption. That seems bad.

I don't see any reasonable way to fix this other than to prevent it
from happening. Because we are only clearing the all-frozen bit,
including a VM clearing operation in every lock record in which the
page is all-visible, means that as soon as a page is all-visible every
single time we lock it the WAL record will have to include the VM page
-- which is very expensive.

This should fix that particular way to get into this situation
if (use_wal)
log_newpage_buffer(dstBuf, forkNum != VISIBILITYMAP_FORKNUM &&
forkNum != FSM_FORKNUM);

But if there are other ways to get into this situation, I'm not quite
sure what we could do. I asked an LLM to come up with scenarios and
outside of RelationCopyStorageUsingBuffer(). It couldn't identify
other specific scenarios that could put us in this bad state. It
mentioned some things with incremental backups from before ed62d26caca
or bad basebackup/crash recovery/PITR scenarios, but I'd need to do
more investigation. After a cursory investigation, the possibility for
those to create this scenario and not other problems seems pretty
remote. Fixing RelationCopyStorageUsingBuffer() seems required at this
point, though.

- Melanie

On Wed, Sep 16, 2026 at 10:26 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:

I now think we have to do something. While exploring another adjacent
topic, I realized that tuple locking has a seemingly unsolvable
problem with this.

If you have a page set all-frozen in the VM and then do CREATE
DATABASE STRATEGY WAL_LOG, your new database will have that page set
all-frozen in the VM on the primary and set neither all-visible nor
all-frozen on the standby. When that standby is promoted, you now have
a primary with the page NOT set all-frozen and a standby with the page
set all-frozen. If you then lock the tuple on the primary, it will
emit a WAL record with no XLH_LOCK_ALL_FROZEN_CLEARED flag because the
primary did not need to clear all-frozen. The standby will update the
xmax but not clear the all-frozen bit in the VM. Now you have a heap
page with a live xmax that is set all-frozen in the VM.

Basically, if you have some all-frozen data and do CREATE DATABASE
STRATEGY WAL_LOG and then failover and then do a SELECT FOR UPDATE,
you'll have data corruption. That seems bad.

I attached an LLM-generated reproducer.

- Melanie

On Mon, Aug 31, 2026 at 11:47 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:

As for whether we should log full page images of the VM and FSM in
RelationCopyStorageUsingBuffer(), that kind of seems like the right
direction to go, but that doesn't feel backpatchable to me. I
understand this doesn't address Matthias' concern about certain AMs,
but it seems like a good idea to have the VM and FSM up-to-date after
promotion and FPIs of just those forks doesn't seem like an
unacceptable increase in WAL volume. I don't know if doing this will
have any side effects or causes bugs in other situations, though. I'd
have to think more about it...

I feel like treating the VM and FSM as non-standard pages is the right
thing to do, because in fact they are not standard pages. Unless you
know something I don't, I think we need to fix
RelationCopyStorageUsingBuffer to treat them that way. What's your
reason for not wanting to back-patch that change?

AFAICS, the tougher question is what to do about the main fork. I
originally resisted the idea of treating all pages as non-standard
pages here because, well, most of the time they will be standard, and
treating them as non-standard adds considerable expense. However, a
bit of exploration with Claude poked a few holes in this idea. It
notes that:

1. RelationCopyStorage treats all pages as non-standard, so having
RelationCopyStorageUsingBuffer do otherwise is inconsistent.

2. ginDeletePostingPage has a comment saying that GIN uses
non-standard pages prior to 9.4.

3. The value of STRATEGY WAL_LOG is that it's faster for small
databases, so the additional WAL volume may not be a huge deal anyway.

So I'm starting to think Matthias had the right idea back in 2024. It
still seems like a lamentable change to me, but to quote Tom quoting
an unnamed wise man: "I can make this code arbitrarily fast, if it
doesn't have to give the right answer." As far as that goes, point #2
is the strongest argument: there aren't likely many people who have a
GIN index that hasn't been rewritten since pre-9.4 and who have that
index inside of a database that they use as a template, but any such
people who do exist will be justifiably annoyed if CREATE DATABASE
with any set of options produces a corrupt copy.

--
Robert Haas

On Wed, Sep 16, 2026 at 5:59 PM Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Aug 31, 2026 at 11:47 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:

As for whether we should log full page images of the VM and FSM in
RelationCopyStorageUsingBuffer(), that kind of seems like the right
direction to go, but that doesn't feel backpatchable to me. I
understand this doesn't address Matthias' concern about certain AMs,
but it seems like a good idea to have the VM and FSM up-to-date after
promotion and FPIs of just those forks doesn't seem like an
unacceptable increase in WAL volume. I don't know if doing this will
have any side effects or causes bugs in other situations, though. I'd
have to think more about it...

I feel like treating the VM and FSM as non-standard pages is the right
thing to do, because in fact they are not standard pages. Unless you
know something I don't, I think we need to fix
RelationCopyStorageUsingBuffer to treat them that way. What's your
reason for not wanting to back-patch that change?

That was before I realized that it allows us to trivially cause a
serious data corruption bug with tuple locking. We should definitely
backpatch the part that passes false for VM and FSM.

AFAICS, the tougher question is what to do about the main fork. I
originally resisted the idea of treating all pages as non-standard
pages here because, well, most of the time they will be standard, and
treating them as non-standard adds considerable expense. However, a
bit of exploration with Claude poked a few holes in this idea. It
notes that:

1. RelationCopyStorage treats all pages as non-standard, so having
RelationCopyStorageUsingBuffer do otherwise is inconsistent.

2. ginDeletePostingPage has a comment saying that GIN uses
non-standard pages prior to 9.4.

3. The value of STRATEGY WAL_LOG is that it's faster for small
databases, so the additional WAL volume may not be a huge deal anyway.

Andres suggested off-list having CreateDatabaseUsingWalLog() thread
information from the catalog about whether the page is a normal heap
page down into RelationCopyStorageUsingBuffer().

I looked into it and we could, when scanning source database pg_class
(ScanSourceDatabasePgClassTuple()), check something like:
RELKIND_HAS_TABLE_AM(classForm->relkind) && classForm->relam ==
HEAP_TABLE_AM_OID
and allowing the "hole punching" if so.

And, it's even easier to do this in RelationCopyStorage() without any
extra catalog info because the callers know what type of relation it
is. So, we could fix RelationCopyStorageUsingBuffer() and make it
consistent with RelationCopyStorage() at the same time (master only).

- Melanie

On Tue, Sep 22, 2026 at 7:12 PM Melanie Plageman
<melanieplageman@gmail.com> wrote:

Andres suggested off-list having CreateDatabaseUsingWalLog() thread
information from the catalog about whether the page is a normal heap
page down into RelationCopyStorageUsingBuffer().

Upon further off-list discussion, instead of checking the AM, we can
just check if the space between pd_upper and pd_lower is all zeroes.
If it is, there is no harm in omitting it. It should always work,
allows the optimization in more cases, and requires less code.
Attached patch does that.

I also added a test. It compares the pages of all forks of a table
between a primary and standby after doing CREATE DATABASE USING
STRATEGY WAL_LOG. I compare the whole page byte-by-byte including page
header and LSN. Are those always guaranteed to be the same on the
standby as well?

- Melanie

Hi Melanie,

Thanks for the patch. I applied it on master (6b3f8857aa8). It applies and
builds cleanly.

Upon further off-list discussion, instead of checking the AM, we can
just check if the space between pd_upper and pd_lower is all zeroes.
If it is, there is no harm in omitting it. It should always work,
allows the optimization in more cases, and requires less code.
Attached patch does that.

I also added a test. It compares the pages of all forks of a table
between a primary and standby after doing CREATE DATABASE USING
STRATEGY WAL_LOG. I compare the whole page byte-by-byte including page
header and LSN. Are those always guaranteed to be the same on the
standby as well?

The approach looks correct to me. On replay, RestoreBlockImage() fills the
hole with zeros, so leaving out a hole that is already all zeros
can't change the page. Because RelationCopyStorageUsingBuffer() and
XLogRecordAssemble() now share PageFreeSpaceBoundsAreValid(), the range
checked for zeros is exactly the range that gets omitted. I like that it
needs no AM or fork knowledge.

On your question about the page header, I don't think the whole page is
guaranteed to match.
pd_lsn should match. log_newpage() sets it on the primary, and
XLogReadBufferForRedoExtended() sets it to the same record LSN after
restoring the image.

pd_checksum doesn't, though. The image is taken right after the memcpy, so
it carries the template page's checksum. After that, each node's
in-memory pd_checksum depends on whether that node has flushed the page
(which, since 41d3d64e87a, updates the checksum in place) or re-read it
from disk. The test checkpoints and restarts the primary, but does nothing
comparable on the standby.

I ran some experiments with a streaming standby (checksums on), page 0 of
the copied table's vm fork looked like this:

lsn checksum
primary, after CHECKPOINT 0/3082BA50 -24598
standby, just replayed 0/3082BA50 -7694 (= template's)
standby, after CHECKPOINT 0/3082BA50 -24598

The fsm page behaves the same way. After the standby's CHECKPOINT the raw
pages matched exactly.

The test passes as written, but I think only because allows_streaming sets
shared_buffers = 1MB, so the standby evicts (and re-checksums) the copied
pages before the comparison.
Adding $standby->append_conf('postgresql.conf', 'shared_buffers = 128MB');
before $standby->start makes both the vm and fsm checks fail here.
The only differing bytes are 8-9 (pd_checksum); pd_lsn matches. A
CHECKPOINT on the standby before comparing, or comparing from
SizeOfPageHeaderData onward, should make it robust.

A few things about the test:
1. The loop compares only block 0 of the vm and fsm forks. Your mail
mentions comparing all forks, but the main fork isn't checked.
If that was left out on purpose (hint bits, say), a comment explaining why
would help. Comparing every block of each fork would also make
the test more robust. The FSM is multi-level, and the old bug affected all
of its pages.

2. In the unpatched build, every vm and fsm image the copy wrote had "hole:
offset: 24, length: 8168", i.e. only the page header was
logged. So a vm/fsm check on any non-empty block will catch a regression.

Regarding WAL volume, in my test, after deletes and VACUUM, almost no heap
or btree page had an all-zero hole. Pruning and compaction leave the old
tuple bytes behind. So for templates with any update/delete history, most
main-fork pages are now logged in full. I measured WAL from
CREATE DATABASE ... STRATEGY WAL_LOG with and without the bufmgr.c changes.
The two test templates each had a 1M-row table with a btree index:

template unpatched patched
template1 (stock) 4.5 MB 6.0 MB
INSERT 1M rows; VACUUM FREEZE 113 MB 114 MB
same, then DELETE 1/3, UPDATE 10%, VACUUM 79 MB 117 MB

In the delete/update case, pg_waldump --bkp-details showed all 11364 heap
pages and 2735 of 2745 btree pages logged as full images, compared with
almost none before. When VACUUM prunes a page, it compacts the remaining
tuples but doesn't zero the space it frees, so the hole still
holds old tuple data and fails the all-zeros check. Btree pages behave the
same way once VACUUM removes index entries. Even stock template1 loses the
hole on 125 catalog pages, presumably because initdb updates and deletes
catalog rows.

I think correctness has to come first, and the append-only case costs
almost nothing extra. But for a template whose tables have had deletes
or updates followed by VACUUM, CREATE DATABASE wrote about 50% more WAL in
my test (79 MB vs 117 MB), so it might be worth mentioning in the commit
message. Do you think it's worth keeping hole punching for pages we know
are standard (e.g. heap and btree), or is the simpler rule the better
trade-off?

Regarding back-patching, pg_memory_is_all_zeros() was added in v18 (commit
07e9e28b56d), so v15-v17 will need it back-patched or an open-coded
loop. The XLogRecordAssemble() refactor isn't needed for the fix and could
stay master-only, if you'd prefer the back branches to change as
little as possible.

Regards,
Surya Poondla