Horton

Thread

Commits

  1. Rework options of pg_checksums options for filenode handling

  1. Why does pg_checksums -r not have a long option?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-05-26T06:12:30Z

    Was this just forgotten?

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

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

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-05-26T06:35:30Z

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

Probably? Attached a patch.

-- 
Fabien.

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

    Michael Paquier <michael@paquier.xyz> — 2019-05-27T01:52:04Z

    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

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

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-05-27T06:32:21Z

    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.

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

    Daniel Gustafsson <daniel@yesql.se> — 2019-05-27T07:22:42Z

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

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

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

    Michael Paquier <michael@paquier.xyz> — 2019-05-27T08:05:12Z

    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

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

    Michael Banck <michael.banck@credativ.de> — 2019-05-27T08:17:43Z

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

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

    Michael Paquier <michael@paquier.xyz> — 2019-05-27T08:33:30Z

    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

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

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-05-27T14:22:37Z

    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.

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

    Michael Paquier <michael@paquier.xyz> — 2019-05-28T02:56:48Z

    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

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

    Fabien COELHO <coelho@cri.ensmp.fr> — 2019-05-28T07:59:37Z

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

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

    Michael Paquier <michael@paquier.xyz> — 2019-05-30T21:55:05Z

    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

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

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-06-05T20:31:54Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2019-06-06T09:01:21Z

    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

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

    Tomas Vondra <tomas.vondra@2ndquadrant.com> — 2019-06-09T11:02:33Z

    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