Обсуждение: add non-option reordering to in-tree getopt_long

While working on 2dcd157, I noticed cfbot failures for Windows due to tests
with commands that had non-options specified before options.  For example,
"createuser myrole -a myadmin" specified a non-option (myrole) before the
option/argument pair (-a admin).  To get the tests passing for Windows,
non-options must be placed at the end (e.g., "createuser -a myadmin
myrole").  This same problem was encountered while working on 08951a7 [0],
but it was again fortunately caught with cfbot.  Others have not been so
lucky [1] [2] [3].

The reason for this discrepancy is because Windows uses the in-tree
implementation of getopt_long(), which differs from the other
implementations I've found in that it doesn't reorder non-options to the
end of argv by default.  Instead, it returns -1 as soon as the first
non-option is found, even if there are other options listed afterwards.  By
moving non-options to the end of argv, getopt_long() can parse all
specified options and return -1 when only non-options remain.  The
implementations I reviewed all reorder argv unless the POSIXLY_CORRECT
environment variable is set or the "optstring" argument begins with '+'.

The best reasons I can think of to keep the current behavior are 1)
reordering involves writing to the original argv array, which could be
risky (as noted by Tom [4]) and 2) any systems with a getopt_long()
implementation that doesn't reorder non-options could begin failing tests
that take advantage of this behavior.  However, this quirk in the in-tree
getopt_long() is periodically missed by hackers, the only systems I'm aware
of that use it are Windows and AIX, and every other implementation of
getopt_long() I saw reorders non-options by default.  Furthermore, C99
omits const decorations in main()'s signature, so modifying argv might not
be too scary.

Thus, I propose introducing this non-option reordering behavior but
allowing it to be disabled via the POSIXLY_CORRECT environment variable.
The attached patch is my first attempt at implementing this proposal.  I
don't think we need to bother with handling '+' at the beginning of
"optstring" since it seems unlikely to be used in PostgreSQL, but it would
probably be easy enough to add if folks want it.

I briefly looked at getopt() and concluded that we should probably retain
its POSIX-compliant behavior for non-options, as reordering support seems
much less universal than with getopt_long().  AFAICT all client utilities
use getopt_long(), anyway.

Thoughts?

[0] https://postgr.es/m/20220525.110752.305692011781436338.horikyota.ntt%40gmail.com
[1] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=869aa40
[2] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=ffd3980
[3] https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=d9ddc50
[4] https://postgr.es/m/20539.1237314382%40sss.pgh.pa.us

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

At Fri, 9 Jun 2023 16:22:57 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> While working on 2dcd157, I noticed cfbot failures for Windows due to tests
> with commands that had non-options specified before options.  For example,
> "createuser myrole -a myadmin" specified a non-option (myrole) before the
> option/argument pair (-a admin).  To get the tests passing for Windows,
> non-options must be placed at the end (e.g., "createuser -a myadmin
> myrole").  This same problem was encountered while working on 08951a7 [0],
> but it was again fortunately caught with cfbot.  Others have not been so
> lucky [1] [2] [3].

While I don't see it as reason to change the behavior, I do believe
the change could be beneficial from a user's perspective.

> The reason for this discrepancy is because Windows uses the in-tree
> implementation of getopt_long(), which differs from the other
> implementations I've found in that it doesn't reorder non-options to the
> end of argv by default.  Instead, it returns -1 as soon as the first
> non-option is found, even if there are other options listed afterwards.  By
> moving non-options to the end of argv, getopt_long() can parse all
> specified options and return -1 when only non-options remain.  The
> implementations I reviewed all reorder argv unless the POSIXLY_CORRECT
> environment variable is set or the "optstring" argument begins with '+'.
>
> The best reasons I can think of to keep the current behavior are 1)
> reordering involves writing to the original argv array, which could be
> risky (as noted by Tom [4]) and 2) any systems with a getopt_long()
> implementation that doesn't reorder non-options could begin failing tests
> that take advantage of this behavior.  However, this quirk in the in-tree
> getopt_long() is periodically missed by hackers, the only systems I'm aware
> of that use it are Windows and AIX, and every other implementation of
> getopt_long() I saw reorders non-options by default.  Furthermore, C99
> omits const decorations in main()'s signature, so modifying argv might not
> be too scary.

