Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts

Thanks for the review and analysis.

On Tue, Oct 13, 2026 at 8:39 PM surya poondla <suryapoondla4(at)gmail(dot)com> wrote:
>
>> 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.

Thanks for the feedback and for doing that investigation. I changed
the test to just check the VM contents across primary and standby with
pg_visibilitymap_summary(). That should resolve this issue.

> 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.

In the new version of the test, I only check the VM page contents are
the same on primary and standby. I'm not too worried about checking
the FSM because it is never a correctness issue for the FSM to be
wrong. We could add something to the test checking that the contents
of the main fork are the same on primary and standby, but that
wouldn't fail without the commit (since it wasn't broken before), so I
didn't do that so far.

> 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.

Right. The new test only compares the VM, so it is only examining one
page. So, that should be efficient enough.

> 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.
<-- snip -->
> 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?

After trying to implement it, excepting heap tables and btree indexes
was a relatively small diff, so I went ahead and did that in v2.

> 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.

Good point. I haven't tried yet, but I think it shouldn't be too hard
to do this ourselves in backbranches. I will check.

> 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.

I think it's probably best to ensure that their criteria are the same,
ensuring that the bytes proven safe to discard are exactly those
removed from the full-page image. It's less of a code cleanup and more
of a code drift protection. But I might be able to be convinced
otherwise.

- Melanie