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

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