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
