|From:||Michael Banck <michael(dot)banck(at)credativ(dot)de>|
|To:||Michael Paquier <michael(at)paquier(dot)xyz>, Fabien COELHO <coelho(at)cri(dot)ensmp(dot)fr>|
|Cc:||Thomas Munro <thomas(dot)munro(at)enterprisedb(dot)com>, Bernd Helmle <bernd(dot)helmle(at)credativ(dot)de>, Pg 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|
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.
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||Justin Pryzby||2019-02-17 20:07:00||Re: REL_11_STABLE: dsm.c - cannot unpin a segment that is not pinned|
|Previous Message||James Coleman||2019-02-17 19:44:24||Re: 2019-03 CF Summary / Review - Tranche #2|