POSIXLY_CORRECT appears to be intended for debugging or feature
validation. If we know we can always rearrange argv on those
platforms, we don't need it.  I would suggest that we turn on the new
feature at the compile time on those platforms where we know this
rearrangement works, instead of switching at runtime.

> Thus, I propose introducing this non-option reordering behavior but
> allowing it to be disabled via the POSIXLY_CORRECT environment variable.
> The attached patch is my first attempt at implementing this proposal.  I
> don't think we need to bother with handling '+' at the beginning of
> "optstring" since it seems unlikely to be used in PostgreSQL, but it would
> probably be easy enough to add if folks want it.
> 
> I briefly looked at getopt() and concluded that we should probably retain
> its POSIX-compliant behavior for non-options, as reordering support seems
> much less universal than with getopt_long().  AFAICT all client utilities
> use getopt_long(), anyway.
> 
> Thoughts?

As far as I can see, getopt_long on Rocky9 does *not* rearrange argv
until it reaches the end of the array. But it won't matter much.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jun 13, 2023 at 12:00:01PM +0900, Kyotaro Horiguchi wrote:
> POSIXLY_CORRECT appears to be intended for debugging or feature
> validation. If we know we can always rearrange argv on those
> platforms, we don't need it.  I would suggest that we turn on the new
> feature at the compile time on those platforms where we know this
> rearrangement works, instead of switching at runtime.

I'd be okay with leaving it out wherever possible.  I'm curious whether any
supported systems do not allow this.

> As far as I can see, getopt_long on Rocky9 does *not* rearrange argv
> until it reaches the end of the array. But it won't matter much.

Do you mean that it rearranges argv once all the options have been
returned, or that it doesn't rearrange argv at all?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

At Mon, 12 Jun 2023 22:13:43 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> On Tue, Jun 13, 2023 at 12:00:01PM +0900, Kyotaro Horiguchi wrote:
> > POSIXLY_CORRECT appears to be intended for debugging or feature
> > validation. If we know we can always rearrange argv on those
> > platforms, we don't need it.  I would suggest that we turn on the new
> > feature at the compile time on those platforms where we know this
> > rearrangement works, instead of switching at runtime.
> 
> I'd be okay with leaving it out wherever possible.  I'm curious whether any
> supported systems do not allow this.

Hmm. from the initial mail, I got the impression that AIX and Windows
allow this, so I thought that we can do that for them.  While there
could be other platforms that allow it, perhaps we don't need to go as
far as extending this until we come across another platform that does.

> > As far as I can see, getopt_long on Rocky9 does *not* rearrange argv
> > until it reaches the end of the array. But it won't matter much.
> 
> Do you mean that it rearranges argv once all the options have been
> returned, or that it doesn't rearrange argv at all?

I meant the former. argv remains unaltered until getopt_long returns
-1. Once it does, non-optional arguments are being collected at the
end of argv.  (But in my opinion, that behavior isn't very significant
in this context..)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jun 13, 2023 at 04:02:01PM +0900, Kyotaro Horiguchi wrote:
> Hmm. from the initial mail, I got the impression that AIX and Windows
> allow this, so I thought that we can do that for them.  While there
> could be other platforms that allow it, perhaps we don't need to go as
> far as extending this until we come across another platform that does.

Windows seems to allow rearranging argv, based upon cfbot's results.  I do
not know about AIX.  In any case, C99 explicitly mentions that argv should
be modifiable.

>> > As far as I can see, getopt_long on Rocky9 does *not* rearrange argv
>> > until it reaches the end of the array. But it won't matter much.
>> 
>> Do you mean that it rearranges argv once all the options have been
>> returned, or that it doesn't rearrange argv at all?
> 
> I meant the former. argv remains unaltered until getopt_long returns
> -1. Once it does, non-optional arguments are being collected at the
> end of argv.  (But in my opinion, that behavior isn't very significant
> in this context..)

Got it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jun 13, 2023 at 03:36:57PM -0700, Nathan Bossart wrote:
> Windows seems to allow rearranging argv, based upon cfbot's results.  I do
> not know about AIX.  In any case, C99 explicitly mentions that argv should
> be modifiable.

