Hi, On Thu, Oct 8, 2026 at 11:35 PM surya poondla <[email protected]> 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.
v3-0001-Report-correct-size-estimate-for-incremental-back.patch
Description: Binary data
