Re: Progress reporting for pg_verify_checksums

Hello.

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