Few people have AIX machines around these days, but looking around it
seems like the answer to this question would be no:
https://github.com/nodejs/node/pull/10633

Noah, do you have an idea?

> Got it.

Making the internal implementation of getopt_long more flexible would
be really nice in the long-term.  This is something that people have
stepped on for many years, like ffd3980.

(TBH, I think that there is little value in spending resources on AIX
these days.  For one, few have an access to it, and getopt is not the
only tweak in the tree related to it.  On top of that, C99 is required
since v12.)
--
Michael

On Wed, Jun 14, 2023 at 08:52:27AM +0900, Michael Paquier wrote:
> On Tue, Jun 13, 2023 at 03:36:57PM -0700, Nathan Bossart wrote:
>> Windows seems to allow rearranging argv, based upon cfbot's results.  I do
>> not know about AIX.  In any case, C99 explicitly mentions that argv should
>> be modifiable.
>
> Few people have AIX machines around these days, but looking around it
> seems like the answer to this question would be no:
> https://github.com/nodejs/node/pull/10633
>
> Noah, do you have an idea?

Forgot to add Noah in CC about this part.
--
Michael

On Wed, Jun 14, 2023 at 09:03:22AM +0900, Michael Paquier wrote:
> On Wed, Jun 14, 2023 at 08:52:27AM +0900, Michael Paquier wrote:
> > On Tue, Jun 13, 2023 at 03:36:57PM -0700, Nathan Bossart wrote:
> >> Windows seems to allow rearranging argv, based upon cfbot's results.  I do
> >> not know about AIX.  In any case, C99 explicitly mentions that argv should
> >> be modifiable.
> > 
> > Few people have AIX machines around these days, but looking around it
> > seems like the answer to this question would be no:
> > https://github.com/nodejs/node/pull/10633
> > 
> > Noah, do you have an idea?

No, I don't have specific knowledge about mutating argv on AIX.  PostgreSQL
includes this, which makes some statement about it working:

#elif ... || defined(_AIX) || ...
#define PS_USE_CLOBBER_ARGV

If you have a test program to be run, I can run it on AIX.

On Tue, Jun 13, 2023 at 05:17:37PM -0700, Noah Misch wrote:
> If you have a test program to be run, I can run it on AIX.

Thanks.  The patch above [0] adjusts 040_createuser.pl to test modifying
argv, so that's one test program.  And here's a few lines for reversing
argv:

    #include <stdio.h>

    int
    main(int argc, char *argv[])
    {
        for (int i = 0; i < argc / 2; i++)
        {
            char       *tmp = argv[i];

            argv[i] = argv[argc - i - 1];
            argv[argc - i - 1] = tmp;
        }

        for (int i = 0; i < argc; i++)
            printf("%s ", argv[i]);
        printf("\n");
    }

[0] https://postgr.es/m/attachment/147420/v1-0001-Teach-in-tree-getopt_long-to-move-non-options-to-.patch

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Jun 14, 2023 at 02:28:16PM -0700, Nathan Bossart wrote:
> On Tue, Jun 13, 2023 at 05:17:37PM -0700, Noah Misch wrote:
> > If you have a test program to be run, I can run it on AIX.
> 
> Thanks.  The patch above [0] adjusts 040_createuser.pl to test modifying
> argv, so that's one test program.  And here's a few lines for reversing
> argv:
> 
>     #include <stdio.h>
> 
>     int
>     main(int argc, char *argv[])
>     {
>         for (int i = 0; i < argc / 2; i++)
>         {
>             char       *tmp = argv[i];
> 
>             argv[i] = argv[argc - i - 1];
>             argv[argc - i - 1] = tmp;
>         }
> 
>         for (int i = 0; i < argc; i++)
>             printf("%s ", argv[i]);
>         printf("\n");
>     }

Here's some output from this program (on AIX 7.1, same output when compiled
32-bit or 64-bit):

$ ./a.out a b c d e f
f e d c b a ./a.out

Interesting discussion here, too:
https://github.com/libuv/libuv/pull/1187

On Wed, Jun 14, 2023 at 03:11:54PM -0700, Noah Misch wrote:
> Here's some output from this program (on AIX 7.1, same output when compiled
> 32-bit or 64-bit):
> 
> $ ./a.out a b c d e f
> f e d c b a ./a.out

