Hi Sai Pranav, > - Nothing tests the full_copy_possible = false in the new block: with that > line removed, the whole suite still passes. I think the line is needed, > though: without it, a chain where no block comes from an incremental file > and the full file happens to be as long as the latest > truncation_block_length would be copied whole, including the blocks the > intermediate backup had truncated away. I couldn't build that case through > the server (regrowing the relation writes blocks that end up in the later > incremental file), so a test may not be practical. A short comment saying > why the line is there would help. > > - Small wording nit in the new comment: blocks at or beyond an incremental > file's truncation_block_length are not necessarily blocks that "did not > exist at the time of this backup"; they may have been truncated away at > some point during that backup's WAL range. Something like "Blocks at or > beyond this file's truncation_block_length were truncated away at some > point before this backup, so their contents in any earlier backup are > stale" might be clearer.
Thanks for reviewing and testing the patch! Both points make sense. I updated the comment in v3 to clarify the truncation wording and explain why we also clear full_copy_possible there. Regards, Rajesh
From a75c1629d07e6d8a78a6d653377d6127262aa87a Mon Sep 17 00:00:00 2001 From: Rajesh Kumar Thandapani <[email protected]> Date: Wed, 4 Nov 2026 11:41:32 +0000 Subject: [PATCH v3] Fix pg_combinebackup with truncated intermediate backups. When reconstructing a relation from a chain of backups, reconstruct_from_incremental_file() walks backward from the latest incremental backup to the full backup, filling sourcemap[] for any block b < latest_source->truncation_block_length that has not yet been assigned a source. However, if an intermediate incremental backup truncated the relation (s->truncation_block_length < latest_source->truncation_block_length) and a later backup extended the relation again without writing every block in the truncated range (for example, via bulk relation extension), those unwritten blocks still had sourcemap[b] == NULL when the loop moved past the intermediate backup. As a result, they were sourced from an earlier backup taken before the truncation, resurrecting stale rows. Fix by tracking the current truncation_block_length upper bound as we walk backward through the backup chain and tightening it whenever an incremental backup has a smaller s->truncation_block_length. Author: Rajesh Kumar Thandapani <[email protected]> Reviewed-by: Raja Sai Pranav <[email protected]> Bug: #19776 Reported-by: Ke <[email protected]> Discussion: https://postgr.es/m/[email protected] --- src/bin/pg_combinebackup/reconstruct.c | 19 +++++- .../pg_combinebackup/t/011_ib_truncation.pl | 59 ++++++++++++++++++- 2 files changed, 75 insertions(+), 3 deletions(-) diff --git a/src/bin/pg_combinebackup/reconstruct.c b/src/bin/pg_combinebackup/reconstruct.c index 5900c5b..f33ad2a 100644 --- a/src/bin/pg_combinebackup/reconstruct.c +++ b/src/bin/pg_combinebackup/reconstruct.c @@ -105,6 +105,7 @@ reconstruct_from_incremental_file(char *input_filename, rfile **sourcemap; off_t *offsetmap; unsigned block_length; + unsigned truncation_block_length; unsigned sidx = n_prior_backups; bool full_copy_possible = true; int copy_source_index = -1; @@ -128,6 +129,7 @@ reconstruct_from_incremental_file(char *input_filename, latest_source = make_incremental_rfile(input_filename); source[n_prior_backups] = latest_source; block_length = find_reconstructed_block_length(latest_source); + truncation_block_length = latest_source->truncation_block_length; /* * For each block in the output file, we need to know from which file we @@ -224,7 +226,7 @@ reconstruct_from_incremental_file(char *input_filename, * later backup is one that we can't validate. */ blocklength = sb.st_size / BLCKSZ; - for (b = 0; b < latest_source->truncation_block_length; ++b) + for (b = 0; b < truncation_block_length; ++b) { if (sourcemap[b] == NULL && b < blocklength) { @@ -264,7 +266,7 @@ reconstruct_from_incremental_file(char *input_filename, { BlockNumber b = s->relative_block_numbers[i]; - if (b < latest_source->truncation_block_length && + if (b < truncation_block_length && sourcemap[b] == NULL) { sourcemap[b] = s; @@ -278,6 +280,19 @@ reconstruct_from_incremental_file(char *input_filename, full_copy_possible = false; } } + + /* + * Blocks at or beyond this incremental file's truncation_block_length + * were either not present or truncated away before this backup, so + * their contents in any earlier backup are stale and must not be + * used. This also prevents copying an earlier full file whole even if + * its size happens to match latest_source->truncation_block_length. + */ + if (s->truncation_block_length < truncation_block_length) + { + truncation_block_length = s->truncation_block_length; + full_copy_possible = false; + } } /* diff --git a/src/bin/pg_combinebackup/t/011_ib_truncation.pl b/src/bin/pg_combinebackup/t/011_ib_truncation.pl index 1174928..d6d6427 100644 --- a/src/bin/pg_combinebackup/t/011_ib_truncation.pl +++ b/src/bin/pg_combinebackup/t/011_ib_truncation.pl @@ -115,7 +115,64 @@ my $restored_count = is($restored_count, $rows_after_truncation, 'Restored backup has correct row count'); -$primary->stop; $restored->stop; +# Test that blocks truncated in an intermediate backup are not resurrected +# when a later backup bulk-extends the relation with unwritten zero-filled +# pages. Increase shared_buffers so COPY can bulk-extend by multiple blocks +# at once, grow the table to 30 blocks, truncate it in an intermediate +# backup, and then bulk-extend it with COPY. +$primary->append_conf('postgresql.conf', 'shared_buffers = 8MB'); +$primary->restart; + +my $full_rows = 30; +$primary->safe_psql( + 'postgres', + "INSERT INTO t + SELECT i, + repeat('0123456789ABCDEF0123456789ABCDEF', ($block_size / (2*32))) + FROM generate_series($rows_after_truncation + 1, $full_rows) i; + VACUUM t;" +); + +my $incr_backup = "$backup_path/incr"; +$primary->backup('incr2', + backup_options => [ '--incremental', "$incr_backup/backup_manifest" ]); + +$primary->safe_psql('postgres', + "DELETE FROM t WHERE id > $rows_after_truncation; + VACUUM (TRUNCATE) t;"); + +my $incr2_backup = "$backup_path/incr2"; +$primary->backup('incr3', + backup_options => [ '--incremental', "$incr2_backup/backup_manifest" ]); + +my $copy_rows = 17; +my $row_data = '0123456789ABCDEF0123456789ABCDEF' x ($block_size / (2 * 32)); +my $copy_input = "COPY t FROM STDIN;\n"; +for my $i (1 .. $copy_rows) +{ + $copy_input .= ($full_rows + $i) . "\t$row_data\n"; +} +$copy_input .= "\\.\n"; +$primary->safe_psql('postgres', $copy_input); + +my $incr3_backup = "$backup_path/incr3"; +$primary->backup('incr4', + backup_options => [ '--incremental', "$incr3_backup/backup_manifest" ]); + +my $restored2 = PostgreSQL::Test::Cluster->new('node3'); +$restored2->init_from_backup( + $primary, 'incr4', + combine_with_prior => [ 'full', 'incr', 'incr2', 'incr3' ]); +$restored2->start(); + +$restored_count = + $restored2->safe_psql('postgres', "SELECT count(*) FROM t;"); +is($restored_count, $rows_after_truncation + $copy_rows, + 'Restored backup after intermediate truncation and bulk extension has correct row count'); + +$primary->stop; +$restored2->stop; + done_testing(); -- 2.56.0.398.g882cbad275-goog
