Re: Progress reporting for pg_verify_checksums

Hi,

Am Mittwoch, den 26.12.2018, 11:14 +0900 schrieb Michael Paquier:
> On Tue, Dec 25, 2018 at 07:05:30PM +0100, Fabien COELHO wrote:
> > You use 1024² bytes. What about 1000000 bytes per MB, as the
> > unit is about stored files?

I haven't changed that as Ihave not been pointed to a clear project
guideline that 1 MB should be 1000000 and not 1024x1024.
 
> > Also, you did not answer to my other points:
> >  - use "instr_time.h" for better precision

Both pg_rewind and pg_basebackup are using time() as well, so I think
there's precedence for this.

> >  - invert sizeonly

The current way is consistent with src/backend/replication/basebackup.c
so I'd prefer to keep it that way.

> >  - reuse a test

I've removed the test. 

> It seems to me that the SIGUSR1 business is not necessary for the
> basic layer of this feature, so I would rather rip that off now.  

But is it gaining all so much if it is ripped out? The patch will be a
dozen lines shorter, but those are all isolated.

And contrary to pg_basebackup, pg_verify_checksums might be run in the
foreground much more often - right now it has to be done while the
cluster is offline, so if you have large instance and you forgot to pass
-P then you're glad you can signal it so you roughly know when you can
expect to startup the instance again.

> If necessary we could always discuss that later on.  

If you insist we can rip it out, of course.

> My take about the option is that --progress should not be the default, 
> but that reports should only be provided if the caller wants them.
>
> --quiet may have some value by itself, but that seems like a separate
> discussion to me.

Right now --progress isn't default so I think that's how you like it?
    
> +   /*
> +    * If we are reporting to a terminal, send a carriage return so that we
> +    * stay on the same line.  If not, send a newline.
> +    */
> +   if (isatty(fileno(stderr)))
> +       fprintf(stderr, "\r");
> +   else
> +       fprintf(stderr, "\n");
> This bit is not really elegant, why not just '\r'?

I've changed it as Alvaro suggested.

> +   /* The same for total size */
> +   if (current_size > total_size)
> +       total_size = current_size / 1048576;
> Let's use that in a variable and not hardcode the number.

Done so, I called it MEGABYTES.

> What's introduced here is very similar to what progress_report() does
> in pg_rewind/logging.c.  The report depends on the context so this
> cannot be a common routine logic but perhaps we could keep a
> consistent output for both tools?

Well, pg_rewind is also using kB, so should I switch it back to that? 

It looks like the progress reporting is otherwise quite similar, so what
exactly did you have in mind?

New patch attached.


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