Thanks again.

> Interesting discussion here, too:
> https://github.com/libuv/libuv/pull/1187

Hm.  IIUC modifying the argv pointers on AIX will modify the process title,
which could cause 'ps' to temporarily show duplicate/missing arguments
during option parsing.  That doesn't seem too terrible, but if pointer
assignments aren't atomic, maybe 'ps' could be sent off to another part of
memory, which does seem terrible.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

At Wed, 14 Jun 2023 15:46:08 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> On Wed, Jun 14, 2023 at 03:11:54PM -0700, Noah Misch wrote:
> > Here's some output from this program (on AIX 7.1, same output when compiled
> > 32-bit or 64-bit):
> > 
> > $ ./a.out a b c d e f
> > f e d c b a ./a.out
> 
> Thanks again.
> 
> > Interesting discussion here, too:
> > https://github.com/libuv/libuv/pull/1187
> 
> Hm.  IIUC modifying the argv pointers on AIX will modify the process title,
> which could cause 'ps' to temporarily show duplicate/missing arguments
> during option parsing.  That doesn't seem too terrible, but if pointer
> assignments aren't atomic, maybe 'ps' could be sent off to another part of
> memory, which does seem terrible.

Hmm, the discussion seems to be based on the assumption that argv[0]
can be safely redirected to a different memory location. If that's the
case, we can prpbably rearrange the array, even if there's a small
window where ps might display a confusing command line, right?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Jun 15, 2023 at 02:30:34PM +0900, Kyotaro Horiguchi wrote:
> At Wed, 14 Jun 2023 15:46:08 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
>> Hm.  IIUC modifying the argv pointers on AIX will modify the process title,
>> which could cause 'ps' to temporarily show duplicate/missing arguments
>> during option parsing.  That doesn't seem too terrible, but if pointer
>> assignments aren't atomic, maybe 'ps' could be sent off to another part of
>> memory, which does seem terrible.
> 
> Hmm, the discussion seems to be based on the assumption that argv[0]
> can be safely redirected to a different memory location. If that's the
> case, we can prpbably rearrange the array, even if there's a small
> window where ps might display a confusing command line, right?

If that's the extent of the breakage, then it seems alright to me.  I've
attached a new version of the patch that omits the POSIXLY_CORRECT stuff.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jun 15, 2023 at 05:09:59PM -0700, Nathan Bossart wrote:
> On Thu, Jun 15, 2023 at 02:30:34PM +0900, Kyotaro Horiguchi wrote:
>> Hmm, the discussion seems to be based on the assumption that argv[0]
>> can be safely redirected to a different memory location. If that's the
>> case, we can prpbably rearrange the array, even if there's a small
>> window where ps might display a confusing command line, right?
>
> If that's the extent of the breakage, then it seems alright to me.

Okay by me to live with this burden.

> I've attached a new version of the patch that omits the
> POSIXLY_CORRECT stuff.

This looks OK at quick glance, though you may want to document at the
top of getopt_long() the reordering business with the non-options?
--
Michael

On Fri, Jun 16, 2023 at 10:30:09AM +0900, Michael Paquier wrote:
> On Thu, Jun 15, 2023 at 05:09:59PM -0700, Nathan Bossart wrote:
>> I've attached a new version of the patch that omits the
>> POSIXLY_CORRECT stuff.
> 
> This looks OK at quick glance, though you may want to document at the
> top of getopt_long() the reordering business with the non-options?

I added a comment to this effect in v3.  I also noticed that '-' wasn't
properly handled as a non-option, so I've tried to fix that as well.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

At Thu, 15 Jun 2023 21:58:28 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> On Fri, Jun 16, 2023 at 10:30:09AM +0900, Michael Paquier wrote:
> > On Thu, Jun 15, 2023 at 05:09:59PM -0700, Nathan Bossart wrote:
> >> I've attached a new version of the patch that omits the
> >> POSIXLY_CORRECT stuff.
> > 
> > This looks OK at quick glance, though you may want to document at the
> > top of getopt_long() the reordering business with the non-options?
> 
> I added a comment to this effect in v3.  I also noticed that '-' wasn't
> properly handled as a non-option, so I've tried to fix that as well.

(Honestly, the rearrangement code looks somewhat tricky to grasp..)

