|From:||Michael Banck <michael(dot)banck(at)credativ(dot)de>|
|To:||Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|Cc:||pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>|
|Subject:||Re: Progress reporting for pg_verify_checksums|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On Wed, Sep 26, 2018 at 10:46:06AM +0200, Fabien COELHO wrote:
> 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".
Right, I've added a paragraph.
> >>The time() granularity to the second makes the result awkward on small
> >> 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
As the use-case of this is not for small tests, I don't think it is
useful to make the code more complicated for this.
> >>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.
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?
> >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.
I guess you mean pg_rewind also has a --progress option.
I do agree it makes sense to refactor that, but I don't think this
should be part of this patch.
> >> + memset(&act, '\0', sizeof(act));
> >>pg uses 0 instead of '\0' everywhere else.
> Not '0', plain 0 (the integer of value zero).
Oops, thanks for spotting that.
I've attached v4 of the patch.
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
credativ GmbH, HRB Mönchengladbach 12080
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer
Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz
|Next Message||Tom Lane||2018-09-28 13:13:56||Re: pgsql: Build src/port files as a library with -fPIC, and use that in li|
|Previous Message||Thomas Munro||2018-09-28 12:19:58||Re: [HACKERS] kqueue|