Re: Progress reporting for pg_verify_checksums

Hello.

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 */
> + 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.

> 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.)

> +#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,
> PG_TEMP_FILES_DIR,
> 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.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center