It doesn't work properly if '--' is involved.

For example, consider the following options (even though they don't
work for the command).

psql -t -T hoho -a hoge -- -1 hage hige huge

After the function returns -1, the argv array looks like this. The
"[..]" indicates the position of optind.

psql -t -T hoho -a -- [-1] hage hige huge hoge

This is clearly incorrect since "hoge" gets moved to the end.  The
rearrangement logic needs to take into account the '--'.  For your
information, the glibc version yields the following result for the
same options.

psql -t -T hoho -a -- [hoge] -1 hage hige huge

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Fri, Jun 16, 2023 at 04:51:38PM +0900, Kyotaro Horiguchi wrote:
> (Honestly, the rearrangement code looks somewhat tricky to grasp..)

Yeah, I think there's still some room for improvement here.

> It doesn't work properly if '--' is involved.
> 
> For example, consider the following options (even though they don't
> work for the command).
> 
> psql -t -T hoho -a hoge -- -1 hage hige huge
> 
> After the function returns -1, the argv array looks like this. The
> "[..]" indicates the position of optind.
> 
> psql -t -T hoho -a -- [-1] hage hige huge hoge
> 
> This is clearly incorrect since "hoge" gets moved to the end.  The
> rearrangement logic needs to take into account the '--'.  For your
> information, the glibc version yields the following result for the
> same options.
> 
> psql -t -T hoho -a -- [hoge] -1 hage hige huge

Ah, so it effectively retains the non-option ordering, even if there is a
'--'.  I think this behavior is worth keeping.  I attempted to fix this in
the attached patch.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

At Fri, 16 Jun 2023 11:28:47 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> On Fri, Jun 16, 2023 at 04:51:38PM +0900, Kyotaro Horiguchi wrote:
> > (Honestly, the rearrangement code looks somewhat tricky to grasp..)
> 
> Yeah, I think there's still some room for improvement here.

The argv elements get shuffled around many times with the
patch. However, I couldn't find a way to decrease the count without
resorting to a forward scan.  So I've concluded the current approach
is them most effeicient, considering the complexity.

> Ah, so it effectively retains the non-option ordering, even if there is a
> '--'.  I think this behavior is worth keeping.  I attempted to fix this in
> the attached patch.

I tried some patterns with the patch and it generates the same results
with the glibc version.

The TAP test looks fine and it catches the change.

Everything looks fine to me.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jun 20, 2023 at 02:12:44PM +0900, Kyotaro Horiguchi wrote:
> The argv elements get shuffled around many times with the
> patch. However, I couldn't find a way to decrease the count without
> resorting to a forward scan.  So I've concluded the current approach
> is them most effeicient, considering the complexity.

Yeah, I'm not sure it's worth doing anything more sophisticated.

> I tried some patterns with the patch and it generates the same results
> with the glibc version.
> 
> The TAP test looks fine and it catches the change.
> 
> Everything looks fine to me.

Thanks for reviewing.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

I spent some time tidying up the patch and adding a more detailed commit
message.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

At Fri, 7 Jul 2023 20:52:24 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> I spent some time tidying up the patch and adding a more detailed commit
> message.

The commit message and the change to TAP script looks good.


Two conditions are to be reversed and one of them look somewhat
unintuitive to me.

+            if (!force_nonopt && place[0] == '-' && place[1])
+            {
+                if (place[1] != '-' || place[2])
+                    break;
+
+                optind++;
+                force_nonopt = true;
+                continue;
+            }

The first if looks good to me, but the second if is a bit hard to get the meaning at a glance. "!(place[1] == '-' &&
place[2]== 0)" is easier to read *to me*. Or I'm fine with the following structure here.
 

> if (!force_nonopt ... )
> {
>   if (place[1] == '-' && place[2] == 0)
>   {
>      optind+;
>      force_nonopt = true;
>      continue;
>   }
>   break;
> }

(To be honest, I see the goto looked clear than for(;;)..)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Mon, Jul 10, 2023 at 04:57:11PM +0900, Kyotaro Horiguchi wrote:
> +            if (!force_nonopt && place[0] == '-' && place[1])
> +            {
> +                if (place[1] != '-' || place[2])
> +                    break;
> +
> +                optind++;
> +                force_nonopt = true;
> +                continue;
> +            }
> 
> The first if looks good to me, but the second if is a bit hard to get the meaning at a glance. "!(place[1] == '-' &&
place[2]== 0)" is easier to read *to me*. Or I'm fine with the following structure here.
 

