Hello Michael-san,
No objections with adding a long option for that stuff. But I do have
an objection with the naming because we have another tool able to work
on relfilenodes:
$ oid2name --help | grep FILE
-f, --filenode=FILENODE show info for table with given file node
In this case, long options are new as of 1aaf532 which is recent, but
-f is around for a much longer time.
Perhaps we should use the same mapping for consistency?
pg_verify_checksums has been using -r for whatever reason, but as we
do a renaming of the binary for v12 we could just fix that
inconsistency as well. Hence I would suggest that for the option
description:
"-f, --filenode=FILENODE"
Fine with me, I was not particularly happy with "relfilenode", but just
picked it up for consistency with -r.
I would also switch to the long option name in the tests at the same
time, this makes the perl scripts easier to follow.
Ok. Attached.
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.
--
Fabien.
diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index a0ffeb0ab0..6d8dd6be29 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -100,6 +100,16 @@ 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 specified relation file node.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>-N</option></term>
<term><option>--no-sync</option></term>
@@ -116,25 +126,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 +137,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..cd621e5417 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;
@@ -83,7 +83,7 @@ usage(void)
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(_(" [-f, --filenode]=NODE check only relation with specified file node\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, "
@@ -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 relation file node the file belongs to for
* filtering.
*/
strlcpy(fnonly, de->d_name, sizeof(fnonly));
@@ -339,7 +339,7 @@ scan_directory(const char *basedir, const char *subdir, bool sizeonly)
if (forkpath != NULL)
*forkpath++ = '\0';
- if (only_relfilenode && strcmp(only_relfilenode, fnonly) != 0)
+ if (only_filenode && strcmp(only_filenode, fnonly) != 0)
/* Relfilenode not to be included */
continue;
@@ -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 file node specification, must be numeric: %s", optarg);
exit(1);
}
- only_relfilenode = pstrdup(optarg);
+ only_filenode = pstrdup(optarg);
break;
case 'P':
showprogress = true;
@@ -466,9 +467,9 @@ main(int argc, char *argv[])
}
/* Relfilenode checking only works in --check mode */
- if (mode != PG_MODE_CHECK && only_relfilenode)
+ 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..be05ee6e66 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
+ '-f', $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
+ '-f', $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
