[patch] pg_test_timing does not prompt illegal option

On Wed, Apr 17, 2019 at 6:21 PM Zhang, Jie <zhangjie2@cn.fujitsu.com> wrote:

Hi all,

pg_test_timing accepts the following command-line options:
-d duration
--duration=duration

Specifies the test duration, in seconds. Longer durations give slightly better accuracy, and are more likely to discover problems with the system clock moving backwards. The default test duration is 3 seconds.
-V
--version

Print the pg_test_timing version and exit.
-?
--help

Show help about pg_test_timing command line arguments, and exit.

[https://www.postgresql.org/docs/11/pgtesttiming.html]

However, when I use the following command, no error prompt. the command can run.
pg_test_timing --

I think "--" is a illegal option, errors should be prompted.

Here is a patch for prompt illegal option.

This is not the problem only for pg_test_timing. If you want to
address this, the patch needs to cover all the client commands
like psql, createuser. I'm not sure if it's worth doing that.

Regards,

--
Fujii Masao

Fujii Masao <masao.fujii@gmail.com> writes:

On Wed, Apr 17, 2019 at 6:21 PM Zhang, Jie <zhangjie2@cn.fujitsu.com> wrote:

I think "--" is a illegal option, errors should be prompted.

This is not the problem only for pg_test_timing. If you want to
address this, the patch needs to cover all the client commands
like psql, createuser. I'm not sure if it's worth doing that.

I think it might be an actively bad idea. There's a pretty
widespread convention that "--" is a no-op switch indicating
the end of switches. At least some of our tools appear to
honor that behavior (probably because glibc's getopt_long
does; I do not think we are implementing it ourselves).

regards, tom lane

On Wed, Apr 17, 2019 at 10:24:17AM -0400, Tom Lane wrote:

Fujii Masao <masao.fujii@gmail.com> writes:

On Wed, Apr 17, 2019 at 6:21 PM Zhang, Jie <zhangjie2@cn.fujitsu.com> wrote:

I think "--" is a illegal option, errors should be prompted.

This is not the problem only for pg_test_timing. If you want to
address this, the patch needs to cover all the client commands
like psql, createuser. I'm not sure if it's worth doing that.

I think it might be an actively bad idea. There's a pretty
widespread convention that "--" is a no-op switch indicating
the end of switches. At least some of our tools appear to
honor that behavior (probably because glibc's getopt_long
does; I do not think we are implementing it ourselves).

Yep, a simple 'ls' on Debian stretch shows it is a common convention:

$ ls --
file1 file2

FYI, 'gcc --' (using Debian 6.3.0-18+deb9u1) does throw an error, so it
is inconsistent.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +

This is not the problem only for pg_test_timing. If you want to
address this, the patch needs to cover all the client commands
like psql, createuser. I'm not sure if it's worth doing that.

I think it might be an actively bad idea. There's a pretty
widespread convention that "--" is a no-op switch indicating
the end of switches. At least some of our tools appear to
honor that behavior (probably because glibc's getopt_long
does; I do not think we are implementing it ourselves).

"src/port/getopt_long.c" checks for "--" as the end of options.

--
Fabien.

Fabien COELHO <coelho@cri.ensmp.fr> writes:

I think it might be an actively bad idea. There's a pretty
widespread convention that "--" is a no-op switch indicating
the end of switches. At least some of our tools appear to
honor that behavior (probably because glibc's getopt_long
does; I do not think we are implementing it ourselves).

"src/port/getopt_long.c" checks for "--" as the end of options.

Ah. But I was checking this on a Linux build that's using glibc's
implementation, not our own. It's pretty easy to prove that psql,
for one, acts that way when using the glibc subroutine:

$ psql -- -E
psql: error: could not connect to server: FATAL: database "-E" does not exist

We've generally felt that deferring to the behavior of the platform's
getopt() or getopt_long() is a better idea than trying to enforce some
lowest-common-denominator version of switch parsing, on the theory that
users of a given platform will be used to whatever its getopt does.
This does mean that we have undocumented behaviors on particular
platforms. I'd say that accepting "--" is one of them. Another example
is that glibc's getopt is willing to reorder the arguments, so that
for example this works for me:

$ psql template1 -E
psql (12devel)
Type "help" for help.

template1=# \set
...
ECHO_HIDDEN = 'on'
...

On other platforms that would not work, so we don't document it.

regards, tom lane

Hello Tom,

We've generally felt that deferring to the behavior of the platform's
getopt() or getopt_long() is a better idea than trying to enforce some
lowest-common-denominator version of switch parsing, on the theory that
users of a given platform will be used to whatever its getopt does.
This does mean that we have undocumented behaviors on particular
platforms.

Interesting.

I'd say that accepting "--" is one of them. Another example is that
glibc's getopt is willing to reorder the arguments, so that for example
this works for me:

$ psql template1 -E
psql (12devel)

Yep, I noticed that one by accident once.

On other platforms that would not work, so we don't document it.

People might get surprised anyway, because the very same command may or
may not work depending on the platform. Does not matter much, though.

--
Fabien.