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.

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=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"

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

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.

On 27 May 2019, at 03:52, Michael Paquier <michael@paquier.xyz> 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:

/messages/by-id/20180228194242.qbjasdtwm2yj5rqg@alvherre.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

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:

/messages/by-id/20180228194242.qbjasdtwm2yj5rqg@alvherre.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

Hi,

On Mon, May 27, 2019 at 09:22:42AM +0200, Daniel Gustafsson wrote:

On 27 May 2019, at 03:52, Michael Paquier <michael@paquier.xyz> 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:

/messages/by-id/20180228194242.qbjasdtwm2yj5rqg@alvherre.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+nsQZFew@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.banck@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

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

Bonjour Michael,

+     <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.

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.

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+nsQZFew@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

|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.

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

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

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

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