On Mon, May 27, 2019 at 08:32:21AM +0200, Fabien COELHO wrote:
> I've used both -f & --filenode in the test to check that the renaming was
> ok. I have reordered the options in the documentation so that they appear in
> alphabetical order, as for some reason --progress was out of it.

No objection to clean up this at the same time.

+     <varlistentry>
+      <term><option>-f <replaceable>filenode</replaceable></option></term>
+      
<term><option>--filenode=<replaceable>filenode</replaceable></option></term>
+      <listitem>
+       <para>
+        Only validate checksums in the relation with specified relation file 
node.
+       </para>
Two nits.  I would just have been careful about the number of
characters in the line within the <para> markup.  And we use
extensively "filenode" in the docs.  So the description would become
as follows:
Only validate checksums in the relation with filenode
<replaceable>filenode</replaceable>.

+       [ 'pg_checksums', '--enable', '-filenode', '1234', '-D', $pgdata ],
This fails, but not for the reason it is written for.

It looks strange to not order --filenode alphabetically in --help.

With all these issues cleaned up, I got the attached.  Does that look
fine?  (I ran pgperltidy and pgindent on top of it.)
--
Michael
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index a0ffeb0ab0..33706d1d97 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -100,6 +100,17 @@ PostgreSQL documentation
       </listitem>
      </varlistentry>
 
+     <varlistentry>
+      <term><option>-f <replaceable>filenode</replaceable></option></term>
+      <term><option>--filenode=<replaceable>filenode</replaceable></option></term>
+      <listitem>
+       <para>
+        Only validate checksums in the relation with filenode
+        <replaceable>filenode</replaceable>. 
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry>
       <term><option>-N</option></term>
       <term><option>--no-sync</option></term>
@@ -116,25 +127,6 @@ PostgreSQL documentation
       </listitem>
      </varlistentry>
 
-     <varlistentry>
-      <term><option>-v</option></term>
-      <term><option>--verbose</option></term>
-      <listitem>
-       <para>
-        Enable verbose output. Lists all checked files.
-       </para>
-      </listitem>
-     </varlistentry>
-
-     <varlistentry>
-      <term><option>-r <replaceable>relfilenode</replaceable></option></term>
-      <listitem>
-       <para>
-        Only validate checksums in the relation with specified relfilenode.
-       </para>
-      </listitem>
-     </varlistentry>
-
      <varlistentry>
       <term><option>-P</option></term>
       <term><option>--progress</option></term>
@@ -146,6 +138,16 @@ PostgreSQL documentation
       </listitem>
      </varlistentry>
 
+     <varlistentry>
+      <term><option>-v</option></term>
+      <term><option>--verbose</option></term>
+      <listitem>
+       <para>
+        Enable verbose output. Lists all checked files.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry>
        <term><option>-V</option></term>
        <term><option>--version</option></term>
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 37fe20bb75..a7f8d1d613 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -36,7 +36,7 @@ static int64 blocks = 0;
 static int64 badblocks = 0;
 static ControlFileData *ControlFile;
 
-static char *only_relfilenode = NULL;
+static char *only_filenode = NULL;
 static bool do_sync = true;
 static bool verbose = false;
 static bool showprogress = false;
@@ -76,16 +76,16 @@ usage(void)
 	printf(_("Usage:\n"));
 	printf(_("  %s [OPTION]... [DATADIR]\n"), progname);
 	printf(_("\nOptions:\n"));
-	printf(_(" [-D, --pgdata=]DATADIR  data directory\n"));
-	printf(_("  -c, --check            check data checksums (default)\n"));
-	printf(_("  -d, --disable          disable data checksums\n"));
-	printf(_("  -e, --enable           enable data checksums\n"));
-	printf(_("  -N, --no-sync          do not wait for changes to be written safely to disk\n"));
-	printf(_("  -P, --progress         show progress information\n"));
-	printf(_("  -v, --verbose          output verbose messages\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"));
+	printf(_(" [-D, --pgdata=]DATADIR    data directory\n"));
+	printf(_("  -c, --check              check data checksums (default)\n"));
+	printf(_("  -d, --disable            disable data checksums\n"));
+	printf(_("  -e, --enable             enable data checksums\n"));
+	printf(_("  -f, --filenode=FILENODE  check only relation with specified filenode\n"));
+	printf(_("  -N, --no-sync            do not wait for changes to be written safely to disk\n"));
+	printf(_("  -P, --progress           show progress information\n"));
+	printf(_("  -v, --verbose            output verbose messages\n"));
+	printf(_("  -V, --version            output version information, then exit\n"));
+	printf(_("  -?, --help               show this help, then exit\n"));
 	printf(_("\nIf no data directory (DATADIR) is specified, "
 			 "the environment variable PGDATA\nis used.\n\n"));
 	printf(_("Report bugs to <pgsql-b...@lists.postgresql.org>.\n"));
