From ffc226ab64d4ebdb089c278396d5df3d0a3f83b9 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Mon, 9 Mar 2026 06:36:42 -0400 Subject: [PATCH] Prevent restore of incremental backup from bloating VM fork. When I (rhaas) wrote the WAL summarizer code, I incorrectly believed that XLOG_SMGR_TRUNCATE truncates all forks to the same length. In fact, what other parts of the code do is compute the truncation length for the FSM and VM forks from the truncation length used for the main fork. But, because I was confused, I coded the WAL summarizer to set the limit block for the VM fork to the same value as for the main fork. (Incremental backup always copies FSM forks in full, so there is no similar issue in that case.) Doing that doesn't directly cause any data corruption, as far as I can see. However, it does create a serious risk of consuming a large amount of extra disk space, because pg_combinebackup's reconstruct.c believes that the reconstructed file should always be at least as long as the limit block value. We might want to be smarter about that at some point in the future, because it's always safe to omit all-zeroes blocks at the end of the last segment of a relation, and doing so could save disk space, but the current algorithm will rarely waste enough disk space to worry about unless we believe that a relation has been truncated to a length much longer than its actual length on disk, which is exactly what happens as a result of the problem mentioned in the previous paragraph. To fix, create a new visibilitymap helper function and use it to include the right limit block in the summary files. Incremental backups taken with existing summary files will still have this issue, but this should improve the situation going forward. Diagnosed-by: Oleg Tkachenko Diagnosed-by: Amul Sul Discussion: http://postgr.es/m/CAAJ_b97PqG89hvPNJ8cGwmk94gJ9KOf_pLsowUyQGZgJY32o9g@mail.gmail.com Discussion: http://postgr.es/m/6897DAF7-B699-41BF-A6FB-B818FCFFD585%40gmail.com Backpatch-through: 17 --- src/backend/access/heap/visibilitymap.c | 17 +++++++++++++ src/backend/postmaster/walsummarizer.c | 4 +++- .../pg_combinebackup/t/011_ib_truncation.pl | 24 ++++++++++++++++--- src/include/access/visibilitymap.h | 1 + 4 files changed, 42 insertions(+), 4 deletions(-) diff --git a/src/backend/access/heap/visibilitymap.c b/src/backend/access/heap/visibilitymap.c index 3047bd46def..e21b96281a6 100644 --- a/src/backend/access/heap/visibilitymap.c +++ b/src/backend/access/heap/visibilitymap.c @@ -116,6 +116,8 @@ /* Mapping from heap block number to the right bit in the visibility map */ #define HEAPBLK_TO_MAPBLOCK(x) ((x) / HEAPBLOCKS_PER_PAGE) +#define HEAPBLK_TO_MAPBLOCK_LIMIT(x) \ + (((x) + HEAPBLOCKS_PER_PAGE - 1) / HEAPBLOCKS_PER_PAGE) #define HEAPBLK_TO_MAPBYTE(x) (((x) % HEAPBLOCKS_PER_PAGE) / HEAPBLOCKS_PER_BYTE) #define HEAPBLK_TO_OFFSET(x) (((x) % HEAPBLOCKS_PER_BYTE) * BITS_PER_HEAPBLOCK) @@ -600,6 +602,21 @@ visibilitymap_prepare_truncate(Relation rel, BlockNumber nheapblocks) return newnblocks; } +/* + * visibilitymap_truncation_length - + * compute truncation length for visibility map + * + * Given a proposed truncation length for the main fork, compute the + * correct truncation length for the visibility map. Should return the + * same answer as visibilitymap_prepare_truncate(), but without modifying + * anything. + */ +BlockNumber +visibilitymap_truncation_length(BlockNumber nheapblocks) +{ + return HEAPBLK_TO_MAPBLOCK_LIMIT(nheapblocks); +} + /* * Read a visibility map page. * diff --git a/src/backend/postmaster/walsummarizer.c b/src/backend/postmaster/walsummarizer.c index 742137edad6..e1aa102f41d 100644 --- a/src/backend/postmaster/walsummarizer.c +++ b/src/backend/postmaster/walsummarizer.c @@ -23,6 +23,7 @@ #include "postgres.h" #include "access/timeline.h" +#include "access/visibilitymap.h" #include "access/xlog.h" #include "access/xlog_internal.h" #include "access/xlogrecovery.h" @@ -1351,7 +1352,8 @@ SummarizeSmgrRecord(XLogReaderState *xlogreader, BlockRefTable *brtab) MAIN_FORKNUM, xlrec->blkno); if ((xlrec->flags & SMGR_TRUNCATE_VM) != 0) BlockRefTableSetLimitBlock(brtab, &xlrec->rlocator, - VISIBILITYMAP_FORKNUM, xlrec->blkno); + VISIBILITYMAP_FORKNUM, + visibilitymap_truncation_length(xlrec->blkno)); } } diff --git a/src/bin/pg_combinebackup/t/011_ib_truncation.pl b/src/bin/pg_combinebackup/t/011_ib_truncation.pl index 47d84434452..c5e0124c04d 100644 --- a/src/bin/pg_combinebackup/t/011_ib_truncation.pl +++ b/src/bin/pg_combinebackup/t/011_ib_truncation.pl @@ -1,7 +1,8 @@ # Copyright (c) 2025-2026, PostgreSQL Global Development Group # -# This test aims to validate that the calculated truncation block never exceeds -# the segment size. +# This test aims to validate two things: (1) that the calculated truncation +# block never exceeds the segment size and (2) that the correct limit block +# length is calculated for the VM fork. use strict; use warnings FATAL => 'all'; @@ -39,7 +40,7 @@ $primary->safe_psql( CREATE TABLE t ( id int, data text STORAGE PLAIN - ); + ) WITH (autovacuum_enabled = false); }); # The tuple size should be enough to prevent two tuples from being on the same @@ -83,6 +84,23 @@ cmp_ok($t_blocks, '>', $target_blocks, $primary->backup('incr', backup_options => [ '--incremental', "$full_backup/backup_manifest" ]); +# We used to have a bug where the wrong limit block was calculated for the +# VM fork, so verify that the WAL summary records the correct VM fork +# truncation limit. We can't just check whether the restored VM fork is +# the right size on disk, because it's so small that the incremental backup +# code will send the entire file. +my $relfilenode = $primary->safe_psql('postgres', + "SELECT pg_relation_filenode('t');"); +my $vm_limits = $primary->safe_psql('postgres', + "SELECT string_agg(relblocknumber::text, ',') + FROM pg_available_wal_summaries() s, + pg_wal_summary_contents(s.tli, s.start_lsn, s.end_lsn) c + WHERE c.relfilenode = $relfilenode + AND c.relforknumber = 2 + AND c.is_limit_block;"); +is($vm_limits, '1', + 'WAL summary has correct VM fork truncation limit'); + # Combine full and incremental backups. Before the fix, this failed because # the INCREMENTAL file header contained an incorrect truncation_block value. my $restored = PostgreSQL::Test::Cluster->new('node2'); diff --git a/src/include/access/visibilitymap.h b/src/include/access/visibilitymap.h index a0166c5b410..52cde56be86 100644 --- a/src/include/access/visibilitymap.h +++ b/src/include/access/visibilitymap.h @@ -45,5 +45,6 @@ extern uint8 visibilitymap_get_status(Relation rel, BlockNumber heapBlk, Buffer extern void visibilitymap_count(Relation rel, BlockNumber *all_visible, BlockNumber *all_frozen); extern BlockNumber visibilitymap_prepare_truncate(Relation rel, BlockNumber nheapblocks); +extern BlockNumber visibilitymap_truncation_length(BlockNumber nheapblocks); #endif /* VISIBILITYMAP_H */