Thanks for the review. v2 attached, rebased on master. On Tue, Oct 6, 2026 at 3:30 PM Fujii Masao <[email protected]> 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.
v2-0001-Report-correct-size-estimate-for-incremental-back.patch
Description: Binary data
