Hi Daria,
I reproduced the wrong estimate and confirmed v1 fixes it. On master, a
finished incremental backup of a lightly-changed cluster ends at
4847/36417 kB (13%); with v1 the same backup ends at 4847/4847 kB
(100%), and pg_stat_progress_basebackup's total drops to match.
The bug reproduces on the back branches too: 17.11 ends at 4798/35759 kB
(13%), 18.6 at 4833/36186 kB, and 19beta4 at 4847/36344 kB. v1 applies
to all three with only line offsets, and once applied each of them ends
at 100% as well, so the backport looks straightforward and needs no
separate patch.
> A second consideration is that the patch makes GetFileBackupMethod()
> run twice per file [...] 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
I measured this. I put an INSTR_TIME around the size-only pass and
logged it, then ran an incremental backup with and without passing ib to
that pass, on clusters made of many empty tables (one file each), median
of 12 runs:
- 20,906 files: 37.4 ms without ib, 45.3 ms with it.
- 100,906 files: 173.8 ms without ib, 204.0 ms with it.
So the extra GetFileBackupMethod() adds about 0.3 microseconds per file
(0.38 at 20k, 0.30 at 100k), ~30 ms at 100k files. It scales linearly
and stays a small part of the size-only pass, which runs once before a
backup that takes far longer. For what it's worth, I don't think
caching the first pass is worth the extra code; the cost is not
measurable against a real backup.
> 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?
No strong opinion, but reusing the field reads right to me. The stated
purpose of PROGRESS is to let the client tell how far along the stream
is, which is the amount that will be sent; a separate field would leave
the documented one reporting bytes that are never sent for an
incremental backup.
Regards,
Manu
