Hi, On Mon, Sep 28, 2026 at 10:30 PM Manu <[email protected]> wrote:
> > 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.