@@ -318,7 +318,7 @@ scan_directory(const char *basedir, const char *subdir, bool sizeonly)
 			/*
 			 * Cut off at the segment boundary (".") to get the segment number
 			 * in order to mix it into the checksum. Then also cut off at the
-			 * fork boundary, to get the relfilenode the file belongs to for
+			 * fork boundary, to get the filenode the file belongs to for
 			 * filtering.
 			 */
 			strlcpy(fnonly, de->d_name, sizeof(fnonly));
@@ -339,8 +339,8 @@ scan_directory(const char *basedir, const char *subdir, bool sizeonly)
 			if (forkpath != NULL)
 				*forkpath++ = '\0';
 
-			if (only_relfilenode && strcmp(only_relfilenode, fnonly) != 0)
-				/* Relfilenode not to be included */
+			if (only_filenode && strcmp(only_filenode, fnonly) != 0)
+				/* filenode not to be included */
 				continue;
 
 			dirsize += st.st_size;
@@ -373,6 +373,7 @@ main(int argc, char *argv[])
 		{"enable", no_argument, NULL, 'e'},
 		{"no-sync", no_argument, NULL, 'N'},
 		{"progress", no_argument, NULL, 'P'},
+		{"filenode", required_argument, NULL, 'f'},
 		{"verbose", no_argument, NULL, 'v'},
 		{NULL, 0, NULL, 0}
 	};
@@ -400,7 +401,7 @@ main(int argc, char *argv[])
 		}
 	}
 
-	while ((c = getopt_long(argc, argv, "cD:deNPr:v", long_options, &option_index)) != -1)
+	while ((c = getopt_long(argc, argv, "cD:deNPf:v", long_options, &option_index)) != -1)
 	{
 		switch (c)
 		{
@@ -422,13 +423,13 @@ main(int argc, char *argv[])
 			case 'D':
 				DataDir = optarg;
 				break;
-			case 'r':
+			case 'f':
 				if (atoi(optarg) == 0)
 				{
-					pg_log_error("invalid relfilenode specification, must be numeric: %s", optarg);
+					pg_log_error("invalid filenode specification, must be numeric: %s", optarg);
 					exit(1);
 				}
-				only_relfilenode = pstrdup(optarg);
+				only_filenode = pstrdup(optarg);
 				break;
 			case 'P':
 				showprogress = true;
@@ -465,10 +466,10 @@ main(int argc, char *argv[])
 		exit(1);
 	}
 
-	/* Relfilenode checking only works in --check mode */
-	if (mode != PG_MODE_CHECK && only_relfilenode)
+	/* filenode checking only works in --check mode */
+	if (mode != PG_MODE_CHECK && only_filenode)
 	{
-		pg_log_error("relfilenode option only possible with --check");
+		pg_log_error("--filenode option only possible with --check");
 		fprintf(stderr, _("Try \"%s --help\" for more information.\n"),
 				progname);
 		exit(1);
diff --git a/src/bin/pg_checksums/t/002_actions.pl b/src/bin/pg_checksums/t/002_actions.pl
index a8f45a268a..ce24d06a10 100644
--- a/src/bin/pg_checksums/t/002_actions.pl
+++ b/src/bin/pg_checksums/t/002_actions.pl
@@ -43,7 +43,7 @@ sub check_relation_corruption
 		[
 			'pg_checksums', '--check',
 			'-D',           $pgdata,
-			'-r',           $relfilenode_corrupted
+			'--filenode',   $relfilenode_corrupted
 		],
 		"succeeds for single relfilenode on tablespace $tablespace with offline cluster"
 	);
@@ -59,7 +59,7 @@ sub check_relation_corruption
 		[
 			'pg_checksums', '--check',
 			'-D',           $pgdata,
-			'-r',           $relfilenode_corrupted
+			'--filenode',   $relfilenode_corrupted
 		],
 		1,
 		[qr/Bad checksums:.*1/],
@@ -165,10 +165,10 @@ command_ok(
 # Specific relation files cannot be requested when action is --disable
 # or --enable.
 command_fails(
-	[ 'pg_checksums', '--disable', '-r', '1234', '-D', $pgdata ],
+	[ 'pg_checksums', '--disable', '--filenode', '1234', '-D', $pgdata ],
 	"fails when relfilenodes are requested and action is --disable");
 command_fails(
-	[ 'pg_checksums', '--enable', '-r', '1234', '-D', $pgdata ],
+	[ 'pg_checksums', '--enable', '--filenode', '1234', '-D', $pgdata ],
 	"fails when relfilenodes are requested and action is --enable");
 
 # Checks cannot happen with an online cluster

Attachment: signature.asc
Description: PGP signature

Reply via email to