I'd readily admit that it's tough to read these lines.  I briefly tried to
make use of strcmp() to improve readability, but I wasn't having much luck.
I'll give it another try.

>> if (!force_nonopt ... )
>> {
>>   if (place[1] == '-' && place[2] == 0)
>>   {
>>      optind+;
>>      force_nonopt = true;
>>      continue;
>>   }
>>   break;
>> }
> 
> (To be honest, I see the goto looked clear than for(;;)..)

Okay.  I don't mind adding it back if it improves readability.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Here's a new version of the patch with the latest feedback addressed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

At Mon, 10 Jul 2023 13:06:58 -0700, Nathan Bossart <nathandbossart@gmail.com> wrote in 
> Here's a new version of the patch with the latest feedback addressed.

Thanks!

+         * An argument is a non-option if it meets any of the following
+         * criteria: it follows an argument that is equivalent to the string
+         * "--", it is equivalent to the string "-", or it does not start with
+         * '-'.  When we encounter a non-option, we move it to the end of argv
+         * (after shifting all remaining arguments over to make room), and
+         * then we try again with the next argument.
+         */
+        if (force_nonopt || strcmp("-", place) == 0 || place[0] != '-')
...
+        else if (strcmp("--", place) == 0)

I like it. We don't need to overcomplicate things just for the sake of
speed here. Plus, this version looks the most simple to me. That being
said, it might be better if the last term is positioned in the second
place. This is mainly because a lone hyphen is less common than words
that starts with characters other than a pyphen.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Tue, Jul 11, 2023 at 04:16:09PM +0900, Kyotaro Horiguchi wrote:
> I like it. We don't need to overcomplicate things just for the sake of
> speed here. Plus, this version looks the most simple to me. That being
> said, it might be better if the last term is positioned in the second
> place. This is mainly because a lone hyphen is less common than words
> that starts with characters other than a pyphen.

Sure.  І did it this way in v7.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jul 11, 2023 at 09:32:32AM -0700, Nathan Bossart wrote:
> Sure.  І did it this way in v7.

After a couple more small edits, I've committed this.  I looked through all
uses of getopt_long() in PostgreSQL earlier today, and of the programs that
accepted non-options, most accepted only one, some others accepted 2-3, and
ecpg and pg_regress accepted any number.  Given this analysis, I'm not too
worried about the O(n^2) behavior that the patch introduces.  You'd likely
need to provide an enormous number of non-options for it to be noticeable,
and I'm dubious such use-cases exist.

During my analysis, I noticed that pg_ctl contains a workaround for the
lack of argument reordering.  I think this can now be removed.  Patch
attached.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Jul 12, 2023 at 08:49:03PM -0700, Nathan Bossart wrote:
> After a couple more small edits, I've committed this.  I looked through all
> uses of getopt_long() in PostgreSQL earlier today, and of the programs that
> accepted non-options, most accepted only one, some others accepted 2-3, and
> ecpg and pg_regress accepted any number.  Given this analysis, I'm not too
> worried about the O(n^2) behavior that the patch introduces.  You'd likely
> need to provide an enormous number of non-options for it to be noticeable,
> and I'm dubious such use-cases exist.
>
> During my analysis, I noticed that pg_ctl contains a workaround for the
> lack of argument reordering.  I think this can now be removed.  Patch
> attached.

Interesting piece of history that you have found here, coming from
f3d6d94 back in 2004.  The old pg_ctl.sh before that did not need any
of that.  It looks sensible to do something about that.

Something does not seem to be right seen from here, a CI run with
Windows 2019 fails when using pg_ctl at the beginning of most of the
tests, like:
[04:56:07.404] ------------------------------------- 8< -------------------------------------
[04:56:07.404] stderr:
[04:56:07.404] pg_ctl: too many command-line arguments (first is "-D")
--
Michael

At Thu, 13 Jul 2023 14:39:32 +0900, Michael Paquier <michael@paquier.xyz> wrote in 
> [04:56:07.404] pg_ctl: too many command-line arguments (first is "-D")

