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 |
Date: | 2018-09-28 13:06:41 |
Message-ID: | 20180928130641.GA20017@nighthawk.caipicrew.dd-dns.de |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi,
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
> >>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, 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.
> >
> >Ok.
>
> Not '0', plain 0 (the integer of value zero).
Oops, thanks for spotting that.
I've attached v4 of the patch.
Michael
--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael(dot)banck(at)credativ(dot)de
credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
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
Attachment | Content-Type | Size |
---|---|---|
pg_verify_checksums_progress_V4.patch | text/x-diff | 8.0 KB |
From | Date | Subject | |
---|---|---|---|
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 |