Re: Progress reporting for pg_verify_checksums - Mailing list pgsql-hackers

Hi,

thanks for the additional review!

Am Donnerstag, den 14.03.2019, 11:54 +0900 schrieb Kyotaro HORIGUCHI:
> At Wed, 13 Mar 2019 16:25:15 +0900, Michael Paquier <michael@paquier.xyz> wrote in
<20190313072515.GB2988@paquier.xyz>
> > On Wed, Mar 13, 2019 at 07:22:28AM +0100, Fabien COELHO wrote:
> > > Does not apply because of the renaming committed by Michaël.
> > > 
> > > Could you rebase?
> > 
> > This stuff touches pg_checksums.c, so you may want to wait one day or
> > two to avoid extra work...  I think that I'll be able to finish the
> > addition of the --enable and --disable switches by then.

I have rebased it now.

> > +     /* Make sure we report at most once every tenth of a second */
> > +     if ((INSTR_TIME_GET_MILLISEC(now) -
> > +              INSTR_TIME_GET_MILLISEC(last_progress_update) < 100) && !force)
> 
> I'm not a skilled gamer and 10Hz update is a bit hard for my
> eyes:p The second hand of old clocks ticks at 4Hz. I think it is
> enough frequently.

Ok.

> > 841/1516 MB (55%, 700 MB/s)
> 
> Differently from other prgress reporting project, estimating ETC
> (estimated time to completion), which is in the most important,
> is quite easy. (pgbench does that.) And I'd like to see a
> progress bar there but it might be overdoing. I'm not sure let
> the current size occupy a part of screen width is needed.  I
> don't think total size is needed to be fixed in MB and I think at
> most four or five digits are enough. (That is the same for
> speed.)
> 
> If the all of aboves are involved, the line would look as the
> follows.
> 
> [=======================             ] ( 63% of 12.53 GB, 179 MB/s, ETC 26s)
> 
> # Note that this is just an opinion.
> 
> (pg_checksum runs fast at the beginning so ETC behaves somewhat
> strange in the meanwhile.)

I haven't changed that for now as it seems to be a bit more involved.
I'd like to hear other opinions on whether that is worthwhile?

> > +#define MEGABYTES 1048576
> 
>  1024 * 1024 is preferable than that many digits.

Good point, changed that way.

> > +     /* we handle SIGUSR1 only, and toggle the value of show_progress */
> > +     if (signum == SIGUSR1)
> > +             show_progress = !show_progress;
> 
> SIGUSR1 *toggles* progress. 

Not sure what you mean here, are you saying it should be called
toggle_progress and not show_progress? I think it was like that
originally but got changed due to review comments.

> A garbage line is left alone after turning it off. It would be
> erasable. I'm not sure which is better, though.

How about just printing a "\n" in the signal handler if we are in a
terminal?  I think erasing the line might get too clever and will be a
portability hazard? I have done that now in the attached patch, let me
know what you think.


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.banck@credativ.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