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
