Fix help option of contrib/oid2name

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

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

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

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

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 still documented? 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 still documented? 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

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

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

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