Обсуждение: 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
> 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: 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
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
Вложения
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
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