Hi Rajesh, I reviewed and tested v2.
v2 applies cleanly to master (316861101e8). Linux aarch64, --enable-cassert --enable-debug --enable-tap-tests, no warnings. - With master's reconstruct.c, the new 011_ib_truncation test fails 3 of 3 (restored count 25, expected 24). With the patch it passes 3 of 3, and the whole pg_combinebackup suite passes. - On a build with --with-segsize-blocks=6 (one of the CI configurations), the new test passes even without the fix, 3 of 3. There the truncation empties whole segments: in incr3, 16384.2 to 16384.4 are zero-length full files, so for the segment that COPY later re-extends (INCREMENTAL.16384.4 in incr4, no blocks) the walk back stops at incr3's empty full file and never reaches the stale copy. I would expect it to show only when the truncation point falls inside the segment that is re-extended (I did not try to build that case). The fix itself is fine there (the suite passes); only the test can't see the bug in that configuration. Probably acceptable, but worth knowing. - The logic looks right to me. The bound only tightens as we walk back, it is applied after the current file's own blocks are taken (so blocks the intermediate backup itself re-extended past its truncation_block_length are still used), and it is only applied to incremental files. - 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. Regards, Sai Pranav On Mon, Nov 02, 2026 04:32 PM, Rajesh Kumar Thandapani < [email protected]> wrote: > Hi all, > > Attaching a rebased v2 of the patch so it applies cleanly against > current master for cfbot. No functional changes from v1. > > Thanks, > Rajesh >
