Hallo Michael,
About patch v3: applies cleanly and compiles.
The xml documentation should be updated! (It is kind of hard to notice
what is not there:-)
See "doc/src/sgml/ref/pg_verify_checksums.sgml".
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().
The "kB" kilo-byte symbols stands for 1000 bytes, but you are displaying
1024-bytes kilos, which symbol is either "KiB" or "KB". Given that we are
about storage which is usually measured in powers of 1,000, so I'd suggest
to use thousands.
Another reserve I have is that on any hardware it is likely that there will
be a lot of kilos, so maybe megas (MB) should be used instead.
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.
So if we change that, pg_basebackup should be changed as well I think.
I'm okay with fixing pg_basebackup as well! I'm unsure about the best
place to put such a function, though. Probably under "src/common/" where
there is already some front-end specific code ("fe_memutils.c").
Maybe the code could be factored out to some common file in the future.
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.
Maybe it would be nice to show elapsed time and expected completion time
based on the current speed.
Maybe; this could be added to the factored out common code I mentioned
above.
Yep.
I would be in favor or having this turned on by default, and silenced with
some option. I'm not sure other people would agree, though, so it is an open
question as well.
If this runs in a script, it would be pretty annoying, so everybody
would have to add --no-progress so I am not convinced. Also, both
pg_basebackup and pgbench don't show progress by default.
Ok.
I do not see much advantage in using intermediate string buffers for values:
+ snprintf(totalstr, sizeof(totalstr), INT64_FORMAT,
+ total_size / 1024);
+ snprintf(currentstr, sizeof(currentstr), INT64_FORMAT,
+ current_size / 1024);
+ snprintf(currspeedstr, sizeof(currspeedstr), INT64_FORMAT,
+ (current_size / 1024) / (((time(NULL) - scan_started) == 0) ? 1 :
(time(NULL) - scan_started)));
+ fprintf(stderr, "%s/%s kB (%d%%, %s kB/s)",
+ currentstr, totalstr, total_percent, currspeedstr);
ISTM that just one simple fprintf would be fine:
fprintf(stderr, INT64_FORMAT "/" INT64_FORMAT " ...",
formulas for each slot...);
That code is adopted from pg_basebackup.c and the comment there says:
| * Separate step to keep platform-dependent format code out of
| * translatable strings. And we only test for INT64_FORMAT
| * availability in snprintf, not fprintf.
So that sounds legit to me.
Hmmm. Translation. Not sure I like much the idea of translating units,
but why not.
| * If we are reporting to a terminal, send a carriage return so that
| * we stay on the same line. If not, send a newline.
And it runs a little amok with "-v".
Do you suggest we should make those mutually exlusive? I agree that in
general, -P is not very useful if -v is on, but if you have a really big
table, it should still be, no?
Yep. I was just mentionning that they interfere on a terminal, but I agree
that there is no obvious fix.
+ memset(&act, '\0', sizeof(act));
pg uses 0 instead of '\0' everywhere else.
Ok.
Not '0', plain 0 (the integer of value zero).
--
Fabien.
