On Wed, 29 Aug 2018 21:09:03 +0900
Yugo Nagata <nag...@sraoss.co.jp> wrote:
> On Wed, 29 Aug 2018 13:46:38 +0200
> Magnus Hagander <mag...@hagander.net> wrote:
>
> > On Wed, Aug 29, 2018 at 1:37 PM, Michael Banck <michael.ba...@credativ.de>
> > wrote:
> >
> > > Hi,
> > >
> > > On Wed, Aug 29, 2018 at 08:33:43PM +0900, Yugo Nagata wrote:
> > > > On Wed, 29 Aug 2018 10:28:33 +0200
> > > > Daniel Gustafsson <dan...@yesql.se> wrote:
> > > > > > On 27 Aug 2018, at 14:05, Yugo Nagata <nag...@sraoss.co.jp> wrote:
> > > > > > On Mon, 27 Aug 2018 13:34:12 +0200
> > > > > > Michael Banck <michael.ba...@credativ.de> wrote:
> > > > > >> On Mon, Aug 27, 2018 at 07:53:36PM +0900, Yugo Nagata wrote:
> > > > > >>> On Fri, 24 Aug 2018 18:01:09 +0200
> > > > > >>> Peter Eisentraut <peter.eisentr...@2ndquadrant.com> wrote:
> > > > > >>>> I'm curious about this option:
> > > > > >>>>
> > > > > >>>> -r RELFILENODE check only relation with specified
> > > relfilenode
> > > > > >>>>
> > > > > >>>> but there is no facility to specify a database.
> > > > > >>>>
> > > > > >>>> Also, referring to the relfilenode of a mapped relation seems a
> > > bit
> > > > > >>>> inaccurate.
> > > > > >>>>
> > > > > >>>> Maybe reframing this in terms of the file name of the file you
> > > want
> > > > > >>>> checked would be better?
> > > > > >>>
> > > > > >>> If we specified 1234 to -r option, pg_verify_shceksums checks not
> > > only 1234
> > > > > >>> but also 1234_vm, 1234_fsm, and 1234.1, 1234.2, ... and so on, so
> > > I think
> > > > > >>> it makes senses to allow to specify a relfilenode instead of a
> > > file name.
> > > > > >>>
> > > > > >>> I think it is reasonable to add a option to specify a database,
> > > although
> > > > > >>> I don't know which character is good because both -d and -D are
> > > already used....
> > > > > >>
> > > > > >> Maybe the -d (debug) option should be revisited as well. Mentioning
> > > > > >> every scanned block generates a huge amount of output which might
> > > > > >> be
> > > > > >> useful during development but does not seem very useful for a
> > > > > >> stable
> > > > > >> release. AFAICT there is no other debug output for now.
> > > > > >>
> > > > > >> So it could be renamed to -v (verbose) and only mention each
> > > > > >> scanned
> > > > > >> file, e.g. (errors/checksum mismatches are still reported of
> > > course).
> > >
> > > I still think this should be changed as well, i.e. -v should not report
> > > every block scanned, as that really is debug output and IMO not useful
> > > in general? AFAICT your page does not change the output at all, just
> > > renames the option.
> > >
> > >
> > I agree with this (though it's my fault initially :P). Per-page output is
> > going to be useless in pretty much all production cases. It makes sense to
> > also change it to be per-file.
>
> I updated the patch to output only per-file information in the verbose mode.
> Does this behavior match you expect?
I am sorry but I attached a wrong file in the previous post.
Attached is the correct version of the updated patch.
Regards,
--
Yugo Nagata <nag...@sraoss.co.jp>
commit 7c673e1d712c74c51c708ddc4a151b6fb8cc2a8f
Author: Yugo Nagata <nag...@sraoss.co.jp>
Date: Wed Aug 29 19:37:13 2018 +0900
Raname pg_verity_checksums debug option -d to -v / verbose
Also, change to only mention each scanced file not every block.
diff --git a/doc/src/sgml/ref/pg_verify_checksums.sgml b/doc/src/sgml/ref/pg_verify_checksums.sgml
index ecc5501eae..a1ff060c2b 100644
--- a/doc/src/sgml/ref/pg_verify_checksums.sgml
+++ b/doc/src/sgml/ref/pg_verify_checksums.sgml
@@ -64,8 +64,8 @@ PostgreSQL documentation
<term><option>-d</option></term>
<listitem>
<para>
- Enable debug output. Lists all checked blocks and their checksum.
- </para>
+ Enable debug output. Lists all checked files.
+ <para>
</listitem>
</varlistentry>
diff --git a/src/bin/pg_verify_checksums/pg_verify_checksums.c b/src/bin/pg_verify_checksums/pg_verify_checksums.c
index 938b92282a..6be138eb75 100644
--- a/src/bin/pg_verify_checksums/pg_verify_checksums.c
+++ b/src/bin/pg_verify_checksums/pg_verify_checksums.c
@@ -31,7 +31,7 @@ static int64 badblocks = 0;
static ControlFileData *ControlFile;
static char *only_relfilenode = NULL;
-static bool debug = false;
+static bool verbose = false;
static const char *progname;
@@ -43,7 +43,7 @@ usage()
printf(_(" %s [OPTION]... [DATADIR]\n"), progname);
printf(_("\nOptions:\n"));
printf(_(" [-D, --pgdata=]DATADIR data directory\n"));
- printf(_(" -d debug output, list all checked blocks\n"));
+ printf(_(" -v, --verbose output verbose messages, list all checked files\n"));
printf(_(" -r RELFILENODE check only relation with specified relfilenode\n"));
printf(_(" -V, --version output version information, then exit\n"));
printf(_(" -?, --help show this help, then exit\n"));
@@ -120,11 +120,11 @@ scan_file(char *fn, int segmentno)
progname, fn, blockno, csum, header->pd_checksum);
badblocks++;
}
- else if (debug)
- fprintf(stderr, _("%s: checksum verified in file \"%s\", block %d: %X\n"),
- progname, fn, blockno, csum);
}
+ if (verbose)
+ fprintf(stderr, _("%s: checksum verified in file \"%s\"\n"), progname, fn);
+
close(f);
}
@@ -208,6 +208,7 @@ main(int argc, char *argv[])
{
static struct option long_options[] = {
{"pgdata", required_argument, NULL, 'D'},
+ {"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
@@ -234,12 +235,12 @@ main(int argc, char *argv[])
}
}
- while ((c = getopt_long(argc, argv, "D:r:d", long_options, &option_index)) != -1)
+ while ((c = getopt_long(argc, argv, "D:r:v", long_options, &option_index)) != -1)
{
switch (c)
{
- case 'd':
- debug = true;
+ case 'v':
+ verbose = true;
break;
case 'D':
DataDir = optarg;
