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

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