Mmm. It checks, for example, for "pg_ctl initdb -D $tempdir/data -o
-N".  This version of getopt_long() returns -1 as soon as it meets the
first non-option "initdb". This is somewhat different from the last
time what I saw the patch and looks strange at a glance..

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

On Thu, Jul 13, 2023 at 02:39:32PM +0900, Michael Paquier wrote:
> Something does not seem to be right seen from here, a CI run with
> Windows 2019 fails when using pg_ctl at the beginning of most of the
> tests, like:
> [04:56:07.404] ------------------------------------- 8< -------------------------------------
> [04:56:07.404] stderr:
> [04:56:07.404] pg_ctl: too many command-line arguments (first is "-D")

Assuming you are referring to [0], it looks like you are missing 411b720.

[0] https://github.com/michaelpq/postgres/commits/getopt_test

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jul 13, 2023 at 07:57:12AM -0700, Nathan Bossart wrote:
> Assuming you are referring to [0], it looks like you are missing 411b720.
>
> [0] https://github.com/michaelpq/postgres/commits/getopt_test

Indeed, it looks like I've fat-fingered a rebase here.  I am able to
get a clean CI run when running this patch, sorry for the noise.

Anyway, this introduces a surprising behavior when specifying too many
subcommands.  On HEAD:
$ pg_ctl stop -D $PGDATA kill -t 20 start
pg_ctl: too many command-line arguments (first is "stop")
Try "pg_ctl --help" for more information.
$ pg_ctl stop -D $PGDATA -t 20 start
pg_ctl: too many command-line arguments (first is "stop")
Try "pg_ctl --help" for more information.

With the patch:
$ pg_ctl stop -D $PGDATA -t 20 start
pg_ctl: too many command-line arguments (first is "start")
Try "pg_ctl --help" for more information.
$ pg_ctl stop -D $PGDATA kill -t 20 start
pg_ctl: too many command-line arguments (first is "kill")
Try "pg_ctl --help" for more information.

So the error message reported is incorrect now, referring to an
incorrect first subcommand.
--
Michael

On Fri, Jul 14, 2023 at 01:27:26PM +0900, Michael Paquier wrote:
> Indeed, it looks like I've fat-fingered a rebase here.  I am able to
> get a clean CI run when running this patch, sorry for the noise.
> 
> Anyway, this introduces a surprising behavior when specifying too many
> subcommands.  On HEAD:
> $ pg_ctl stop -D $PGDATA kill -t 20 start
> pg_ctl: too many command-line arguments (first is "stop")
> Try "pg_ctl --help" for more information.
> $ pg_ctl stop -D $PGDATA -t 20 start
> pg_ctl: too many command-line arguments (first is "stop")
> Try "pg_ctl --help" for more information.
> 
> With the patch:
> $ pg_ctl stop -D $PGDATA -t 20 start
> pg_ctl: too many command-line arguments (first is "start")
> Try "pg_ctl --help" for more information.
> $ pg_ctl stop -D $PGDATA kill -t 20 start
> pg_ctl: too many command-line arguments (first is "kill")
> Try "pg_ctl --help" for more information.
> 
> So the error message reported is incorrect now, referring to an
> incorrect first subcommand.

I did notice this, but I had the opposite reaction.  Take the following
examples of client programs that accept one non-option:

    ~$ pg_resetwal a b c
    pg_resetwal: error: too many command-line arguments (first is "b")
    pg_resetwal: hint: Try "pg_resetwal --help" for more information.

    ~$ createuser a b c
    createuser: error: too many command-line arguments (first is "b")
    createuser: hint: Try "createuser --help" for more information.

    ~$ pgbench a b c
    pgbench: error: too many command-line arguments (first is "b")
    pgbench: hint: Try "pgbench --help" for more information.

    ~$ pg_restore a b c
    pg_restore: error: too many command-line arguments (first is "b")
    pg_restore: hint: Try "pg_restore --help" for more information.

Yet pg_ctl gives:

    ~$ pg_ctl start a b c
    pg_ctl: too many command-line arguments (first is "start")
    Try "pg_ctl --help" for more information.

