Hi, While looking at pg_basebackup --progress I noticed that an incremental backup reports its progress against the size of a full backup. The final line of a completed backup:
6534/346720 kB (1%), 1/1 tablespace The backup completes at 1%: 346720 kB is reported as the total, while only 6519 kB is streamed. pg_stat_progress_basebackup shows the same: backup_type says 'incremental', but backup_total is the size of a full backup. The estimate comes from a separate size-only pass, which is not told that the backup is incremental (basebackup.c). As a result, the estimate does not use the same file backup method as the actual transfer: /* estimate */ sendDir(sink, ".", 1, true, ..., InvalidOid, NULL); /* transfer */ sendDir(sink, ".", 1, false, ..., InvalidOid, ib); The last parameter is IncrementalBackupInfo *ib. Without it, GetFileBackupMethod() is never reached, so every file is counted in full. sendDir() already handles this when sizeonly is set; the incremental path is simply unreachable without ib. 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%). This patch also changes what the size field of the tablespace list means for incremental backups. The protocol documentation currently describes it as the size of the tablespace, while the stated purpose of PROGRESS is to let the client determine how far along the stream is. For a full backup those are the same number, but they diverge for an incremental one, where the old value describes data that is never sent. The patch therefore updates the documentation to describe the field as the approximate amount of data expected to be sent. Since this changes the meaning of a documented field, I'd rather check before going ahead: would this be acceptable, or would a separate field be preferable? A second consideration is that the patch makes GetFileBackupMethod() run twice per file, once in each pass. It operates on the in-memory block reference table rather than doing I/O, so I expect the additional cost to be small, but I'm not sure whether this is something we should be concerned about for clusters with a very large number of files, or whether it would be preferable to cache the result from the first pass. Opinions welcome. The patch adds a TAP test. It compares the total reported for a full backup with the total reported for an incremental backup of the same cluster. I see the same code in 17, 18 and 19, but so far I've only prepared the patch for master. -- Daria Lepikhova
v1-0001-Report-correct-size-estimate-for-incremental-back.patch
Description: Binary data
