Re: Incremental backups report progress as if they were full backups

Hi,

On Thu, Oct 8, 2026 at 11:35 PM surya poondla <suryapoondla4(at)gmail(dot)com>
wrote:

> I tested v2 on master (f884f359f5a). It applies cleanly, and the new test
> fails without the fix and passes with it. With v2, an incremental backup
> of a cluster with an extra tablespace now ends at
> 3987/3987 kB (100%), compared with 3987/56202 kB (7%) without the patch.
>

Thanks for testing it.

1. The test doesn't cover the sendTablespace() half of the fix. With only
> that hunk reverted, so that the estimate pass still passes NULL
> to sendTablespace(), 012_incremental_progress.pl still passes, because
> the test cluster has no user-defined tablespace. With an in-place
> tablespace holding a table that doesn't change, the partial fix still
> reports 3987/35982 kB (11%) at the end.
> Could you add an in-place tablespace to the test
> (allow_in_place_tablespaces, as in
> 011_in_place_tablespace.pl) and put most of the unchanged data in it?
>
> You are right, and I have reworked the test along those lines. The data
that does not change now lives in an in-place tablespace, the table that
changes
stays in $PGDATA, and the backups are taken in tar format, so no mapping is
needed.
I also added a second check. Instead of comparing the two backups, it
compares the estimate with what was actually streamed. pg_basebackup raises
the total to the amount streamed, so the two numbers end up equal when the
estimate is right, and the total stays far above it when any part of the
cluster is still counted in full. That check does not depend on how the
data is distributed between $PGDATA and the tablespace.
With each half of the fix reverted on its own, the test now fails in both
cases. Reverting the sendTablespace() hunk makes the estimate exceed
the amount streamed, and reverting the sendDir() hunk does the same. The
first case is the one you described, and it is caught only by the new check.

>
> 2. With server-side compression the new "sent" wording isn't accurate.
>

Agreed, and the comments and the protocol documentation now say "archive
data", with the counting described as happening before any server-side
compression.
The numbers themselves are consistent, though. pg_basebackup does not count
what it receives: it takes the counter from the server's progress report
message, which carries the same pre-compression value, so the reported pair
stays in one unit and the percentage is correct. The 81 kB you saw is what
was received by the client after compression, not what the progress line
measures.
While there, one question. The description of --no-estimate-size in
pg_basebackup.sgml says "the total amount of data that will be streamed",
and
monitoring.sgml describes backup_total the same way. Both descriptions are
potentially misleading when server-side compression is enabled, since the
progress counters measure archive data before compression. I left them
unchanged because the wording predates this patch and is consistent between
the two places. Do you think this should be updated as well?

> 3. Minor suggestion, the new test file starts its own cluster for a single
> check.
>

I would prefer to keep it as its own file. In src/bin/pg_combinebackup/t
each of the twelve files starts its own clusters and runs a handful of
checks,
so a cluster per file is the usual arrangement in this area. In
010_pg_basebackup.pl the main cluster does not enable
summarize_wal, and the one that does, node2, exists in order to have a
different system identifier. The whole file runs in a few seconds. If you
would still prefer to keep the test in 010_pg_basebackup.pl, node2 would be
the appropriate place for it.

4. On back branches: 17 and 18 have the same unfreed ipath, and with
> --progress the backported fix would call GetFileBackupMethod() twice
> per relation file there too. So I'd keep the pfree(ipath) in the
> backpatch; it's two lines and is low risk.
>

The pfree() is part of the patch now, so it will follow wherever the patch
goes. If the estimate change is taken only on master and the leak is still
worth fixing in 17 and 18, I can send it separately.

The patch is rebased on master, v3 is attached.