Thread: [patch] pg_test_timing does not prompt illegal option
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
discoverproblems 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.
Best Regards!
Attachment
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 discoverproblems 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.