> 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.
Thanks for the review, and especially for reproducing the issue and doing the detailed measurements. Your results confirm that, in practice,
the additional cost of calling GetFileBackupMethod() twice is small.
> 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.
I agree with this reasoning. I was guided by the same logic. A separate field
would not remove the misleading value, it would just put the useful one next to it.
