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

On Tue, Sep 29, 2026 at 4:32 AM Daria Lepikhova
<daria(dot)n(dot)lepikhova(at)gmail(dot)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