Re: Progress reporting for pg_verify_checksums

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
To: michael(at)paquier(dot)xyz
Cc: coelho(at)cri(dot)ensmp(dot)fr, michael(dot)banck(at)credativ(dot)de, alvherre(at)2ndquadrant(dot)com, mailings(at)oopsware(dot)de, thomas(dot)munro(at)enterprisedb(dot)com, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Progress reporting for pg_verify_checksums
Date: 2019-03-14 02:54:17
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


At Wed, 13 Mar 2019 16:25:15 +0900, Michael Paquier <michael(at)paquier(dot)xyz> wrote in <20190313072515(dot)GB2988(at)paquier(dot)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.

> + /* Make sure we report at most once every tenth of a second */
> + 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.

> 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

If the all of aboves are involved, the line would look as the

[======================= ] ( 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.)

> +#define MEGABYTES 1048576

1024 * 1024 is preferable than that many digits.

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

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

> @@ -167,7 +255,7 @@ scan_directory(const char *basedir, const char *subdir)
> if (strncmp(de->d_name,
> strlen(PG_TEMP_FILES_DIR)) == 0)
> - return;
> + continue;

Why this patch changes the behavior for temprary directories? It
seems like a bug fix of pg_checksums.


Kyotaro Horiguchi
NTT Open Source Software Center

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Stephen Frost 2019-03-14 03:03:26 Re: Special role for subscriptions
Previous Message Tom Lane 2019-03-14 02:44:30 Re: pgsql: Add support for hyperbolic functions, as well as log10().