Re: Re: BUG #19776: pg_combinebackup ignores the truncation_block_length of an intermediate incremental backup

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 <
rajeshkumarth(at)google(dot)com> 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
>