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

Hi Rajesh,

Thanks - v3 addresses both points from the v2 review.

- v3 applies cleanly to current master (72b9498df9a), including past
fcfd2456078 ("Reject out-of-range block numbers in incremental files"),
which also touches reconstruct.c.
- The reconstruct.c change is the same logic I tested on v2 (track the
running
truncation_block_length upper bound, tighten it on each intermediate backup
with a smaller s->truncation_block_length, applied after the file's own
blocks and only to incremental files, and clear full_copy_possible there).
In my v2 testing that fixed the 011 test (which failed 3/3 on master without
it).
- v3 now adds the bulk-extension regression test I'd flagged as missing: the
node3 / incr2-incr4 chain that bumps shared_buffers so COPY bulk-extends the
relation past an intermediate truncation point, leaving zero-filled
unwritten
pages in the truncated range. That exercises exactly the full_copy_possible
=
false / "unwritten blocks sourced from a pre-truncation backup" path, which
is what the fix guards.
- The reworded comment reads clearly now.

LGTM.

(Note on testing: I re-confirmed v3 applies to current master and re-read
the
diff; the reconstruct.c fix is unchanged from the v2 I built and tested. I
did
not re-run the TAP suite on v3 this round - my macOS TAP environment aborts
in
pg_regress --config-auth independent of the patch - so the pass/fail
evidence
is inherited from the identical v2 fix plus a read of the new test.)

Regards,
Sai Pranav

On Wed, Nov 04, 2026 08:21 PM, Rajesh Kumar Thandapani <
rajeshkumarth(at)google(dot)com> wrote:

> 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
>