Thread: Fix help option of contrib/oid2name
Hi,
Almost all client applications and extensions will show "Options" and
"Connection options" sections when running with help option (--help).
However, "oid2name" was different, there is only the Options section.
For example,
$ vacuumlo --help
vacuumlo removes unreferenced large objects from databases.
Usage:
vacuumlo [OPTION]... DBNAME...
Options:
-l LIMIT commit after removing each LIMIT large objects
-n don't remove large objects, just show what would be done
-v write a lot of progress messages
-V, --version output version information, then exit
-?, --help show this help, then exit
Connection options:
-h HOSTNAME database server host or socket directory
-p PORT database server port
-U USERNAME user name to connect as
-w never prompt for password
-W force password prompt
$ oid2name --help
oid2name helps examining the file structure used by PostgreSQL.
Usage:
oid2name [OPTION]...
Options:
-d DBNAME database to connect to
-f FILENODE show info for table with given file node
-H HOSTNAME database server host or socket directory
-i show indexes and sequences too
-o OID show info for table with given OID
-p PORT database server port number
-q quiet (don't show headers)
-s show all tablespaces
-S show system objects too
-t TABLE show info for named table
-U NAME connect as specified database user
-V, --version output version information, then exit
-x extended (show additional columns)
-?, --help show this help, then exit
Above oid2name's "-d, -H, -p and -U" options are related to Connection
Options. So, it would be beter to write it in Connection options.
For consistency, attached patch divides the Options section of oid2name
into two sections, Options and Connection options.
Regards,
Tatsuro Yamada
NTT Open Source Software Center
Attachment
Tatsuro Yamada wrote: > Almost all client applications and extensions will show "Options" and > "Connection options" sections when running with help option (--help). > However, "oid2name" was different, there is only the Options section. > > For consistency, attached patch divides the Options section of oid2name > into two sections, Options and Connection options. I don't think it is super important, but +1 for consistency. Yours, Laurenz Albe
On Thu, Aug 16, 2018 at 12:40:42PM +0200, Laurenz Albe wrote: > I don't think it is super important, but +1 for consistency. I agree on both points. Any objections if I apply what's proposed here on HEAD? -- Michael
Attachment
On Thu, Aug 16, 2018 at 08:57:57PM +0900, Michael Paquier wrote: > I agree on both points. Any objections if I apply what's proposed here > on HEAD? I have been looking at this patch. And while consistency is nice, I think that if we are cleaning up this stuff we could do a bit more to be more consistent with the other binary tools: - Addition of long options for connection options. - removal of -h, and use it for host value instead of "-H". - Use "USERNAME" instead of "NAME" in help(). vacuumlo could also be improved a bit... -- Michael
Attachment
Hi Laurenz and Michael, On 2018/08/16 20:57, Michael Paquier wrote: > On Thu, Aug 16, 2018 at 12:40:42PM +0200, Laurenz Albe wrote: >> I don't think it is super important, but +1 for consistency. Thanks! :) > I agree on both points. Any objections if I apply what's proposed here > on HEAD? I have no objection. The patch is for improvement, not bug fix. But if you think it needs back-patch, please let me know, I can create it. Thank you for taking your time! Regards, Tatsuro Yamada NTT Open Source Software Center
On Fri, Aug 17, 2018 at 12:19:42PM +0900, Tatsuro Yamada wrote: > But if you think it needs back-patch, please let me know, I can create it. This would not be back-patched. -- Michael
Attachment
On 2018/08/17 12:31, Michael Paquier wrote: > On Fri, Aug 17, 2018 at 12:19:42PM +0900, Tatsuro Yamada wrote: >> But if you think it needs back-patch, please let me know, I can create it. > > This would not be back-patched. I see. Thanks, Tatsuro Yamada
On 2018/08/17 11:47, Michael Paquier wrote: > On Thu, Aug 16, 2018 at 08:57:57PM +0900, Michael Paquier wrote: >> I agree on both points. Any objections if I apply what's proposed here >> on HEAD? > > I have been looking at this patch. And while consistency is nice, I > think that if we are cleaning up this stuff we could do a bit more to > be more consistent with the other binary tools: > - Addition of long options for connection options. > - removal of -h, and use it for host value instead of "-H". > - Use "USERNAME" instead of "NAME" in help(). > > vacuumlo could also be improved a bit... Yeah, I'll revise the patch based on your suggestions. Regards, Tatsuro Yamada
On 2018/08/17 12:42, Tatsuro Yamada wrote: > On 2018/08/17 11:47, Michael Paquier wrote: >> On Thu, Aug 16, 2018 at 08:57:57PM +0900, Michael Paquier wrote: >>> I agree on both points. Any objections if I apply what's proposed here >>> on HEAD? >> >> I have been looking at this patch. And while consistency is nice, I >> think that if we are cleaning up this stuff we could do a bit more to >> be more consistent with the other binary tools: >> - Addition of long options for connection options. >> - removal of -h, and use it for host value instead of "-H". >> - Use "USERNAME" instead of "NAME" in help(). >> >> vacuumlo could also be improved a bit... > > Yeah, I'll revise the patch based on your suggestions. I created WIP patch for oid2name and vacuumlo includes followings: oid2name and vacuumlo - Add long options only oid2name - Replace -H with -h - Replace NAME with USERNAME I'll revise their documents and create new patch next week, probably. :) Regards, Tatsuro Yamada
Attachment
On 2018-Aug-17, Tatsuro Yamada wrote: > only oid2name > - Replace -H with -h I think this one is a bad idea, as it'll break scripts. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Aug-17, Tatsuro Yamada wrote:
>> only oid2name
>> - Replace -H with -h
> I think this one is a bad idea, as it'll break scripts.
Well, we can't remove the -H option, for that reason. But I think
we could get away with repurposing -h to also mean "--host", rather
than "--help" as it is now. Seems unlikely that any scripts are
depending on it to mean --help, nor are users likely to expect that
when none of our other programs treat it that way.
regards, tom lane
On August 17, 2018 10:53:48 PM GMT+09:00, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Well, we can't remove the -H option, for that reason. But I think > we could get away with repurposing -h to also mean "--host", rather > than "--help" as it is now. Seems unlikely that any scripts are > depending on it to mean --help, nor are users likely to expect that > when none of our other programs treat it that way. Yeah, I don't see much point in mapping -h to --help. Let's map silently -H to --host then. Would you prefer if -H is stilldocumented? Or would it be better if it is not documented, mapping silently? -- Michael
Michael Paquier <michael@paquier.xyz> writes:
> On August 17, 2018 10:53:48 PM GMT+09:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Well, we can't remove the -H option, for that reason. But I think
>> we could get away with repurposing -h to also mean "--host", rather
>> than "--help" as it is now. Seems unlikely that any scripts are
>> depending on it to mean --help, nor are users likely to expect that
>> when none of our other programs treat it that way.
> Yeah, I don't see much point in mapping -h to --help. Let's map silently -H to --host then. Would you prefer if -H is
stilldocumented? Or would it be better if it is not documented, mapping silently?
I think it probably needs to stay documented, but we could mark it as
deprecated ...
regards, tom lane
On August 18, 2018 10:52:33 AM GMT+09:00, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I think it probably needs to stay documented, but we could mark it as > deprecated ... Okay, no issues with doing so. -- Michael
> On August 18, 2018 10:52:33 AM GMT+09:00, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think it probably needs to stay documented, but we could mark it as
>> deprecated ...
>
> Okay, no issues with doing so.
I revised the patch like following:
vacuumlo:
Document
- Add long options
- Add environment section
oid2name:
Document
- Add long options
- Add environment section
- Remove deprecated and unhandled option "-P password"
Code
- Revive handling "-H host" option silently
I didn't add "-H" to the help message because it's a deprecated option.
I guess that that means "silently" as you said on previous email.
Should I add it? For example,
Not added "-H". (current patch)
--------
Connection options:
-d, --dbname=DBNAME database to connect to
-h, --host=HOSTNAME database server host or socket directory
-p, --port=PORT database server port number
-U, --username=USERNAME connect as specified database user
--------
Added "-H" to the help message after "-h"
--------
Connection options:
-d, --dbname=DBNAME database to connect to
-h, -H, --host=HOSTNAME database server host or socket directory
-p, --port=PORT database server port number
-U, --username=USERNAME connect as specified database user
--------
Please find the attached patch.
Regards,
Tatsuro Yamada
Attachment
On Mon, Aug 20, 2018 at 12:30:29PM +0900, Tatsuro Yamada wrote: > vacuumlo: > Document > - Add long options > - Add environment section Let's keep things simple by not adding long options where it is not especially obvious, so I would suggest to keep the patch simple and just add long options for connection options. > oid2name: > Document > - Add long options > - Add environment section > - Remove deprecated and unhandled option "-P password" Is this a good idea? I doubt that it is used but keeping it would not cause any breakage, and removing it could. oid2name supports also PGDATABASE. > Code > - Revive handling "-H host" option silently > > I didn't add "-H" to the help message because it's a deprecated > option. It seems to me that this should be still documented in both --help and in the docs, and that we should just mark it as deprecated in both. > I guess that that means "silently" as you said on previous email. > Should I add it? For example, > > [...] Let's use a different line for that. See for example how pg_standby -k is treated. I can see why you have reordered the options, this is more consistent with things in src/bin/scripts. We could have as well some basic TAP tests for both utilities while on it. Something as simple as the first tests in 050_dropdb.pl would do a fine first shot. -- Michael
Attachment
On 2018/08/20 13:54, Michael Paquier wrote:
> On Mon, Aug 20, 2018 at 12:30:29PM +0900, Tatsuro Yamada wrote:
>> vacuumlo:
>> Document
>> - Add long options
>> - Add environment section
>
> Let's keep things simple by not adding long options where it is not
> especially obvious, so I would suggest to keep the patch simple and just
> add long options for connection options.
Okay.
>> oid2name:
>> Document
>> - Add long options
>> - Add environment section
>> - Remove deprecated and unhandled option "-P password"
>
> Is this a good idea? I doubt that it is used but keeping it would not
> cause any breakage, and removing it could.
Yeah, why I remove that code is that there is no code to handle "-P" option,
and it caused error when executing the command with the option like this:
$ oid2name -P hoge
oid2name: invalid option -- 'P'
Therefore, "-P" is a manual bag. I investigated more using git log command and
understood followings:
1. -P option was removed on 4192f2d85
2. -P option revived in only the manual on 2963d8228
So, we have 2 options, remove -P option from the manual or revive the code
regarding to -P option (I mean it is like a git revert 4192f2d85).
> oid2name supports also PGDATABASE.
As far as I know, this environment variable is also unused because
I could not get the results as I expected. So, I didn't add it to the manual.
For example, following command expected that it shows about "postgres", but
it couldn't.
$ env |grep PG
...
PGDATABASE=postgres
...
$ oid2name
All databases:
Oid Database Name Tablespace
----------------------------------
16392 hogedb pg_default
13310 postgres pg_default
13309 template0 pg_default
1 template1 pg_default
$ oid2name -d postgres
From database "postgres":
Filenode Table Name
----------------------
16388 t1
>> Code
>> - Revive handling "-H host" option silently
>>
>> I didn't add "-H" to the help message because it's a deprecated
>> option.
>
> It seems to me that this should be still documented in both --help and
> in the docs, and that we should just mark it as deprecated in both.
>
>> I guess that that means "silently" as you said on previous email.
>> Should I add it? For example,
>>
>> [...]
>
> Let's use a different line for that. See for example how pg_standby -k
> is treated.
I will do that.
> I can see why you have reordered the options, this is more consistent
> with things in src/bin/scripts.
>
> We could have as well some basic TAP tests for both utilities while on
> it. Something as simple as the first tests in 050_dropdb.pl would do a
> fine first shot.
For now, I'm not sure about TAP test but I knew both utilities have no
regression tests. It would be better to use something tests.
Regards,
Tatsuro Yamada
On Mon, Aug 20, 2018 at 03:51:07PM +0900, Tatsuro Yamada wrote: > On 2018/08/20 13:54, Michael Paquier wrote: > Therefore, "-P" is a manual bag. I investigated more using git log command and > understood followings: > > 1. -P option was removed on 4192f2d85 > 2. -P option revived in only the manual on 2963d8228 Bruce did that to keep a trace of it in the docs, let's nuke it then, we don't handle it and the documentation is mentioning the thing as deprecated since 2010. >> oid2name supports also PGDATABASE. > > As far as I know, this environment variable is also unused because > I could not get the results as I expected. So, I didn't add it to the manual. > For example, following command expected that it shows about "postgres", but > it couldn't. Yep, you're right. > For now, I'm not sure about TAP test but I knew both utilities have no > regression tests. It would be better to use something tests. If you don't add them, I think that I would just that myself, just to have some basics. Not that it is a barrier for this patch anyway. -- Michael
Attachment
On 2018/08/20 17:38, Michael Paquier wrote:
> On Mon, Aug 20, 2018 at 03:51:07PM +0900, Tatsuro Yamada wrote:
>> On 2018/08/20 13:54, Michael Paquier wrote:
>> Therefore, "-P" is a manual bag. I investigated more using git log command and
>> understood followings:
>>
>> 1. -P option was removed on 4192f2d85
>> 2. -P option revived in only the manual on 2963d8228
>
> Bruce did that to keep a trace of it in the docs, let's nuke it then, we
> don't handle it and the documentation is mentioning the thing as
> deprecated since 2010.
Yep, right.
I see, and will remove -P option's explanation from the manual.
>> For now, I'm not sure about TAP test but I knew both utilities have no
>> regression tests. It would be better to use something tests.
>
> If you don't add them, I think that I would just that myself, just to
> have some basics. Not that it is a barrier for this patch anyway.
Hmm...
As long as I've come this far, I'll do it through.
BTW, can I add the patch to the Commitfest September?
The patch includes improvements and bug fix as you know, So, I can divide the
patch into 2 patches for that.
Which one is better to create, 2 patches or 1 patch?
I summed up fixes of oid2name and vacuumlo so far, the next patch will include
following stuffs:
oid2name
bug fix
- Remove "-P password" option from the document
improvements
- Divide "Options section" into "Options" and "Connection Options" sections in
the help message (--help)
- Add long options only for Connection options (-d, -H, -h, -p and -U)
- Add "-H host" and "-h host" options to the help message and the document
- Add environment variable (PGHOST, PGPORT and PGUSER) to the document
- Add TAP tests for checking command-line options
vacuumlo
improvements
- Add long options only for Connection options (-h, -p and -U)
- Add environment variable (PGHOST, PGPORT and PGUSER) to the document
- Add TAP tests for checking command-line options
Thanks,
Tatsuro Yamada
On Tue, Aug 21, 2018 at 12:26:15PM +0900, Tatsuro Yamada wrote: > BTW, can I add the patch to the Commitfest September? You should. > The patch includes improvements and bug fix as you know, So, I can divide the > patch into 2 patches for that. I am not really seeing any bug fix, but if you think so then splitting into two patches would help in making the difference for sure. -- Michael
Attachment
On 2018/08/21 12:40, Michael Paquier wrote:
> On Tue, Aug 21, 2018 at 12:26:15PM +0900, Tatsuro Yamada wrote:
>> BTW, can I add the patch to the Commitfest September?
>
> You should.
Thanks, I'll do that.
>> The patch includes improvements and bug fix as you know, So, I can divide the
>> patch into 2 patches for that.
>
> I am not really seeing any bug fix, but if you think so then splitting
> into two patches would help in making the difference for sure.
Yep, I meant the bug fix is below:
oid2name
- Remove "-P password" option from the document
As I wrote before, "-P option" is not works well because there is no code to handle
that option in the code. So, it will be removed on next patch.
I'll send 2 patches in this week, probably.
Regards,
Tatsuro Yamada
On 2018/08/21 12:57, Tatsuro Yamada wrote:
> On 2018/08/21 12:40, Michael Paquier wrote:
>> On Tue, Aug 21, 2018 at 12:26:15PM +0900, Tatsuro Yamada wrote:
>>> BTW, can I add the patch to the Commitfest September?
>>
>> You should.
>
> Thanks, I'll do that.
> I'll send 2 patches in this week, probably.
I revised the patch and created new tap tests.
fix_oid2name_wip3.patch:
bug fix
- Remove "-P password" option from the document
improvements
- Divide "Options section" into "Options" and "Connection Options" sections in
the help message (--help)
- Add long options
- Add "-H host" and "-h host" options to the help message and the document
- Add environment variable (PGHOST, PGPORT and PGUSER) to the document
fix_vacuumlo_wip3.patch:
improvements
- Add long options
- Add environment variable (PGHOST, PGPORT and PGUSER) to the document
Regarding to Long options,
I read the following both codes and I understand meanings of short options, so I
leave all long options in patches not only for connection options.
oid2name.c
/* these are the opts structures for command line params */
struct options
{
eary *tables;
eary *oids;
eary *filenodes;
bool quiet;
bool systables;
bool indexes;
bool nodb;
bool extended;
bool tablespaces;
char *dbname;
char *hostname;
char *port;
char *username;
const char *progname;
};
vacuumlo.c
struct _param
{
char *pg_user;
enum trivalue pg_prompt;
char *pg_port;
char *pg_host;
const char *progname;
int verbose;
int dry_run;
long transaction_limit;
};
Following TAP tests is for checking command-line options but I coudn't add some
test cases such as it is required argument and mixed varaiety of options.
I'm not sure about naming rule of tap test, so I added 300 and 301 to names as
temporarily. Please rename these files to more suitable names.
300_oid2name.pl
301_vacuumlo.pl
Please find attached files.
Thanks,
Tatsuro Yamada
Attachment
On 2018/08/21 12:57, Tatsuro Yamada wrote:
> On 2018/08/21 12:40, Michael Paquier wrote:
>> On Tue, Aug 21, 2018 at 12:26:15PM +0900, Tatsuro Yamada wrote:
>>> BTW, can I add the patch to the Commitfest September?
>>
>> You should.
>
> Thanks, I'll do that.
> I'll send 2 patches in this week, probably.
(I resend this email because I forgot to mark CC on my previous email. Sorry.)
I revised the patch and created new tap tests.
fix_oid2name_wip3.patch:
bug fix
- Remove "-P password" option from the document
improvements
- Divide "Options section" into "Options" and "Connection Options" sections in
the help message (--help)
- Add long options
- Add "-H host" and "-h host" options to the help message and the document
- Add environment variable (PGHOST, PGPORT and PGUSER) to the document
fix_vacuumlo_wip3.patch:
improvements
- Add long options
- Add environment variable (PGHOST, PGPORT and PGUSER) to the document
Regarding to Long options,
I read the following both codes and I understand meanings of short options, so I
leave all long options in patches not only for connection options.
oid2name.c
/* these are the opts structures for command line params */
struct options
{
eary *tables;
eary *oids;
eary *filenodes;
bool quiet;
bool systables;
bool indexes;
bool nodb;
bool extended;
bool tablespaces;
char *dbname;
char *hostname;
char *port;
char *username;
const char *progname;
};
vacuumlo.c
struct _param
{
char *pg_user;
enum trivalue pg_prompt;
char *pg_port;
char *pg_host;
const char *progname;
int verbose;
int dry_run;
long transaction_limit;
};
Following TAP tests is for checking command-line options but I coudn't add some
test cases such as it is required argument and mixed varaiety of options.
I'm not sure about naming rule of tap test, so I added 300 and 301 to names as
temporarily. Please rename these files to more suitable names.
300_oid2name.pl
301_vacuumlo.pl
Please find attached files.
Thanks,
Tatsuro Yamada
Attachment
On Fri, Aug 24, 2018 at 01:32:47PM +0900, Tatsuro Yamada wrote: > I revised the patch and created new tap tests. Thanks, I have looked at the patch set. And here are some notes about issues found while reviewing. For oid2name: - Documentation had some typos, --tablename was used instead of --table. - Using plural forms for some options makes more sense to me, like --indexes, --system-objects, --tablespaces - I have tweaked the description of -H in usage(). - An '=' was missing for --filenode - I have reordered the options in alphabetical order. - Added some basic tap tests, and added correct handling of the tests in oid2name/Makefile. - Inclusion of getopt_long.h has been missing, this would have caused failures on Windows. - getopt_long() has been reordered to be alphabetical. Same thing for long_options[]. This has the advantage to ease the review and code read. - Some formatting issues with option docs, leading to some noise diffs. For vacuumlo: - Some typos in documentation. - Documentation format was rather weird for some options, so I made the whole consistent. - I agree with the option names you put. - Reorganization of the options. - Added basic TAP tests, and fixed Makefile. - default clause was missing from the option set. Attached are updated patches, which I would be fine to commit. What do you think? -- Michael
Attachment
On Mon, Aug 27, 2018 at 07:03:18PM +0900, Michael Paquier wrote: > Thanks, I have looked at the patch set. I have been through the set once again, and pushed both things. Thanks a lot Yamada-san. -- Michael
Attachment
On 2018/08/28 22:36, Michael Paquier wrote: > On Mon, Aug 27, 2018 at 07:03:18PM +0900, Michael Paquier wrote: >> Thanks, I have looked at the patch set. > > I have been through the set once again, and pushed both things. Thanks > a lot Yamada-san. Thank you very much for your time to review and revise the patch! :) In this year, I will try to review someone's patch as you did. Please let me know if you need reviewer. Regards, Tatsuro Yamada