From 51e932eae4582c88783b0e66bc950599fb35ea9c Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Tue, 10 Nov 2026 14:47:58 -0500 Subject: [PATCH v2 1/2] Fix WAL-logged CREATE DATABASE with nonstandard pages RelationCopyStorageUsingBuffer() passed page_std=true when WAL-logging every copied relation fork. This allowed WAL insertion to omit the region between pd_lower and pd_upper from full-page images and restore it as zeroes during replay. That is safe only for pages using the standard page layout. On nonstandard pages, the apparent hole can contain meaningful data, causing the primary and standby to diverge. Known affected cases include free space maps, visibility maps, and old GIN indexes, though any access method using nonstandard pages could be affected. Losing VM contents can lead to data corruption after promotion. For example, a tuple lock on a promoted standby may find the all-frozen bit already clear and therefore emit no WAL to clear it. The old primary then replays the xmax change without clearing the all-frozen bit, retaining the stale bit. This can cause vacuum failures and make invisible data visible again. Only remove the apparent free-space region from WAL when copying a heap table or B-tree index, whose pages are known to use the standard layout. For other relation types, remove the region only when it contains all zeroes, ensuring that replay reconstructs the page without data loss. Use a shared bounds helper so that RelationCopyStorageUsingBuffer() checks the same region that XLogRecordAssemble() omits. This may increase WAL volume for access methods other than heap and B-tree when the space between pd_lower and pd_upper contains stale data that was previously omitted. Such access methods can preserve hole removal by zeroing that space. Reported-by: Matthias van de Meent Reported-by: Rogers Wang Reviewed-by: Robert Haas Reviewed-by: surya poondla Author: Melanie Plageman Discussion: https://postgr.es/m/flat/CAEze2WgNjODiAiTU18E06_BrRK0nqwJ1kdS_bN61c2_DpFDSnQ%40mail.gmail.com Backpatch-through: 15 --- src/backend/access/transam/xloginsert.c | 13 +++---- src/backend/commands/dbcommands.c | 18 ++++++++- src/backend/storage/buffer/bufmgr.c | 37 +++++++++++++++---- src/include/storage/bufmgr.h | 3 +- src/include/storage/bufpage.h | 14 +++++++ src/test/recovery/Makefile | 1 + src/test/recovery/t/034_create_database.pl | 43 +++++++++++++++++++--- 7 files changed, 106 insertions(+), 23 deletions(-) diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c index 341deb7b80d..251688e37fd 100644 --- a/src/backend/access/transam/xloginsert.c +++ b/src/backend/access/transam/xloginsert.c @@ -740,16 +740,13 @@ XLogRecordAssemble(RmgrId rmid, uint8 info, */ if (regbuf->flags & REGBUF_STANDARD) { - /* Assume we can omit data between pd_lower and pd_upper */ - uint16 lower = ((const PageHeaderData *) page)->pd_lower; - uint16 upper = ((const PageHeaderData *) page)->pd_upper; + const PageHeaderData *phdr = (const PageHeaderData *) page; - if (lower >= SizeOfPageHeaderData && - upper > lower && - upper <= BLCKSZ) + /* Assume we can omit data between pd_lower and pd_upper */ + if (PageFreeSpaceBoundsAreValid(page)) { - bimg.hole_offset = lower; - cbimg.hole_length = upper - lower; + bimg.hole_offset = phdr->pd_lower; + cbimg.hole_length = phdr->pd_upper - phdr->pd_lower; } else { diff --git a/src/backend/commands/dbcommands.c b/src/backend/commands/dbcommands.c index 7e3fc59eafd..f514579e8fd 100644 --- a/src/backend/commands/dbcommands.c +++ b/src/backend/commands/dbcommands.c @@ -36,6 +36,7 @@ #include "catalog/dependency.h" #include "catalog/indexing.h" #include "catalog/objectaccess.h" +#include "catalog/pg_am_d.h" #include "catalog/pg_authid.h" #include "catalog/pg_collation.h" #include "catalog/pg_database.h" @@ -110,6 +111,7 @@ typedef struct CreateDBRelInfo RelFileLocator rlocator; /* physical relation identifier */ Oid reloid; /* relation oid */ bool permanent; /* relation is permanent or unlogged */ + bool mainfork_page_std; /* main fork has standard page layout */ } CreateDBRelInfo; @@ -217,7 +219,8 @@ CreateDatabaseUsingWalLog(Oid src_dboid, Oid dst_dboid, LockRelationId(&dstrelid, AccessShareLock); /* Copy relation storage from source to the destination. */ - CreateAndCopyRelationData(srcrlocator, dstrlocator, relinfo->permanent); + CreateAndCopyRelationData(srcrlocator, dstrlocator, relinfo->permanent, + relinfo->mainfork_page_std); /* Release the relation locks. */ UnlockRelationId(&srcrelid, AccessShareLock); @@ -449,6 +452,19 @@ ScanSourceDatabasePgClassTuple(HeapTupleData *tuple, Oid tbid, Oid dbid, relinfo->permanent = (classForm->relpersistence == RELPERSISTENCE_PERMANENT) ? true : false; + /* + * Heap tables and B-tree indexes are common cases whose main forks are + * known to use the standard page layout. Other access methods may do so + * as well, but some, such as GIN, have legacy page formats that differ + * from their current format. Keep the classification conservative by + * limiting it to heap and B-tree. + */ + relinfo->mainfork_page_std = + (RELKIND_HAS_TABLE_AM(classForm->relkind) && + classForm->relam == HEAP_TABLE_AM_OID) || + (classForm->relkind == RELKIND_INDEX && + classForm->relam == BTREE_AM_OID); + return relinfo; } diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c index c17fc3590f0..4dbc25c6c50 100644 --- a/src/backend/storage/buffer/bufmgr.c +++ b/src/backend/storage/buffer/bufmgr.c @@ -65,6 +65,7 @@ #include "storage/smgr.h" #include "storage/standby.h" #include "utils/memdebug.h" +#include "utils/memutils.h" #include "utils/ps_status.h" #include "utils/rel.h" #include "utils/resowner.h" @@ -666,7 +667,8 @@ static void FindAndDropRelationBuffers(RelFileLocator rlocator, BlockNumber firstDelBlock); static void RelationCopyStorageUsingBuffer(RelFileLocator srclocator, RelFileLocator dstlocator, - ForkNumber forkNum, bool permanent); + ForkNumber forkNum, bool permanent, + bool page_std); static void AtProcExit_Buffers(int code, Datum arg); static void CheckForBufferLeaks(void); #ifdef USE_ASSERT_CHECKING @@ -5376,7 +5378,8 @@ FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels) static void RelationCopyStorageUsingBuffer(RelFileLocator srclocator, RelFileLocator dstlocator, - ForkNumber forkNum, bool permanent) + ForkNumber forkNum, bool permanent, + bool page_std) { Buffer srcBuf; Buffer dstBuf; @@ -5462,7 +5465,24 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator, /* WAL-log the copied page. */ if (use_wal) - log_newpage_buffer(dstBuf, true); + { + bool allow_page_std = page_std; + + /* + * For page layouts we don't recognize, hole removal is still safe + * if the exact range that WAL would omit contains only zeroes. + */ + if (!allow_page_std && PageFreeSpaceBoundsAreValid(dstPage)) + { + PageHeader phdr = (PageHeader) dstPage; + + allow_page_std = + pg_memory_is_all_zeros(dstPage + phdr->pd_lower, + phdr->pd_upper - phdr->pd_lower); + } + + log_newpage_buffer(dstBuf, allow_page_std); + } END_CRIT_SECTION(); @@ -5484,12 +5504,15 @@ RelationCopyStorageUsingBuffer(RelFileLocator srclocator, * * Pass permanent as true for permanent relations and false for * unlogged relations. Currently this API is not supported for - * temporary relations. + * temporary relations. mainfork_page_std indicates whether the main + * fork uses the standard page layout; all other forks are treated as + * non-standard. * -------------------------------------------------------------------- */ void CreateAndCopyRelationData(RelFileLocator src_rlocator, - RelFileLocator dst_rlocator, bool permanent) + RelFileLocator dst_rlocator, bool permanent, + bool mainfork_page_std) { char relpersistence; SMgrRelation src_rel; @@ -5512,7 +5535,7 @@ CreateAndCopyRelationData(RelFileLocator src_rlocator, /* copy main fork. */ RelationCopyStorageUsingBuffer(src_rlocator, dst_rlocator, MAIN_FORKNUM, - permanent); + permanent, mainfork_page_std); /* copy those extra forks that exist */ for (ForkNumber forkNum = MAIN_FORKNUM + 1; @@ -5531,7 +5554,7 @@ CreateAndCopyRelationData(RelFileLocator src_rlocator, /* Copy a fork's data, block by block. */ RelationCopyStorageUsingBuffer(src_rlocator, dst_rlocator, forkNum, - permanent); + permanent, false); } } } diff --git a/src/include/storage/bufmgr.h b/src/include/storage/bufmgr.h index 6837b35fc6d..b8e9b7f1093 100644 --- a/src/include/storage/bufmgr.h +++ b/src/include/storage/bufmgr.h @@ -297,7 +297,8 @@ extern void FlushRelationBuffers(Relation rel); extern void FlushRelationsAllBuffers(SMgrRelation *smgrs, int nrels); extern void CreateAndCopyRelationData(RelFileLocator src_rlocator, RelFileLocator dst_rlocator, - bool permanent); + bool permanent, + bool mainfork_page_std); extern void FlushDatabaseBuffers(Oid dbid); extern void DropRelationBuffers(SMgrRelation smgr_reln, ForkNumber *forkNum, diff --git a/src/include/storage/bufpage.h b/src/include/storage/bufpage.h index 713b6222286..2fe9085f881 100644 --- a/src/include/storage/bufpage.h +++ b/src/include/storage/bufpage.h @@ -241,6 +241,20 @@ typedef PageHeaderData *PageHeader; */ #define SizeOfPageHeaderData (offsetof(PageHeaderData, pd_linp)) +/* + * Returns true if the page header describes a non-empty, in-bounds free space + * range. This does not prove that the page uses the standard page layout. + */ +static inline bool +PageFreeSpaceBoundsAreValid(const PageData *page) +{ + const PageHeaderData *phdr = (const PageHeaderData *) page; + + return phdr->pd_lower >= SizeOfPageHeaderData && + phdr->pd_upper > phdr->pd_lower && + phdr->pd_upper <= BLCKSZ; +} + /* * PageIsEmpty * returns true iff no itemid has been allocated on the page diff --git a/src/test/recovery/Makefile b/src/test/recovery/Makefile index 9c4102b6b2c..e90e0b1ad4c 100644 --- a/src/test/recovery/Makefile +++ b/src/test/recovery/Makefile @@ -11,6 +11,7 @@ EXTRA_INSTALL=contrib/pg_prewarm \ contrib/pg_stat_statements \ + contrib/pg_visibility \ contrib/test_decoding \ src/test/modules/injection_points \ src/test/modules/test_wait_lsn diff --git a/src/test/recovery/t/034_create_database.pl b/src/test/recovery/t/034_create_database.pl index 194d1837e23..a5c1ff228ba 100644 --- a/src/test/recovery/t/034_create_database.pl +++ b/src/test/recovery/t/034_create_database.pl @@ -10,19 +10,34 @@ use PostgreSQL::Test::Utils; use Test::More; my $node = PostgreSQL::Test::Cluster->new('node'); -$node->init; +$node->init(allows_streaming => 1); $node->start; +$node->backup('backup'); +my $standby = PostgreSQL::Test::Cluster->new('standby'); +$standby->init_from_backup($node, 'backup', has_streaming => 1); +$standby->start; + # This checks that any DDLs run on the template database that modify pg_class # are persisted after creating a database from it using the WAL_LOG strategy, # as a direct copy of the template database's pg_class is used in this case. my $db_template = "template1"; my $db_new = "test_db_1"; -# Create table. It should persist on the template database. +# Create a table with a populated visibility map in the template database. +$node->safe_psql($db_template, q{ + CREATE TABLE tab_db_before_create_1 AS + SELECT generate_series(1, 10000) AS a; + VACUUM (FREEZE) tab_db_before_create_1; +}); + +# Copy the template database. $node->safe_psql("postgres", "CREATE DATABASE $db_new STRATEGY WAL_LOG TEMPLATE $db_template;"); +$node->safe_psql($db_new, "CREATE EXTENSION pg_visibility;"); +# This table should persist in the template database but not be present in the +# copy made above. $node->safe_psql($db_template, "CREATE TABLE tab_db_after_create_1 (a INT);"); # Flush the changes affecting the template database, then replay them. @@ -31,15 +46,31 @@ $node->safe_psql("postgres", "CHECKPOINT;"); $node->stop('immediate'); $node->start; my $result = $node->safe_psql($db_template, - "SELECT count(*) FROM pg_class WHERE relname LIKE 'tab_db_%';"); + "SELECT count(*) FROM pg_class WHERE relname = 'tab_db_after_create_1';"); is($result, "1", "check that table exists on template after crash, with checkpoint"); -# The new database should have no tables. +# The new database should not have the table created after it was copied. $result = $node->safe_psql($db_new, - "SELECT count(*) FROM pg_class WHERE relname LIKE 'tab_db_%';"); + "SELECT count(*) FROM pg_class WHERE relname = 'tab_db_after_create_1';"); is($result, "0", - "check that there are no tables from template on new database after crash" + "check that table created after database copy is absent after crash" ); +$node->wait_for_replay_catchup($standby); + +my $visibility_map_query = q{ + SELECT blkno, all_visible, all_frozen + FROM pg_visibility_map('tab_db_before_create_1') + ORDER BY blkno; +}; +my $primary_visibility_map = + $node->safe_psql($db_new, $visibility_map_query); +my $standby_visibility_map = + $standby->safe_psql($db_new, $visibility_map_query); + +isnt($primary_visibility_map, '', 'copied table has visibility map entries'); +is($standby_visibility_map, $primary_visibility_map, + "copied table visibility map matches standby"); + done_testing(); -- 2.43.0