pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedWAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
17+ messages / 8 participants
[nested] [flat]
* WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2024-05-13 12:31 Matthias van de Meent <boekewurm+postgres@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Matthias van de Meent @ 2024-05-13 12:31 UTC (permalink / raw)
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Robert Haas <robertmhaas@gmail.com>
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)
Attachments:
[application/octet-stream] v1-0001-Fix-logging-of-non-standard-pages-in-RelationCopy.patch (1.4K, ../../CAEze2WgNjODiAiTU18E06_BrRK0nqwJ1kdS_bN61c2_DpFDSnQ@mail.gmail.com/2-v1-0001-Fix-logging-of-non-standard-pages-in-RelationCopy.patch)
download | inline diff:
From ca3e957b7684c5e67ff8868f6d53cbcf9e4e4d1f Mon Sep 17 00:00:00 2001
From: Matthias van de Meent <boekewurm+postgres@gmail.com>
Date: Mon, 13 May 2024 13:48:04 +0200
Subject: [PATCH v1] Fix logging of non-standard pages in
RelationCopyStorageUsingBuffer
If the data in the page is non-standard (e.g. the VM, FSM, or an access
method that doesn't update PageHeader's fields as required) then we may
fail to WAL-log the data on that page. When the server crashes after
flushing the WAL record, but before flushing the file writes, this data
may be permanently lost.
Reported-By: Konstantin Knizhnik
---
src/backend/storage/buffer/bufmgr.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 49637284f9..6a3b9357db 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -4695,9 +4695,13 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
memcpy(dstPage, srcPage, BLCKSZ);
MarkBufferDirty(dstBuf);
- /* WAL-log the copied page. */
+ /*
+ * WAL-log the copied page.
+ * Note that we don't know about the type of data contained in the
+ * page, so we can't report that the buffer is a standard page.
+ */
if (use_wal)
- log_newpage_buffer(dstBuf, true);
+ log_newpage_buffer(dstBuf, false);
END_CRIT_SECTION();
--
2.40.1
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2024-05-13 14:13 Tom Lane <tgl@sss.pgh.pa.us>
parent: Matthias van de Meent <boekewurm+postgres@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Tom Lane @ 2024-05-13 14:13 UTC (permalink / raw)
To: Matthias van de Meent <boekewurm+postgres@gmail.com>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Robert Haas <robertmhaas@gmail.com>
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2024-05-13 14:52 Matthias van de Meent <boekewurm+postgres@gmail.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 17+ messages in thread
From: Matthias van de Meent @ 2024-05-13 14:52 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>; Robert Haas <robertmhaas@gmail.com>
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)
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2024-05-13 19:43 Robert Haas <robertmhaas@gmail.com>
parent: Matthias van de Meent <boekewurm+postgres@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Robert Haas @ 2024-05-13 19:43 UTC (permalink / raw)
To: Matthias van de Meent <boekewurm+postgres@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2024-09-14 18:57 Akshat Jaimini <destrex271@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Akshat Jaimini @ 2024-09-14 18:57 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Matthias van de Meent <boekewurm+postgres@gmail.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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2024-12-10 06:25 Michael Paquier <michael@paquier.xyz>
parent: Akshat Jaimini <destrex271@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Michael Paquier @ 2024-12-10 06:25 UTC (permalink / raw)
To: Akshat Jaimini <destrex271@gmail.com>; +Cc: pgsql-hackers@lists.postgresql.org, Matthias van de Meent <boekewurm+postgres@gmail.com>
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../Z1fezFJJJwBxtnkD@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-02-02 15:26 Álvaro Herrera <alvherre@kurilemu.de>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 17+ messages in thread
From: Álvaro Herrera @ 2026-02-02 15:26 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Akshat Jaimini <destrex271@gmail.com>; pgsql-hackers@lists.postgresql.org, Matthias van de Meent <boekewurm+postgres@gmail.com>
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-08-23 17:04 Rogers Wang <rogers.ww@qq.com>
parent: Álvaro Herrera <alvherre@kurilemu.de>
0 siblings, 1 reply; 17+ messages in thread
From: Rogers Wang @ 2026-08-23 17:04 UTC (permalink / raw)
To: pgsql-hackers@lists.postgresql.org; +Cc: Matthias van de Meent <boekewurm+postgres@gmail.com>; Álvaro Herrera <alvherre@kurilemu.de>; Michael Paquier <michael@paquier.xyz>
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
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 169829eb020..6cee2e21954 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -5454,9 +5454,15 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
memcpy(dstPage, srcPage, BLCKSZ);
MarkBufferDirty(dstBuf);
- /* 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);
END_CRIT_SECTION();
diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile
index 9c4102b6b2c..dfd3617c84c 100644
--- a/src/test/recovery/Makefile
+++ b/src/test/recovery/Makefile
@@ -9,7 +9,8 @@
#
#-------------------------------------------------------------------------
-EXTRA_INSTALL=contrib/pg_prewarm \
+EXTRA_INSTALL=contrib/pageinspect \
+ contrib/pg_prewarm \
contrib/pg_stat_statements \
contrib/test_decoding \
src/test/modules/injection_points \
diff --git a/src/test/recovery/t/034_create_database.pl b/src/test/recovery/t/034_create_database.pl
index 194d1837e23..bff6c65a32b 100644
--- a/src/test/recovery/t/034_create_database.pl
+++ b/src/test/recovery/t/034_create_database.pl
@@ -42,4 +42,133 @@ is($result, "0",
"check that there are no tables from template on new database after crash"
);
+# Check that the VM and FSM contents of relations copied with the
+# WAL_LOG strategy are preserved on a standby.
+#
+# The copy WAL-logs each page via log_newpage_buffer(). VM and FSM
+# pages never maintain pd_lower/pd_upper after PageInit(), so logging
+# them with page_std=true would omit their whole content area from the
+# FPI and replay would zero-fill it. When the promoted standby later
+# vacuums the table, replaying the VM-setting PRUNE records on the old
+# primary (where the bits are already set) would be a no-op that leaves
+# the VM buffer clean, tripping Assert(BufferIsDirty(vmbuffer)).
+
+# full_page_writes=off guarantees that the VM-setting record generated
+# below carries no FPI; the large checkpoint_timeout prevents WAL writes
+# while the old primary is demoted, so no rewind is needed then.
+my $node_primary = PostgreSQL::Test::Cluster->new('primary');
+$node_primary->init(allows_streaming => 1);
+$node_primary->append_conf(
+ 'postgresql.conf', qq{
+full_page_writes = off
+autovacuum = off
+checkpoint_timeout = 1h
+});
+$node_primary->start;
+
+# Create a source database with a table whose VM bits are all set
+# (all-visible and all-frozen) and whose FSM is filled.
+$node_primary->safe_psql('postgres', "CREATE DATABASE src_db");
+$node_primary->safe_psql(
+ 'src_db', qq{
+CREATE EXTENSION pageinspect;
+CREATE TABLE test (a int);
+INSERT INTO test SELECT generate_series(1, 10000);
+VACUUM FREEZE test;
+});
+
+# Check that the VM page has real content beyond the 24-byte page
+# header, so that the comparison below is meaningful.
+my $vm_hex = $node_primary->safe_psql('src_db',
+ "SELECT encode(get_raw_page('test', 'vm', 0), 'hex')");
+like(substr($vm_hex, 48),
+ qr/[^0]/, 'VM bits are set on src_db before CREATE DATABASE');
+
+# Flush and take a base backup; the backup copies the VM/FSM files
+# verbatim.
+$node_primary->safe_psql('postgres', "CHECKPOINT");
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+my $node_standby = PostgreSQL::Test::Cluster->new('standby');
+$node_standby->init_from_backup($node_primary, $backup_name,
+ has_streaming => 1);
+$node_standby->start;
+
+# Create dst_db from src_db: the primary lands the copied forks on
+# disk, while the standby replays the FPIs.
+$node_primary->safe_psql('postgres',
+ "CREATE DATABASE dst_db TEMPLATE src_db STRATEGY WAL_LOG");
+$node_primary->wait_for_catchup($node_standby);
+
+# Compare the VM and FSM pages with pageinspect.
+my $p_vm = $node_primary->safe_psql('dst_db',
+ "SELECT encode(get_raw_page('test', 'vm', 0), 'hex')");
+my $s_vm = $node_standby->safe_psql('dst_db',
+ "SELECT encode(get_raw_page('test', 'vm', 0), 'hex')");
+is($s_vm, $p_vm, 'new database: VM page identical on both nodes');
+
+my $p_fsm = $node_primary->safe_psql('dst_db',
+ "SELECT encode(get_raw_page('test', 'fsm', 0), 'hex')");
+my $s_fsm = $node_standby->safe_psql('dst_db',
+ "SELECT encode(get_raw_page('test', 'fsm', 0), 'hex')");
+is($s_fsm, $p_fsm, 'new database: FSM page identical on both nodes');
+
+# Switch over and check that the old primary survives replaying the
+# PRUNE records generated by vacuuming the table on the new primary.
+
+# Move the redo point past the CREATE DATABASE records, so that crash
+# recovery of the old primary does not replay the CREATE DATABASE FPIs,
+# and wait until the standby has replayed the checkpoint.
+$node_primary->safe_psql('postgres', "CHECKPOINT");
+$node_primary->wait_for_catchup($node_standby);
+
+# Promote the standby, then demote the old primary to a standby of the
+# new primary: its WAL ends exactly at the promotion fork point, so no
+# rewind is needed. The slot preserves the WAL generated below.
+$node_standby->promote;
+
+$node_primary->stop('immediate');
+$node_standby->safe_psql('postgres',
+ "SELECT pg_create_physical_replication_slot('old_primary')");
+my $new_primary_connstr = $node_standby->connstr;
+$node_primary->append_conf(
+ 'postgresql.conf', qq{
+primary_conninfo = '$new_primary_connstr application_name=old_primary'
+primary_slot_name = 'old_primary'
+});
+$node_primary->set_standby_mode;
+$node_primary->start;
+
+# Wait until the new standby is fully caught up.
+$node_standby->wait_for_catchup($node_primary);
+
+# Note the log offset now: the crash can follow the VACUUM within
+# milliseconds.
+my $log_offset = -s $node_primary->logfile;
+
+# If the VM bitmap is all zero on the new primary, vacuum sets the bits
+# again, generating PRUNE records without an FPI for the VM buffer.
+$node_standby->safe_psql('dst_db',
+ "VACUUM (FREEZE, DISABLE_PAGE_SKIPPING) test");
+
+# Replay of the PRUNE records on the new standby would then be a no-op
+# that leaves the VM buffer clean and crashes on
+# Assert(BufferIsDirty(vmbuffer)), in which case the standby never
+# replays past the target LSN and this poll times out. The crash
+# follows the VACUUM within milliseconds, so a short timeout suffices.
+my $target_lsn = $node_standby->lsn('flush');
+my $saved_timeout = $PostgreSQL::Test::Utils::timeout_default;
+$PostgreSQL::Test::Utils::timeout_default = 15;
+my $caught_up = $node_standby->poll_query_until(
+ 'postgres',
+ "SELECT '$target_lsn' <= replay_lsn AND state = 'streaming'
+ FROM pg_catalog.pg_stat_replication
+ WHERE application_name = 'old_primary'");
+$PostgreSQL::Test::Utils::timeout_default = $saved_timeout;
+ok($caught_up, 'new standby catches up after replaying PRUNE records');
+ok(!$node_primary->log_contains(qr/TRAP:.*BufferIsDirty/, $log_offset),
+ 'no assertion crash during replay');
+
+$node_primary->stop;
+
done_testing();
Attachments:
[text/plain] 0001-Fix-hole-punching-of-VM-FSM-pages-in-CREATE-DATABASE.patch (754B, ../../tencent_2E870046716FD94285045E96505A2D4E2908@qq.com/3-0001-Fix-hole-punching-of-VM-FSM-pages-in-CREATE-DATABASE.patch)
download | inline diff:
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index 169829eb020..6cee2e21954 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -5454,9 +5454,15 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator,
memcpy(dstPage, srcPage, BLCKSZ);
MarkBufferDirty(dstBuf);
- /* 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);
END_CRIT_SECTION();
[text/plain] 0002-Add-test-for-VM-FSM-divergence-after-CREATE-DATABASE.patch (6.3K, ../../tencent_2E870046716FD94285045E96505A2D4E2908@qq.com/4-0002-Add-test-for-VM-FSM-divergence-after-CREATE-DATABASE.patch)
download | inline diff:
diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile
index 9c4102b6b2c..dfd3617c84c 100644
--- a/src/test/recovery/Makefile
+++ b/src/test/recovery/Makefile
@@ -9,7 +9,8 @@
#
#-------------------------------------------------------------------------
-EXTRA_INSTALL=contrib/pg_prewarm \
+EXTRA_INSTALL=contrib/pageinspect \
+ contrib/pg_prewarm \
contrib/pg_stat_statements \
contrib/test_decoding \
src/test/modules/injection_points \
diff --git a/src/test/recovery/t/034_create_database.pl b/src/test/recovery/t/034_create_database.pl
index 194d1837e23..bff6c65a32b 100644
--- a/src/test/recovery/t/034_create_database.pl
+++ b/src/test/recovery/t/034_create_database.pl
@@ -42,4 +42,133 @@ is($result, "0",
"check that there are no tables from template on new database after crash"
);
+# Check that the VM and FSM contents of relations copied with the
+# WAL_LOG strategy are preserved on a standby.
+#
+# The copy WAL-logs each page via log_newpage_buffer(). VM and FSM
+# pages never maintain pd_lower/pd_upper after PageInit(), so logging
+# them with page_std=true would omit their whole content area from the
+# FPI and replay would zero-fill it. When the promoted standby later
+# vacuums the table, replaying the VM-setting PRUNE records on the old
+# primary (where the bits are already set) would be a no-op that leaves
+# the VM buffer clean, tripping Assert(BufferIsDirty(vmbuffer)).
+
+# full_page_writes=off guarantees that the VM-setting record generated
+# below carries no FPI; the large checkpoint_timeout prevents WAL writes
+# while the old primary is demoted, so no rewind is needed then.
+my $node_primary = PostgreSQL::Test::Cluster->new('primary');
+$node_primary->init(allows_streaming => 1);
+$node_primary->append_conf(
+ 'postgresql.conf', qq{
+full_page_writes = off
+autovacuum = off
+checkpoint_timeout = 1h
+});
+$node_primary->start;
+
+# Create a source database with a table whose VM bits are all set
+# (all-visible and all-frozen) and whose FSM is filled.
+$node_primary->safe_psql('postgres', "CREATE DATABASE src_db");
+$node_primary->safe_psql(
+ 'src_db', qq{
+CREATE EXTENSION pageinspect;
+CREATE TABLE test (a int);
+INSERT INTO test SELECT generate_series(1, 10000);
+VACUUM FREEZE test;
+});
+
+# Check that the VM page has real content beyond the 24-byte page
+# header, so that the comparison below is meaningful.
+my $vm_hex = $node_primary->safe_psql('src_db',
+ "SELECT encode(get_raw_page('test', 'vm', 0), 'hex')");
+like(substr($vm_hex, 48),
+ qr/[^0]/, 'VM bits are set on src_db before CREATE DATABASE');
+
+# Flush and take a base backup; the backup copies the VM/FSM files
+# verbatim.
+$node_primary->safe_psql('postgres', "CHECKPOINT");
+my $backup_name = 'my_backup';
+$node_primary->backup($backup_name);
+my $node_standby = PostgreSQL::Test::Cluster->new('standby');
+$node_standby->init_from_backup($node_primary, $backup_name,
+ has_streaming => 1);
+$node_standby->start;
+
+# Create dst_db from src_db: the primary lands the copied forks on
+# disk, while the standby replays the FPIs.
+$node_primary->safe_psql('postgres',
+ "CREATE DATABASE dst_db TEMPLATE src_db STRATEGY WAL_LOG");
+$node_primary->wait_for_catchup($node_standby);
+
+# Compare the VM and FSM pages with pageinspect.
+my $p_vm = $node_primary->safe_psql('dst_db',
+ "SELECT encode(get_raw_page('test', 'vm', 0), 'hex')");
+my $s_vm = $node_standby->safe_psql('dst_db',
+ "SELECT encode(get_raw_page('test', 'vm', 0), 'hex')");
+is($s_vm, $p_vm, 'new database: VM page identical on both nodes');
+
+my $p_fsm = $node_primary->safe_psql('dst_db',
+ "SELECT encode(get_raw_page('test', 'fsm', 0), 'hex')");
+my $s_fsm = $node_standby->safe_psql('dst_db',
+ "SELECT encode(get_raw_page('test', 'fsm', 0), 'hex')");
+is($s_fsm, $p_fsm, 'new database: FSM page identical on both nodes');
+
+# Switch over and check that the old primary survives replaying the
+# PRUNE records generated by vacuuming the table on the new primary.
+
+# Move the redo point past the CREATE DATABASE records, so that crash
+# recovery of the old primary does not replay the CREATE DATABASE FPIs,
+# and wait until the standby has replayed the checkpoint.
+$node_primary->safe_psql('postgres', "CHECKPOINT");
+$node_primary->wait_for_catchup($node_standby);
+
+# Promote the standby, then demote the old primary to a standby of the
+# new primary: its WAL ends exactly at the promotion fork point, so no
+# rewind is needed. The slot preserves the WAL generated below.
+$node_standby->promote;
+
+$node_primary->stop('immediate');
+$node_standby->safe_psql('postgres',
+ "SELECT pg_create_physical_replication_slot('old_primary')");
+my $new_primary_connstr = $node_standby->connstr;
+$node_primary->append_conf(
+ 'postgresql.conf', qq{
+primary_conninfo = '$new_primary_connstr application_name=old_primary'
+primary_slot_name = 'old_primary'
+});
+$node_primary->set_standby_mode;
+$node_primary->start;
+
+# Wait until the new standby is fully caught up.
+$node_standby->wait_for_catchup($node_primary);
+
+# Note the log offset now: the crash can follow the VACUUM within
+# milliseconds.
+my $log_offset = -s $node_primary->logfile;
+
+# If the VM bitmap is all zero on the new primary, vacuum sets the bits
+# again, generating PRUNE records without an FPI for the VM buffer.
+$node_standby->safe_psql('dst_db',
+ "VACUUM (FREEZE, DISABLE_PAGE_SKIPPING) test");
+
+# Replay of the PRUNE records on the new standby would then be a no-op
+# that leaves the VM buffer clean and crashes on
+# Assert(BufferIsDirty(vmbuffer)), in which case the standby never
+# replays past the target LSN and this poll times out. The crash
+# follows the VACUUM within milliseconds, so a short timeout suffices.
+my $target_lsn = $node_standby->lsn('flush');
+my $saved_timeout = $PostgreSQL::Test::Utils::timeout_default;
+$PostgreSQL::Test::Utils::timeout_default = 15;
+my $caught_up = $node_standby->poll_query_until(
+ 'postgres',
+ "SELECT '$target_lsn' <= replay_lsn AND state = 'streaming'
+ FROM pg_catalog.pg_stat_replication
+ WHERE application_name = 'old_primary'");
+$PostgreSQL::Test::Utils::timeout_default = $saved_timeout;
+ok($caught_up, 'new standby catches up after replaying PRUNE records');
+ok(!$node_primary->log_contains(qr/TRAP:.*BufferIsDirty/, $log_offset),
+ 'no assertion crash during replay');
+
+$node_primary->stop;
+
done_testing();
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-08-31 05:37 Michael Paquier <michael@paquier.xyz>
parent: Rogers Wang <rogers.ww@qq.com>
0 siblings, 1 reply; 17+ messages in thread
From: Michael Paquier @ 2026-08-31 05:37 UTC (permalink / raw)
To: Rogers Wang <rogers.ww@qq.com>; +Cc: pgsql-hackers@lists.postgresql.org, Matthias van de Meent <boekewurm+postgres@gmail.com>; Álvaro Herrera <alvherre@kurilemu.de>; melanieplageman@gmail.com
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
Attachments:
[application/pgp-signature] signature.asc (832B, ../../apUTKUUvYfG3SlBF@paquier.xyz/2-signature.asc)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-08-31 15:46 Melanie Plageman <melanieplageman@gmail.com>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 3 replies; 17+ messages in thread
From: Melanie Plageman @ 2026-08-31 15:46 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Rogers Wang <rogers.ww@qq.com>; pgsql-hackers@lists.postgresql.org, Matthias van de Meent <boekewurm+postgres@gmail.com>; Álvaro Herrera <alvherre@kurilemu.de>; Andres Freund <andres@anarazel.de>
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-08-31 16:08 Matthias van de Meent <boekewurm+postgres@gmail.com>
parent: Melanie Plageman <melanieplageman@gmail.com>
2 siblings, 1 reply; 17+ messages in thread
From: Matthias van de Meent @ 2026-08-31 16:08 UTC (permalink / raw)
To: Melanie Plageman <melanieplageman@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Rogers Wang <rogers.ww@qq.com>; pgsql-hackers@lists.postgresql.org, Álvaro Herrera <alvherre@kurilemu.de>; Andres Freund <andres@anarazel.de>
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)
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-09-11 20:44 Melanie Plageman <melanieplageman@gmail.com>
parent: Melanie Plageman <melanieplageman@gmail.com>
2 siblings, 1 reply; 17+ messages in thread
From: Melanie Plageman @ 2026-09-11 20:44 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Rogers Wang <rogers.ww@qq.com>; pgsql-hackers@lists.postgresql.org, Matthias van de Meent <boekewurm+postgres@gmail.com>; Álvaro Herrera <alvherre@kurilemu.de>; Andres Freund <andres@anarazel.de>
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
Attachments:
[text/x-patch] v1-0001-Handle-no-op-visibility-map-setting-during-redo.patch (6.9K, ../../CAAKRu_aVpFUO=3EL3bUutPeKvrhojg_wSZ-jpAE3D1onaNOMPw@mail.gmail.com/2-v1-0001-Handle-no-op-visibility-map-setting-during-redo.patch)
download | inline diff:
From 4781d34b85a55fca467ebe6ae46d11f790be5bb8 Mon Sep 17 00:00:00 2001
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Fri, 11 Sep 2026 16:33:56 -0400
Subject: [PATCH v1] Handle no-op visibility map setting during redo
add323da40a consolidated visibility map updates into
XLOG_HEAP2_PRUNE_FREEZE records. It assumed that a record containing a
visibility map update would always modify the VM page during replay.
That is not true when the requested bits are already set on the
standby.
Primary and standby visibility maps can diverge for several reasons.
This issue was exposed by a CREATE DATABASE ... STRATEGY WAL_LOG bug.
After a standby with an out-of-date VM is promoted, VACUUM may set bits
that were already present on the former primary. When the former primary
later replays that record as a standby, visibilitymap_set() is a no-op.
Restore visibilitymap_set()'s former API, which returns the state of the
VM bits before setting the requested flags. Use that result to set the
VM page LSN only when the operation actually modifies the page.
---
src/backend/access/heap/heapam.c | 10 +++++-----
src/backend/access/heap/heapam_xlog.c | 18 ++++++------------
src/backend/access/heap/pruneheap.c | 5 +++--
src/backend/access/heap/vacuumlazy.c | 16 ++++++++--------
src/backend/access/heap/visibilitymap.c | 6 +++++-
src/include/access/visibilitymap.h | 6 +++---
6 files changed, 30 insertions(+), 31 deletions(-)
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 10766d330a9..b9bf47fd798 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2484,11 +2484,11 @@ heap_multi_insert(Relation relation, TupleTableSlot **slots, int ntuples,
{
PageSetAllVisible(page);
PageClearPrunable(page);
- visibilitymap_set(BufferGetBlockNumber(buffer),
- vmbuffer,
- VISIBILITYMAP_ALL_VISIBLE |
- VISIBILITYMAP_ALL_FROZEN,
- relation->rd_locator);
+ (void) visibilitymap_set(BufferGetBlockNumber(buffer),
+ vmbuffer,
+ VISIBILITYMAP_ALL_VISIBLE |
+ VISIBILITYMAP_ALL_FROZEN,
+ relation->rd_locator);
}
/*
diff --git a/src/backend/access/heap/heapam_xlog.c b/src/backend/access/heap/heapam_xlog.c
index 7a7bc7ea740..c66b30f4c58 100644
--- a/src/backend/access/heap/heapam_xlog.c
+++ b/src/backend/access/heap/heapam_xlog.c
@@ -283,10 +283,8 @@ heap_xlog_prune_freeze(XLogReaderState *record)
if (PageIsNew(vmpage))
PageInit(vmpage, BLCKSZ, 0);
- visibilitymap_set(blkno, vmbuffer, vmflags, rlocator);
-
- Assert(BufferIsDirty(vmbuffer));
- PageSetLSN(vmpage, lsn);
+ if (visibilitymap_set(blkno, vmbuffer, vmflags, rlocator) != vmflags)
+ PageSetLSN(vmpage, lsn);
}
if (BufferIsValid(vmbuffer))
@@ -692,19 +690,15 @@ heap_xlog_multi_insert(XLogReaderState *record)
&vmbuffer) == BLK_NEEDS_REDO)
{
Page vmpage = BufferGetPage(vmbuffer);
+ uint8 vmflags = VISIBILITYMAP_ALL_VISIBLE |
+ VISIBILITYMAP_ALL_FROZEN;
/* initialize the page if it was read as zeros */
if (PageIsNew(vmpage))
PageInit(vmpage, BLCKSZ, 0);
- visibilitymap_set(blkno,
- vmbuffer,
- VISIBILITYMAP_ALL_VISIBLE |
- VISIBILITYMAP_ALL_FROZEN,
- rlocator);
-
- Assert(BufferIsDirty(vmbuffer));
- PageSetLSN(vmpage, lsn);
+ if (visibilitymap_set(blkno, vmbuffer, vmflags, rlocator) != vmflags)
+ PageSetLSN(vmpage, lsn);
}
if (BufferIsValid(vmbuffer))
diff --git a/src/backend/access/heap/pruneheap.c b/src/backend/access/heap/pruneheap.c
index 29f4722b02d..6f6253d7f66 100644
--- a/src/backend/access/heap/pruneheap.c
+++ b/src/backend/access/heap/pruneheap.c
@@ -1310,8 +1310,9 @@ heap_page_prune_and_freeze(PruneFreezeParams *params,
*/
PageSetAllVisible(prstate.page);
PageClearPrunable(prstate.page);
- visibilitymap_set(prstate.block, prstate.vmbuffer, prstate.new_vmbits,
- prstate.relation->rd_locator);
+ (void) visibilitymap_set(prstate.block, prstate.vmbuffer,
+ prstate.new_vmbits,
+ prstate.relation->rd_locator);
}
MarkBufferDirty(prstate.buffer);
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index 063ef2208de..8e1f660bc2f 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -1964,11 +1964,11 @@ lazy_scan_new_or_empty(LVRelState *vacrel, Buffer buf, BlockNumber blkno,
PageSetAllVisible(page);
PageClearPrunable(page);
- visibilitymap_set(blkno,
- vmbuffer,
- VISIBILITYMAP_ALL_VISIBLE |
- VISIBILITYMAP_ALL_FROZEN,
- vacrel->rel->rd_locator);
+ (void) visibilitymap_set(blkno,
+ vmbuffer,
+ VISIBILITYMAP_ALL_VISIBLE |
+ VISIBILITYMAP_ALL_FROZEN,
+ vacrel->rel->rd_locator);
/*
* Emit WAL for setting PD_ALL_VISIBLE on the heap page and
@@ -2846,9 +2846,9 @@ lazy_vacuum_heap_page(LVRelState *vacrel, BlockNumber blkno, Buffer buffer,
*/
PageSetAllVisible(page);
PageClearPrunable(page);
- visibilitymap_set(blkno,
- vmbuffer, vmflags,
- vacrel->rel->rd_locator);
+ (void) visibilitymap_set(blkno,
+ vmbuffer, vmflags,
+ vacrel->rel->rd_locator);
conflict_xid = newest_live_xid;
}
diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c
index 15bd5cf717c..fe5ce437e1b 100644
--- a/src/backend/access/heap/visibilitymap.c
+++ b/src/backend/access/heap/visibilitymap.c
@@ -255,8 +255,10 @@ visibilitymap_pin_ok(BlockNumber heapBlk, Buffer vmbuf)
* containing heapBlk.
*
* rlocator is used only for debugging messages.
+ *
+ * Returns the state of the page's VM bits before setting flags.
*/
-void
+uint8
visibilitymap_set(BlockNumber heapBlk,
Buffer vmBuf, uint8 flags,
RelFileLocator rlocator)
@@ -299,6 +301,8 @@ visibilitymap_set(BlockNumber heapBlk,
map[mapByte] |= (flags << mapOffset);
MarkBufferDirty(vmBuf);
}
+
+ return status;
}
/*
diff --git a/src/include/access/visibilitymap.h b/src/include/access/visibilitymap.h
index b860c4ef3ca..165efd1c00e 100644
--- a/src/include/access/visibilitymap.h
+++ b/src/include/access/visibilitymap.h
@@ -31,9 +31,9 @@ extern bool visibilitymap_clear(RelFileLocator rlocator, BlockNumber heapBlk,
extern void visibilitymap_pin(Relation rel, BlockNumber heapBlk,
Buffer *vmbuf);
extern bool visibilitymap_pin_ok(BlockNumber heapBlk, Buffer vmbuf);
-extern void visibilitymap_set(BlockNumber heapBlk,
- Buffer vmBuf, uint8 flags,
- RelFileLocator rlocator);
+extern uint8 visibilitymap_set(BlockNumber heapBlk,
+ Buffer vmBuf, uint8 flags,
+ RelFileLocator rlocator);
extern uint8 visibilitymap_get_status(Relation rel, BlockNumber heapBlk, Buffer *vmbuf);
extern void visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen);
extern BlockNumber visibilitymap_prepare_truncate(Relation rel,
--
2.43.0
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-09-14 15:49 Melanie Plageman <melanieplageman@gmail.com>
parent: Melanie Plageman <melanieplageman@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Melanie Plageman @ 2026-09-14 15:49 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Rogers Wang <rogers.ww@qq.com>; pgsql-hackers@lists.postgresql.org, Matthias van de Meent <boekewurm+postgres@gmail.com>; Álvaro Herrera <alvherre@kurilemu.de>; Andres Freund <andres@anarazel.de>
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-09-16 14:26 Melanie Plageman <melanieplageman@gmail.com>
parent: Matthias van de Meent <boekewurm+postgres@gmail.com>
0 siblings, 1 reply; 17+ messages in thread
From: Melanie Plageman @ 2026-09-16 14:26 UTC (permalink / raw)
To: Matthias van de Meent <boekewurm+postgres@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Rogers Wang <rogers.ww@qq.com>; pgsql-hackers@lists.postgresql.org, Álvaro Herrera <alvherre@kurilemu.de>; Andres Freund <andres@anarazel.de>; Robert Haas <robertmhaas@gmail.com>
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-09-16 18:32 Melanie Plageman <melanieplageman@gmail.com>
parent: Melanie Plageman <melanieplageman@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Melanie Plageman @ 2026-09-16 18:32 UTC (permalink / raw)
To: Matthias van de Meent <boekewurm+postgres@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Rogers Wang <rogers.ww@qq.com>; pgsql-hackers@lists.postgresql.org, Álvaro Herrera <alvherre@kurilemu.de>; Andres Freund <andres@anarazel.de>; Robert Haas <robertmhaas@gmail.com>
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
Attachments:
[application/x-shellscript] repro-noop-vm-frozen-lock-portable.sh (9.0K, ../../CAAKRu_ZaPGw8tN1nrSYbW-fUTyTYFWUNZ+r7bttpiWOo6P5X_Q@mail.gmail.com/2-repro-noop-vm-frozen-lock-portable.sh)
download
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-09-16 21:59 Robert Haas <robertmhaas@gmail.com>
parent: Melanie Plageman <melanieplageman@gmail.com>
2 siblings, 1 reply; 17+ messages in thread
From: Robert Haas @ 2026-09-16 21:59 UTC (permalink / raw)
To: Melanie Plageman <melanieplageman@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Rogers Wang <rogers.ww@qq.com>; pgsql-hackers@lists.postgresql.org, Matthias van de Meent <boekewurm+postgres@gmail.com>; Álvaro Herrera <alvherre@kurilemu.de>; Andres Freund <andres@anarazel.de>
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
^ permalink raw reply [nested|flat] 17+ messages in thread
* Re: WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts
@ 2026-09-22 23:12 Melanie Plageman <melanieplageman@gmail.com>
parent: Robert Haas <robertmhaas@gmail.com>
0 siblings, 0 replies; 17+ messages in thread
From: Melanie Plageman @ 2026-09-22 23:12 UTC (permalink / raw)
To: Robert Haas <robertmhaas@gmail.com>; +Cc: Michael Paquier <michael@paquier.xyz>; Rogers Wang <rogers.ww@qq.com>; pgsql-hackers@lists.postgresql.org, Matthias van de Meent <boekewurm+postgres@gmail.com>; Álvaro Herrera <alvherre@kurilemu.de>; Andres Freund <andres@anarazel.de>
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
^ permalink raw reply [nested|flat] 17+ messages in thread
end of thread, other threads:[~2026-09-22 23:12 UTC | newest]
Thread overview: 17+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-05-13 12:31 WAL_LOG CREATE DATABASE strategy broken for non-standard page layouts Matthias van de Meent <boekewurm+postgres@gmail.com>
2024-05-13 14:13 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-05-13 14:52 ` Matthias van de Meent <boekewurm+postgres@gmail.com>
2024-05-13 19:43 ` Robert Haas <robertmhaas@gmail.com>
2024-09-14 18:57 ` Akshat Jaimini <destrex271@gmail.com>
2024-12-10 06:25 ` Michael Paquier <michael@paquier.xyz>
2026-02-02 15:26 ` Álvaro Herrera <alvherre@kurilemu.de>
2026-08-23 17:04 ` Rogers Wang <rogers.ww@qq.com>
2026-08-31 05:37 ` Michael Paquier <michael@paquier.xyz>
2026-08-31 15:46 ` Melanie Plageman <melanieplageman@gmail.com>
2026-08-31 16:08 ` Matthias van de Meent <boekewurm+postgres@gmail.com>
2026-09-16 14:26 ` Melanie Plageman <melanieplageman@gmail.com>
2026-09-16 18:32 ` Melanie Plageman <melanieplageman@gmail.com>
2026-09-11 20:44 ` Melanie Plageman <melanieplageman@gmail.com>
2026-09-14 15:49 ` Melanie Plageman <melanieplageman@gmail.com>
2026-09-16 21:59 ` Robert Haas <robertmhaas@gmail.com>
2026-09-22 23:12 ` Melanie Plageman <melanieplageman@gmail.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox