Incremental backups report progress as if they were full backups

Hi Daria,

I reproduced the wrong estimate and confirmed v1 fixes it. On master, a
finished incremental backup of a lightly-changed cluster ends at
4847/36417 kB (13%); with v1 the same backup ends at 4847/4847 kB
(100%), and pg_stat_progress_basebackup's total drops to match.

The bug reproduces on the back branches too: 17.11 ends at 4798/35759 kB
(13%), 18.6 at 4833/36186 kB, and 19beta4 at 4847/36344 kB. v1 applies
to all three with only line offsets, and once applied each of them ends
at 100% as well, so the backport looks straightforward and needs no
separate patch.

A second consideration is that the patch makes GetFileBackupMethod()
run twice per file [...] I'm not sure whether this is something we
should be concerned about for clusters with a very large number of
files, or whether it would be preferable to cache the result

I measured this. I put an INSTR_TIME around the size-only pass and
logged it, then ran an incremental backup with and without passing ib to
that pass, on clusters made of many empty tables (one file each), median
of 12 runs:

- 20,906 files: 37.4 ms without ib, 45.3 ms with it.
- 100,906 files: 173.8 ms without ib, 204.0 ms with it.

So the extra GetFileBackupMethod() adds about 0.3 microseconds per file
(0.38 at 20k, 0.30 at 100k), ~30 ms at 100k files. It scales linearly
and stays a small part of the size-only pass, which runs once before a
backup that takes far longer. For what it's worth, I don't think
caching the first pass is worth the extra code; the cost is not
measurable against a real backup.

Since this changes the meaning of a documented field, I'd rather check
before going ahead: would this be acceptable, or would a separate
field be preferable?

No strong opinion, but reusing the field reads right to me. The stated
purpose of PROGRESS is to let the client tell how far along the stream
is, which is the amount that will be sent; a separate field would leave
the documented one reporting bytes that are never sent for an
incremental backup.

Regards,
Manu

Hi,

On Mon, Sep 28, 2026 at 10:30 PM Manu <manuelreyesbravo@gmail.com> wrote:

A second consideration is that the patch makes GetFileBackupMethod()
run twice per file [...] I'm not sure whether this is something we
should be concerned about for clusters with a very large number of
files, or whether it would be preferable to cache the result

I measured this. I put an INSTR_TIME around the size-only pass and
logged it, then ran an incremental backup with and without passing ib to
that pass, on clusters made of many empty tables (one file each), median
of 12 runs:

- 20,906 files: 37.4 ms without ib, 45.3 ms with it.
- 100,906 files: 173.8 ms without ib, 204.0 ms with it.

So the extra GetFileBackupMethod() adds about 0.3 microseconds per file
(0.38 at 20k, 0.30 at 100k), ~30 ms at 100k files. It scales linearly
and stays a small part of the size-only pass, which runs once before a
backup that takes far longer. For what it's worth, I don't think
caching the first pass is worth the extra code; the cost is not
measurable against a real backup.

Thanks for the review, and especially for reproducing the issue and doing
the detailed measurements. Your results confirm that, in practice,
the additional cost of calling GetFileBackupMethod() twice is small.

Since this changes the meaning of a documented field, I'd rather check

before going ahead: would this be acceptable, or would a separate
field be preferable?

No strong opinion, but reusing the field reads right to me. The stated
purpose of PROGRESS is to let the client tell how far along the stream
is, which is the amount that will be sent; a separate field would leave
the documented one reporting bytes that are never sent for an
incremental backup.

I agree with this reasoning. I was guided by the same logic. A separate
field
would not remove the misleading value, it would just put the useful one
next to it.

On Tue, Sep 29, 2026 at 4:32 AM Daria Lepikhova
<daria.n.lepikhova@gmail.com> wrote:

The attached patch passes ib to both calls in that loop
(PrepareForIncrementalBackup() has already run, so nothing extra is read).
The same backup then ends at 6519/6519 kB (100%).

Thanks for the patch! I have a few comments on the patch.

The description of --no-estimate-size in pg_basebackup.sgml still refers
to "the size of the entire database". This should be updated as well?

The bbsink_state comments in basebackup_sink.h still say that
"bytes_total is the total number of bytes estimated to be present in
$PGDATA". This should be updated too?

+ ok($result, "backup into $path succeeded") or diag $stderr;

Isn't it better to use ok(...) or die(...) here to stop the test if the
backup fails? Otherwise, diag only prints the output and the test
continues. The subsequent incremental backup would then fail because its
reference manifest is missing, which seems wasteful.

+ ok(@lines > 0, "progress was reported for $path") or return undef;

Isn't it better to use ok(...) or die(...) here to stop the test, too,
if no progress output was reported? The caller assumes a defined
return value, so returning undef seems to result in a fatal
uninitialized-value warning when it prints the total.

+ my ($total) = $lines[-1] =~ m{\d+/(\d+) kB};
+ return $total;

Isn't it better to check that $total is defined before returning it,
for example with "defined($total) or die(...)"? The preceding filter only
checks whether a line contains "kB", which does not guarantee that this
pattern matches. If parsing fails, it seems better to stop the test with an
explicit error rather than pass undef to the caller.

GetFileBackupMethod() does not free ipath, so these allocations seem
to accumulate until the backup finishes. Since the patch increases the number
of calls to GetFileBackupMethod(), it also increases that accumulation.
So, this missing pfree() is not the issue introduced by the patch, but
isn't it better to fix that existing issue here as well?

Regards,

--
Fujii Masao

Hello!

-           The approximate size of the tablespace, in kilobytes (1024 bytes),
-           if progress report has been requested; otherwise it's null.
+           The approximate amount of data that will be sent for the
+           tablespace, in kilobytes (1024 bytes), if progress report has been
+           requested; otherwise it's null. For an incremental backup this can
+           be much smaller than the size of the tablespace, since files that
+           have changed only partially are sent as incremental files.

One nit to this: shouldn't it also mention unchanged files?

For example:

For an incremental backup this can be much smaller than the size of the tablespace, since relation files that are unchanged or only partly changed since the prior backup are sent as incremental files containing just the modified blocks.

Thanks for the review. v2 attached, rebased on master.

On Tue, Oct 6, 2026 at 3:30 PM Fujii Masao <masao.fujii@gmail.com> wrote:

The description of --no-estimate-size in pg_basebackup.sgml still refers
to "the size of the entire database". This should be updated as well?

Yes, I missed that paragraph. I have updated it to say
that the backup starts by estimating the total amount of data that will be
streamed. I used the same wording as monitoring.sgml, which already
describes
backup_total that way.

The bbsink_state comments in basebackup_sink.h still say that
"bytes_total is the total number of bytes estimated to be present in
$PGDATA". This should be updated too?

I have updated that comment too. bytes_total is now described as the number
of bytes estimated to be sent.
I also changed the line above it. bytes_done is described there as the
number
of bytes read from $PGDATA, but it is incremented with the length of the
archive contents passed to the sink, so it counts what is sent. I think both
fields should describe the same thing. If you prefer, I can leave that line
as it was.

+ ok($result, "backup into $path succeeded") or diag $stderr;

Isn't it better to use ok(...) or die(...) here to stop the test if the
backup fails? Otherwise, diag only prints the output and the test
continues. The subsequent incremental backup would then fail because its
reference manifest is missing, which seems wasteful.

+ ok(@lines > 0, "progress was reported for $path") or return undef;

Isn't it better to use ok(...) or die(...) here to stop the test, too,
if no progress output was reported? The caller assumes a defined
return value, so returning undef seems to result in a fatal
uninitialized-value warning when it prints the total.

+ my ($total) = $lines[-1] =~ m{\d+/(\d+) kB};
+ return $total;

Isn't it better to check that $total is defined before returning it,
for example with "defined($total) or die(...)"? The preceding filter only
checks whether a line contains "kB", which does not guarantee that this
pattern matches. If parsing fails, it seems better to stop the test with an
explicit error rather than pass undef to the caller.

I agree with all three. The test now dies in each of those places. The
messages name the backup path, and when the progress line cannot be parsed
it
includes the line itself.

