Re: Why does pg_checksums -r not have a long option?
On Thu, Jun 06, 2019 at 06:01:21PM +0900, Michael Paquier wrote: On Wed, Jun 05, 2019 at 10:31:54PM +0200, Peter Eisentraut wrote: I think -r/--relfilenode was actually a good suggestion. Because it doesn't actually check a *file* but potentially several files (forks, segments). The -f naming makes it sound like it operates on a specific file. Hmm. I still tend to prefer the -f/--filenode interface as that's more consistent with what we have in the documentation, where relfilenode gets only used when referring to the pg_class attribute. You have a point about the fork types and extra segments, but I am not sure that --relfilenode defines that in a better way than --filenode. -- I agree. The "rel" prefix is there mostly because the other pg_class attributes have it too (reltablespace, reltuples, ...) and we use "filenode" elsewhere. For example we have pg_relation_filenode() function, operating with exactly this piece of information. So +1 to keep the "-f/--filenode" options. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Why does pg_checksums -r not have a long option?
On Wed, Jun 05, 2019 at 10:31:54PM +0200, Peter Eisentraut wrote: > I think -r/--relfilenode was actually a good suggestion. Because it > doesn't actually check a *file* but potentially several files (forks, > segments). The -f naming makes it sound like it operates on a specific > file. Hmm. I still tend to prefer the -f/--filenode interface as that's more consistent with what we have in the documentation, where relfilenode gets only used when referring to the pg_class attribute. You have a point about the fork types and extra segments, but I am not sure that --relfilenode defines that in a better way than --filenode. -- Michael signature.asc Description: PGP signature
Re: Why does pg_checksums -r not have a long option?
On 2019-05-28 04:56, Michael Paquier wrote: > You could also use a long option for that without a one-letter option, > like --file-path or such, so reserving a one-letter option for a > future, hypothetical use is not really a stopper in my opinion. In > consequence, I think that that it is fine to just use -f/--filenode. > Any objections or better suggestions from other folks here? I think -r/--relfilenode was actually a good suggestion. Because it doesn't actually check a *file* but potentially several files (forks, segments). The -f naming makes it sound like it operates on a specific file. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Why does pg_checksums -r not have a long option?
On Mon, May 27, 2019 at 04:22:37PM +0200, Fabien COELHO wrote: > Works for me. Doc build is ok as well. Thanks, committed. -- Michael signature.asc Description: PGP signature
Re: Why does pg_checksums -r not have a long option?
|I have no problem with changing it to -r. -f seems a bit wrong to me, |as it might read as a file. And in the future we might want to implement |the ability to take full filename (with path), in which case it would |make sense to use -f for that. You could also use a long option for that without a one-letter option, like --file-path or such, so reserving a one-letter option for a future, hypothetical use is not really a stopper in my opinion. In consequence, I think that that it is fine to just use -f/--filenode. Yep. Also, the -f option could be overloaded by guessing whether is associated argument is a number or a path… Any objections or better suggestions from other folks here? -- Fabien.
Re: Why does pg_checksums -r not have a long option?
On Mon, May 27, 2019 at 10:17:43AM +0200, Michael Banck wrote: > Before we switch to -f out of consistency with oid2name, we should > consider Magnus' argument from > cabuevezoexaxbcymmzsnf1aqdcwovys7-chqcugry5+nsqz...@mail.gmail.com IMO: > > |I have no problem with changing it to -r. -f seems a bit wrong to me, > |as it might read as a file. And in the future we might want to implement > |the ability to take full filename (with path), in which case it would > |make sense to use -f for that. You could also use a long option for that without a one-letter option, like --file-path or such, so reserving a one-letter option for a future, hypothetical use is not really a stopper in my opinion. In consequence, I think that that it is fine to just use -f/--filenode. Any objections or better suggestions from other folks here? -- Michael signature.asc Description: PGP signature
Re: Why does pg_checksums -r not have a long option?
Bonjour Michael, + + -f filenode + --filenode=filenode + + +Only validate checksums in the relation with specified relation file node. + Two nits. I would just have been careful about the number of characters in the line within the markup. And we use extensively "filenode" in the docs. Ok. + [ 'pg_checksums', '--enable', '-filenode', '1234', '-D', $pgdata ], This fails, but not for the reason it is written for. Indeed. command_fails() is a little too simplistic, it should really check that the error message is there. It looks strange to not order --filenode alphabetically in --help. Forgot, it stayed at the r position for no good reason. With all these issues cleaned up, I got the attached. Does that look fine? (I ran pgperltidy and pgindent on top of it.) Works for me. Doc build is ok as well. -- Fabien.
Re: Why does pg_checksums -r not have a long option?
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.
+
+ -f filenode
+
--filenode=filenode
+
+
+Only validate checksums in the relation with specified relation file
node.
+
Two nits. I would just have been careful about the number of
characters in the line within the markup. And we use
extensively "filenode" in the docs. So the description would become
as follows:
Only validate checksums in the relation with filenode
filenode.
+ [ '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
+
+ -f filenode
+ --filenode=filenode
+
+
+Only validate checksums in the relation with filenode
+filenode.
+
+
+
+
-N
--no-sync
@@ -116,25 +127,6 @@ PostgreSQL documentation
-
- -v
- --verbose
-
-
-Enable verbose output. Lists all checked files.
-
-
-
-
-
- -r relfilenode
-
-
-Only validate checksums in the relation with specified relfilenode.
-
-
-
-
-P
--progress
@@ -146,6 +138,16 @@ PostgreSQL documentation
+
+ -v
+ --verbose
+
+
+Enable verbose output. Lists all checked files.
+
+
+
+
-V
--version
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, --checkcheck 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=]DATADIRdata directory\n"));
+ printf(_(" -c, --check check data checksums (default)\n"));
+ printf(_(" -d, --disabledisable data checksums\n"));
+ printf(_(" -e, --enable enable data checksums\n"));
+ printf(_(" -f, --filenode=FILENODE check only relation with specified filenode\n"));
+ printf(_(" -N, --no-syncdo not wait for changes to be written safely to disk\n"));
+ printf(_(" -P, --progress show progress information\n"));
+ printf(_(" -v, --verboseoutput verbose messages\n"));
+ printf(_(" -V, --versionoutput 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 .\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)
Re: Why does pg_checksums -r not have a long option?
Hi, On Mon, May 27, 2019 at 09:22:42AM +0200, Daniel Gustafsson wrote: > > On 27 May 2019, at 03:52, Michael Paquier wrote: > > 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. > > The original patch used -o in pg_verify_checksums, the discussion of which > started in the below mail: > > https://postgr.es/m/20180228194242.qbjasdtwm2yj5rqg%40alvherre.pgsql > > Since -f was already used for “force check”, -r ended up being used. Now that > there no longer is a -f flag in pg_checksums, it can be renamed. Before we switch to -f out of consistency with oid2name, we should consider Magnus' argument from cabuevezoexaxbcymmzsnf1aqdcwovys7-chqcugry5+nsqz...@mail.gmail.com IMO: |I have no problem with changing it to -r. -f seems a bit wrong to me, |as it might read as a file. And in the future we might want to implement |the ability to take full filename (with path), in which case it would |make sense to use -f for that. Cheers, Michael -- Michael Banck Projektleiter / Senior Berater Tel.: +49 2166 9901-171 Fax: +49 2166 9901-100 Email: michael.ba...@credativ.de credativ GmbH, HRB Mönchengladbach 12080 USt-ID-Nummer: DE204566209 Trompeterallee 108, 41189 Mönchengladbach Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer Unser Umgang mit personenbezogenen Daten unterliegt folgenden Bestimmungen: https://www.credativ.de/datenschutz
Re: Why does pg_checksums -r not have a long option?
On Mon, May 27, 2019 at 09:22:42AM +0200, Daniel Gustafsson wrote: > The original patch used -o in pg_verify_checksums, the discussion of which > started in the below mail: > > https://postgr.es/m/20180228194242.qbjasdtwm2yj5rqg%40alvherre.pgsql > > Since -f was already used for “force check”, -r ended up being used. Now that > there no longer is a -f flag in pg_checksums, it can be renamed. Interesting point. Thanks for sharing. -- Michael signature.asc Description: PGP signature
Re: Why does pg_checksums -r not have a long option?
> On 27 May 2019, at 03:52, Michael Paquier wrote: > 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. The original patch used -o in pg_verify_checksums, the discussion of which started in the below mail: https://postgr.es/m/20180228194242.qbjasdtwm2yj5rqg%40alvherre.pgsql Since -f was already used for “force check”, -r ended up being used. Now that there no longer is a -f flag in pg_checksums, it can be renamed. cheers ./daniel
Re: Why does pg_checksums -r not have a long option?
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=FILENODEshow 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
+
+ -f filenode
+ --filenode=filenode
+
+
+Only validate checksums in the relation with specified relation file node.
+
+
+
+
-N
--no-sync
@@ -116,25 +126,6 @@ PostgreSQL documentation
-
- -v
- --verbose
-
-
-Enable verbose output. Lists all checked files.
-
-
-
-
-
- -r relfilenode
-
-
-Only validate checksums in the relation with specified relfilenode.
-
-
-
-
-P
--progress
@@ -146,6 +137,16 @@ PostgreSQL documentation
+
+ -v
+ --verbose
+
+
+Enable verbose output. Lists all checked files.
+
+
+
+
-V
--version
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, _index)) != -1)
+ while ((c = getopt_long(argc, argv, "cD:deNPf:v", long_options, _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
Re: Why does pg_checksums -r not have a long option?
On Sun, May 26, 2019 at 08:35:30AM +0200, Fabien COELHO wrote: > Probably? Attached a patch. 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=FILENODEshow 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" I would also switch to the long option name in the tests at the same time, this makes the perl scripts easier to follow. -- Michael signature.asc Description: PGP signature
Re: Why does pg_checksums -r not have a long option?
Subject: Why does pg_checksums -r not have a long option?
Was this just forgotten?
Probably? Attached a patch.
--
Fabien.diff --git a/doc/src/sgml/ref/pg_checksums.sgml b/doc/src/sgml/ref/pg_checksums.sgml
index a0ffeb0ab0..5549ea679a 100644
--- a/doc/src/sgml/ref/pg_checksums.sgml
+++ b/doc/src/sgml/ref/pg_checksums.sgml
@@ -128,6 +128,7 @@ PostgreSQL documentation
-r relfilenode
+ --relfilenode=relfilenode
Only validate checksums in the relation with specified relfilenode.
diff --git a/src/bin/pg_checksums/pg_checksums.c b/src/bin/pg_checksums/pg_checksums.c
index 37fe20bb75..a994c51515 100644
--- a/src/bin/pg_checksums/pg_checksums.c
+++ b/src/bin/pg_checksums/pg_checksums.c
@@ -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(_(" [-r,--relfilenode]=NODE check only relation with specified relation 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, "
@@ -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'},
+ {"relfilenode", required_argument, NULL, 'r'},
{"verbose", no_argument, NULL, 'v'},
{NULL, 0, NULL, 0}
};
Why does pg_checksums -r not have a long option?
Was this just forgotten? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
