Thread: 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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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
Attachment
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