GetFileBackupMethod() does not free ipath, so these allocations seem
to accumulate until the backup finishes. Since the patch increases the
number
of calls to GetFileBackupMethod(), it also increases that accumulation.
So, this missing pfree() is not the issue introduced by the patch, but
isn't it better to fix that existing issue here as well?

This is a fair point, and the pfree() is now part of the patch. I did not

propose it for 17 and 18, since the leak is small and the memory is
released
when the command ends.

Zsolt also noted that the new description of the size field should mention
unchanged files, not only partly changed ones. Agreed, and it went into v2
together with the other changes.

Hi Daria,

Thank you for reporting the issue and working on it.

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.

A few comments:
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?

In-place tablespaces get a path in the tablespace list, so they go through
sendTablespace(), and with --format=tar no tablespace mapping is needed.

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

  + * 'bytes_done' is the number of bytes sent so far.
  + * 'bytes_total' is the total number of bytes estimated to be sent, if we
  + * have estimated this.

The progress sink is stacked on top of the compression sink (basebackup.c,
after bbsink_gzip_new() and friends), so these counters
measure uncompressed archive data, not what goes over the wire. With
--compress=server-gzip, my incremental backup reported 3987 kB total
while 81 kB of compressed archives were received (very compressible test
data, but the point stands). I agree "read from $PGDATA" was
inaccurate too. Maybe something like "the number of bytes of archive data
generated so far, before any server-side compression"? The same
applies to the new protocol.sgml text for the size field ("amount of data
that will be sent").

3. Minor suggestion, the new test file starts its own cluster for a single
check.
It could instead go into 010_pg_basebackup.pl, or into
src/bin/pg_combinebackup/t/, where most of the incremental backup tests
live.

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.

Regards,
Surya Poondla

Hi,

On Thu, Oct 8, 2026 at 11:35 PM surya poondla <suryapoondla4@gmail.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.

Hi Daria,

Thanks for v3.

I tested it on master (8e82d6610e7). To check the new test, I reverted each
half of the fix in turn and reran it:
1. Without the sendTablespace() change, the backup ended at 3811/14478 kB.
Only your new check caught this; the comparison with
the full backup still passed, just as you said.
2. Without the sendDir() change, it ended at 3811/24251 kB, and both checks
failed.
3. With the full patch, it ended at 3811/3811 kB.

So the test now covers both halves of the fix. I also tried an incremental
backup with --compress=server-gzip, and it ends at 100% as
well. The pg_basebackup and pg_combinebackup test suites pass.

Regarding compression, thanks for explaining. You're right that
compression doesn't throw off the percentage, since both numbers are counted
before compression. My concern was only the wording.

On your question about the "streamed" wording: the --no-estimate-size
sentence is new in this patch (before, it said "enumerating the size of
the entire database"), so I'd change it to match the new wording.
Something like "estimating the total amount of archive data that will be
generated". The monitoring.sgml text is older and says "streamed"
for both backup_total and backup_streamed, so I'd leave it for a separate
patch, if anyone thinks it's worth changing.

One small leftover: the comment in perform_base_backup() still says
  +              * Calculate the total backup size by summing up the amount
of data
  +              * that will be sent for each tablespace
which could say "archive data" too.

Your reasoning on keeping the test in its own file, and on the back
branches, makes sense to me.

Regards,
Surya Poondla

On Tue, Oct 13, 2026 at 5:50 AM surya poondla <suryapoondla4@gmail.com> wrote:

Hi Daria,

Thanks for v3.

Thanks for updating the patch!

The documentation clarification below about the relationship between
estimated backup size and compression addresses a separate issue from the
incorrect progress reporting for incremental backups:

+           The approximate size of the archive that will be generated for the
+           tablespace, in kilobytes (1024 bytes), before any server-side
+           compression, if progress report has been requested; otherwise it's

Rather than combining both fixes in one patch, how about splitting them into
two patches? They address separate issues and have different backpatch
ranges. The documentation clarification should be backpatched to v15, where
server-side compression was introduced, while the incremental backup
progress fix should be backpatched to v17.

I've created both patches based on v3 patch and attached them.
For the 0002 patch, I've also made some cosmetic changes and
cleaned up the tests.

Regards,

--
Fujii Masao

Hi Fujii,

Rather than combining both fixes in one patch, how about splitting them
into
two patches? They address separate issues and have different backpatch
ranges. The documentation clarification should be backpatched to v15, where
server-side compression was introduced, while the incremental backup
progress fix should be backpatched to v17.

I've created both patches based on v3 patch and attached them.
For the 0002 patch, I've also made some cosmetic changes and
cleaned up the tests.

Thanks for splitting the patch. The split looks good to me.
The compression wording applies to every base backup since v15, and the
estimate fix only matters for incremental backups, which arrived in v17.

I looked through both patches and few observations:
1. 0001 applies cleanly to master and to REL_15_STABLE through
REL_18_STABLE, and 0002 applies on top of it on master, REL_17_STABLE and
REL_18_STABLE.
2. The new wording in 0001 matches the code on all of those branches. The
progress sink sits above the compression sinks, so the counters measure
data before compression.
3. Your fix to the progress report message description is right, too.
bytes_done is only reset when the backup starts, so the message
carries a running total for the whole backup, not a per-tablespace count.
4. The code in 0002 is the same as in v3, apart from how the pfree() is
written.

I also reran the tests on master (158b6d5109f) with v4 applied. The new
test passes, and still fails with either half of the fix reverted,
with the same numbers as with v3. Since the CHECKPOINTs were removed, I
ran it five times in a row, and it passed every time. An incremental
backup with --compress=server-gzip ends at 100%, and the pg_basebackup and
pg_combinebackup suites pass.

v4 looks good to me.

Regards,
Surya Poondla

Hi,

On Tue, Oct 13, 2026 at 5:45 PM Fujii Masao <masao.fujii@gmail.com> wrote:

The documentation clarification below about the relationship between
estimated backup size and compression addresses a separate issue from the
incorrect progress reporting for incremental backups:

+ The approximate size of the archive that will be generated

for the

+           tablespace, in kilobytes (1024 bytes), before any server-side
+           compression, if progress report has been requested; otherwise

it's

Rather than combining both fixes in one patch, how about splitting them

into

two patches? They address separate issues and have different backpatch
ranges. The documentation clarification should be backpatched to v15,

where

server-side compression was introduced, while the incremental backup
progress fix should be backpatched to v17.

I've created both patches based on v3 patch and attached them.
For the 0002 patch, I've also made some cosmetic changes and
cleaned up the tests.

Thank you for splitting the patch and for preparing both versions yourself.

Agreed, we got a bit carried away and ended up touching two separate issues
in one patch. The split and the resulting backpatch ranges make sense to me.

I have no objections to the contents of either patch. I checked two points
independently. The clarification about the progress report’s byte count
applies as far back
as v15: bytes_done is initialized to zero once, at the start of the backup,
and the progress message carries state->bytes_done unchanged, so the count
has always
been cumulative rather than per tablespace. The compression wording is also
accurate for v15 and v16, where the progress sink is created above the
compression
sinks, so the counters measure archive data before compression on those
branches too.

I got the same results as Surya. I also checked the branch ranges myself:
patch 0001 applies to REL_15_STABLE through REL_18_STABLE, and patch 0002
applies on top of it
to REL_17_STABLE and REL_18_STABLE. Since the CHECKPOINTs have been removed
from the test, I ran it twenty times in a row rather than five, with no
failures, and
the totals are the same as with v3: 34694/34694 kB for the full backup and
3811/3811 kB for the incremental one.

And I also tested a larger cluster outside the test suite, since the test
only uses small relations. With a 20 GB table spread over twenty segments
in a separate tablespace and
fifty rows changed in the last segment, the full backup ends at
21684624/21684624 kB and the incremental backup at 9606/9606 kB. Nineteen
of the twenty segments
are sent as 12-byte incremental files, while the twentieth contains 72 kB
of changed blocks. None of the segments is sent in full. pg_combinebackup
reconstructs the chain,
pg_verifybackup passes, and the restored data matches.

On Wed, Oct 14, 2026 at 6:35 PM Daria Lepikhova
<daria.n.lepikhova@gmail.com> wrote:

I have no objections to the contents of either patch.

I've pushed the patches. Thanks!

Regards,

--
Fujii Masao