WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t49624psql -h localhost -U postgresBuilt from patchset v8 (message #8), August 24, 2026 at 12:30 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t49624_8 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t49624_8 && git checkout t49624_8Patchset v8 (message #8) is on t49624_8
Hi,
My collegue Konstantin Knizhnik pointed out that we fail to mark pages
with a non-standard page layout with page_std=false in
RelationCopyStorageUsingBuffer when we WAL-log them. This causes us to
interpret the registered buffer as a standard buffer, and omit the
hole in the page, which for FSM/VM pages covers the whole page.
The immediate effect of this bug is that replicas and primaries in a
physical replication system won't have the same data in their VM- and
FSM-forks until the first VACUUM on the new database has WAL-logged
these pages again. Whilst not actively harmful for the VM/FSM
subsystems, it's definitely suboptimal.
Secondary unwanted effects are that AMs that use the buffercache- but
which don't use or update the pageheader- also won't see the main data
logged in WAL, thus potentially losing user data in the physical
replication stream or with a system crash. I've not looked for any
such AMs and am unaware of any that would have this issue, but it's
better to fix this.
PFA a patch that fixes this issue, by assuming that all pages in the
source database utilize a non-standard page layout.
Kind regards,
Matthias van de Meent
Neon (https://neon.tech)
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