In this example, isn't "a" the first extra non-option that should be
reported?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jul 13, 2023 at 09:38:42PM -0700, Nathan Bossart wrote:
> I did notice this, but I had the opposite reaction.

Ahah, well ;)

> Take the following examples of client programs that accept one non-option:
>
>     ~$ pg_resetwal a b c
>     pg_resetwal: error: too many command-line arguments (first is "b")
>     pg_resetwal: hint: Try "pg_resetwal --help" for more information.
>
> Yet pg_ctl gives:
>
>     ~$ pg_ctl start a b c
>     pg_ctl: too many command-line arguments (first is "start")
>     Try "pg_ctl --help" for more information.
>
> In this example, isn't "a" the first extra non-option that should be
> reported?

Good point.  This is interpreting "first" as being the first option
that's invalid.  Here my first impression was that pg_ctl got that
right, where "first" refers to the first subcommand that would be
valid.  Objection withdrawn.
--
Michael

On Fri, Jul 14, 2023 at 02:02:28PM +0900, Michael Paquier wrote:
> Objection withdrawn.

Committed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Jul 13, 2023 at 09:38:42PM -0700, Nathan Bossart wrote:
>> Take the following examples of client programs that accept one non-option:
>>
>> ~$ pg_resetwal a b c
>> pg_resetwal: error: too many command-line arguments (first is "b")
>> pg_resetwal: hint: Try "pg_resetwal --help" for more information.
>>
>> Yet pg_ctl gives:
>>
>> ~$ pg_ctl start a b c
>> pg_ctl: too many command-line arguments (first is "start")
>> Try "pg_ctl --help" for more information.
>>
>> In this example, isn't "a" the first extra non-option that should be
>> reported?

> Good point.  This is interpreting "first" as being the first option
> that's invalid.  Here my first impression was that pg_ctl got that
> right, where "first" refers to the first subcommand that would be
> valid.  Objection withdrawn.

We just had a user complaint that seems to trace to exactly this
bogus reporting in pg_ctl [1].  Although I was originally not
very pleased with changing our getopt_long to do switch reordering,
I'm now wondering if we should back-patch these changes as bug
fixes.  It's probably not worth the risk, but ...

            regards, tom lane

[1]
https://www.postgresql.org/message-id/flat/CANzqJaDCagH5wNkPQ42%3DFx3mJPR-YnB3PWFdCAYAVdb9%3DQ%2Bt-A%40mail.gmail.com

On Mon, Dec 18, 2023 at 02:41:22PM -0500, Tom Lane wrote:
> We just had a user complaint that seems to trace to exactly this
> bogus reporting in pg_ctl [1].  Although I was originally not
> very pleased with changing our getopt_long to do switch reordering,
> I'm now wondering if we should back-patch these changes as bug
> fixes.  It's probably not worth the risk, but ...

I'm not too concerned about the risks of back-patching these commits, but
if this 19-year-old bug was really first reported today, I'd agree that
fixing it in the stable branches is probably not worth it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Mon, Dec 18, 2023 at 02:41:22PM -0500, Tom Lane wrote:
>> We just had a user complaint that seems to trace to exactly this
>> bogus reporting in pg_ctl [1].  Although I was originally not
>> very pleased with changing our getopt_long to do switch reordering,
>> I'm now wondering if we should back-patch these changes as bug
>> fixes.  It's probably not worth the risk, but ...

> I'm not too concerned about the risks of back-patching these commits, but
> if this 19-year-old bug was really first reported today, I'd agree that
> fixing it in the stable branches is probably not worth it.

Agreed, if it actually is 19 years old.  I'm wondering a little bit
if there could be some moderately-recent glibc behavior change
involved.  I'm not excited enough about it to go trawl their change
log, but we should keep our ears cocked for similar reports.

            regards, tom lane

On Mon, Dec 18, 2023 at 09:31:54PM -0500, Tom Lane wrote:
> Agreed, if it actually is 19 years old.  I'm wondering a little bit
> if there could be some moderately-recent glibc behavior change
> involved.  I'm not excited enough about it to go trawl their change
> log, but we should keep our ears cocked for similar reports.

From a brief glance, I believe this is long-standing behavior.  Even though
we advance optind at the bottom of the loop, the next getopt_long() call
seems to reset it to the first non-option (which was saved in a previous
call).

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com