The time() granularity to the second makes the result awkward on small
tests:
8/1554552 kB (0%, 8 kB/s)
1040856/1554552 kB (66%, 1040856 kB/s)
1554552/1554552 kB (100%, 1554552 kB/s)
I'd suggest to reuse the "portability/instr_time.h" stuff which allows a
much better precision.
I still think it would make sense to use that instead of low-precision
time().
As the use-case of this is not for small tests,
Even for a long run, the induce error by the 1 second imprecision on both
points is significant at the beginning of the scan.
I don't think it is useful to make the code more complicated for this.
I do not think that it would be much more complicated to use the
portability macros to get a precise time.
The display is exactly the same as in pg_basebackup (except the
bandwith is shown as well), so right now I think it is more useful to be
consistent here.
Hmmm... units are units, and the display is wrong. The fact that other
commands are wrong as well does not look like a good argument to persist in
an error.
I've had a look around, and "kB" seems to be a common unit for 1024
bytes, e.g. in pg_test_fsync etc., unless I am mistaken?
I can only repeat my above comment: the fact that postgres is wrong
elsewhere is not a good reason to persist in reproducing an error.
See the mother of all knowledge https://en.wikipedia.org/wiki/Kilobyte
- SI (decimal, 1000): kB, MB, GB, ...
- IEC (binary 1024): KiB, MiB, GiB, ...
- JEDEC (binary 1024, used for memory): KB, MB, GB.
Summary:
- 1 kB = 1000 bytes
- 1 KB = 1 KiB = 1024 bytes
Decimal is used for storage (HDD, SSD), binary for memory. That is life,
and IMHO Postgres code is not the place to invent new units.
Moreover, I still think that the progress should use MB defined as 1000000
bytes for showing the progress.
I would be okay with a progress printing function shared between some
commands. It just needs some place. pg_rewind also has a --rewind option.
I guess you mean pg_rewind also has a --progress option.
Indeed.
I do agree it makes sense to refactor that, but I don't think this
should be part of this patch.
That's a point. I'd suggest to put the new progress function directly in
the common part, and other patches could take advantage of it for other
commands when someone feels like it.
Other comments:
function toggle_progress_report has empty lines after { and before },
but this style is not used elsewhere, I think that they should be removed.
The report_progress API should be ready to be used by another client
application, which suggest that global variables should be avoided.
Maybe:
void report_progress(current, total, force)
and the scan start and last times could be a static variable inside the
function initialized on the first call, which would suggest to call the
function at the beginning of the scan, probably with current == 0.
Or maybe:
time_type report_progress(current, total, start, last, force)
Which would return the last time, and the caller has responsability for
initializing a start time.
--
Fabien.
