Why does pg_checksums -r not have a long option?

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
15 messages Options
Reply | Threaded
Open this post in threaded view
|

Why does pg_checksums -r not have a long option?

Peter Eisentraut-6
Was this just forgotten?

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Fabien COELHO-3

> Subject: Why does pg_checksums -r not have a long option?
>
> Was this just forgotten?

Probably? Attached a patch.

--
Fabien.

checksums-long-option-1.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Michael Paquier-2
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

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Fabien COELHO-3

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.

checksums-long-option-2.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Daniel Gustafsson
In reply to this post by Michael Paquier-2
> On 27 May 2019, at 03:52, Michael Paquier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Michael Paquier-2
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Michael Banck-2
In reply to this post by Daniel Gustafsson
Hi,

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

> > On 27 May 2019, at 03:52, Michael Paquier <[hidden email]> 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
[hidden email] 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: [hidden email]

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


Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Michael Paquier-2
In reply to this post by Fabien COELHO-3
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

checksums-long-option-3.patch (8K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Fabien COELHO-3

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.


Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Michael Paquier-2
In reply to this post by Michael Banck-2
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
> [hidden email] 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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Fabien COELHO-3

>> |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.
Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Michael Paquier-2
In reply to this post by Fabien COELHO-3
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Peter Eisentraut-6
In reply to this post by Michael Paquier-2
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


Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Michael Paquier-2
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Why does pg_checksums -r not have a long option?

Tomas Vondra-4
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