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(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