Обсуждение: Re: pgsql: Allow db.schema.table patterns, but complain about random garbag
On Fri, Apr 22, 2022 at 10:24 AM Andrew Dunstan <andrew@dunslane.net> wrote: > On 2022-04-22 Fr 10:04, Tom Lane wrote: > > Andrew Dunstan <andrew@dunslane.net> writes: > >> This has upset the buildfarm's msys2 animals. There appears to be some > >> wildcard expansion going on that causes the problem. I don't know why it > >> should here when it's not causing trouble elsewhere. I have tried > >> changing the way the tests are quoted, without success. Likewise, > >> setting SHELLOPTS=noglob didn't work. > >> At this stage I'm fresh out of ideas to fix it. It's also quite possible > >> that my diagnosis is wrong. > > When I was looking at this patch, I thought the number of test cases > > was very substantially out of line anyway. I suggest that rather > > than investing a bunch of brain cells trying to work around this, > > we just remove the failing test cases. > > WFM. Sure, see also http://postgr.es/m/CA+TgmoYRGUcFBy6VgN0+Pn4f6Wv=2H0HZLuPHqSy6VC8Ba7vdg@mail.gmail.com where Andrew's opinion on how to fix this was sought. I have to say the fact that IPC::Run does shell-glob expansion of its arguments on some machines and not others seems ludicrous to me. This patch may be overtested, but such a radical behavior difference is completely nuts. How is anyone supposed to write reliable tests for any feature in the face of such wildly inconsistent behavior? -- Robert Haas EDB: http://www.enterprisedb.com
On Sat, Apr 23, 2022 at 8:06 AM Robert Haas <robertmhaas@gmail.com> wrote: > Sure, see also http://postgr.es/m/CA+TgmoYRGUcFBy6VgN0+Pn4f6Wv=2H0HZLuPHqSy6VC8Ba7vdg@mail.gmail.com > where Andrew's opinion on how to fix this was sought. > > I have to say the fact that IPC::Run does shell-glob expansion of its > arguments on some machines and not others seems ludicrous to me. This > patch may be overtested, but such a radical behavior difference is > completely nuts. How is anyone supposed to write reliable tests for > any feature in the face of such wildly inconsistent behavior? Yeah, I was speculating that it's a bug in IPC::Run that has been fixed (by our very own Noah), and some of the machines are still running the buggy version. (Not a Windows person, but I speculate the reason that such a stupid bug is even possible may be that Windows lacks a way to 'exec' stuff with a passed-in unadulterated argv[] array, so you always need to build a full shell command subject to interpolation, so if you're trying to emulate an argv[]-style interface you have to write the code to do the escaping, and so everyone gets a chance to screw that up.)
On Sat, Apr 23, 2022 at 09:12:20AM +1200, Thomas Munro wrote: > On Sat, Apr 23, 2022 at 8:06 AM Robert Haas <robertmhaas@gmail.com> wrote: > > I have to say the fact that IPC::Run does shell-glob expansion of its > > arguments on some machines and not others seems ludicrous to me. This > > patch may be overtested, but such a radical behavior difference is > > completely nuts. How is anyone supposed to write reliable tests for > > any feature in the face of such wildly inconsistent behavior? The MinGW gcc crt*.o files do shell-glob expansion on the arguments before entering main(). See https://google.com/search?q=mingw+command+line+glob for various discussion of that behavior. I suspect you experienced that, not any IPC::Run behavior. (I haven't tested, though.) Commit 11e9caf likely had the same cause, though the commit message attributed it to the msys shell rather than to crt*.o. Let's disable that MinGW compiler behavior. https://willus.com/mingw/_globbing.shtml lists two ways of achieving that. > Yeah, I was speculating that it's a bug in IPC::Run that has been > fixed (by our very own Noah), and some of the machines are still > running the buggy version. That change affected arguments containing double quote characters, but mere asterisks shouldn't need it. It also has not yet been released, so few or no buildfarm machines are using it. > (Not a Windows person, but I speculate the reason that such a stupid > bug is even possible may be that Windows lacks a way to 'exec' stuff > with a passed-in unadulterated argv[] array, so you always need to > build a full shell command subject to interpolation, so if you're > trying to emulate an argv[]-style interface you have to write the code > to do the escaping, and so everyone gets a chance to screw that up.) You needn't involve any shell. Other than that, your description pretty much says it. CreateProcessA() is the Windows counterpart of execve(). There's no argv[] array, just a single string. (The following trivia does not affect PostgreSQL.) Worse, while there's a typical way to convert that string to argv[] at program start, some programs do it differently. You need to know your callee in order to construct the string: https://github.com/toddr/IPC-Run/pull/148/commits/c299a86c9a292375fbfc39fb756883c80adac4b0#diff-5833a343d19ba684779743e2c90516fc65479609274731785364d2d2b49e2211
On 2022-04-22 Fr 22:59, Noah Misch wrote: > On Sat, Apr 23, 2022 at 09:12:20AM +1200, Thomas Munro wrote: >> On Sat, Apr 23, 2022 at 8:06 AM Robert Haas <robertmhaas@gmail.com> wrote: >>> I have to say the fact that IPC::Run does shell-glob expansion of its >>> arguments on some machines and not others seems ludicrous to me. This >>> patch may be overtested, but such a radical behavior difference is >>> completely nuts. How is anyone supposed to write reliable tests for >>> any feature in the face of such wildly inconsistent behavior? (I missed seeing the part where I was asked for help earlier on this thread) > The MinGW gcc crt*.o files do shell-glob expansion on the arguments before > entering main(). See https://google.com/search?q=mingw+command+line+glob for > various discussion of that behavior. I suspect you experienced that, not any > IPC::Run behavior. (I haven't tested, though.) Commit 11e9caf likely had the > same cause, though the commit message attributed it to the msys shell rather > than to crt*.o. > > Let's disable that MinGW compiler behavior. > https://willus.com/mingw/_globbing.shtml lists two ways of achieving that. Yeah. I can definitely confirm that this is the proximate cause of the issue, and not either IPC::Run or the shell, which is why all my experiments on this failed. With this patch diff --git a/src/include/port/win32.h b/src/include/port/win32.h index c6213c77c3..456c3f31f1 100644 --- a/src/include/port/win32.h +++ b/src/include/port/win32.h @@ -77,3 +77,7 @@ struct sockaddr_un char sun_path[108]; }; #define HAVE_STRUCT_SOCKADDR_UN 1 + +#ifndef _MSC_VER +extern int _CRT_glob = 0; /* 0 turns off globbing; 1 turns it on */ +#endif fairywren happily passes the tests that Robert has since reverted. I'm rather tempted to call this CRT behaviour a mis-feature, especially as a default. I think we should certainly disable it in the development branch, and consider back-patching it, although it is a slight change in behaviour, albeit one that we didn't know about much less want or document. Still, we been building with mingw compilers for about 20 years and haven't hit this before so far as we know, so maybe not. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On Sun, Apr 24, 2022 at 01:09:08PM -0400, Andrew Dunstan wrote: > On 2022-04-22 Fr 22:59, Noah Misch wrote: > > The MinGW gcc crt*.o files do shell-glob expansion on the arguments before > > entering main(). See https://google.com/search?q=mingw+command+line+glob for > > various discussion of that behavior. I suspect you experienced that, not any > > IPC::Run behavior. (I haven't tested, though.) Commit 11e9caf likely had the > > same cause, though the commit message attributed it to the msys shell rather > > than to crt*.o. > > > > Let's disable that MinGW compiler behavior. > > https://willus.com/mingw/_globbing.shtml lists two ways of achieving that. > > Yeah. I can definitely confirm that this is the proximate cause of the > issue, and not either IPC::Run or the shell, which is why all my > experiments on this failed. With this patch Thanks for confirming. > diff --git a/src/include/port/win32.h b/src/include/port/win32.h > index c6213c77c3..456c3f31f1 100644 > --- a/src/include/port/win32.h > +++ b/src/include/port/win32.h > @@ -77,3 +77,7 @@ struct sockaddr_un > char sun_path[108]; > }; > #define HAVE_STRUCT_SOCKADDR_UN 1 > + > +#ifndef _MSC_VER > +extern int _CRT_glob = 0; /* 0 turns off globbing; 1 turns it on */ > +#endif > > > fairywren happily passes the tests that Robert has since reverted. > > > I'm rather tempted to call this CRT behaviour a mis-feature, especially > as a default. I think we should certainly disable it in the development > branch, and consider back-patching it, although it is a slight change in > behaviour, albeit one that we didn't know about much less want or > document. Still, we been building with mingw compilers for about 20 > years and haven't hit this before so far as we know, so maybe not. I'd lean toward back-patching. We position MSVC and MinGW as two ways to build roughly the same PostgreSQL, not as two routes to different user-facing behavior. Since the postgresql.org/download binaries use MSVC, aligning with their behavior is good. It's fair to mention in the release notes, of course. Does your win32.h patch build without warnings or errors? Even if MinGW has some magic to make that work, I suspect we'll want a non-header home. Perhaps src/common/exec.c? It's best to keep this symbol out of libpq and other DLLs, though I bet exports.txt would avoid functional problems.
On 2022-04-24 Su 14:19, Noah Misch wrote: > On Sun, Apr 24, 2022 at 01:09:08PM -0400, Andrew Dunstan wrote: >> On 2022-04-22 Fr 22:59, Noah Misch wrote: >>> The MinGW gcc crt*.o files do shell-glob expansion on the arguments before >>> entering main(). See https://google.com/search?q=mingw+command+line+glob for >>> various discussion of that behavior. I suspect you experienced that, not any >>> IPC::Run behavior. (I haven't tested, though.) Commit 11e9caf likely had the >>> same cause, though the commit message attributed it to the msys shell rather >>> than to crt*.o. >>> >>> Let's disable that MinGW compiler behavior. >>> https://willus.com/mingw/_globbing.shtml lists two ways of achieving that. >> Yeah. I can definitely confirm that this is the proximate cause of the >> issue, and not either IPC::Run or the shell, which is why all my >> experiments on this failed. With this patch > Thanks for confirming. > >> diff --git a/src/include/port/win32.h b/src/include/port/win32.h >> index c6213c77c3..456c3f31f1 100644 >> --- a/src/include/port/win32.h >> +++ b/src/include/port/win32.h >> @@ -77,3 +77,7 @@ struct sockaddr_un >> char sun_path[108]; >> }; >> #define HAVE_STRUCT_SOCKADDR_UN 1 >> + >> +#ifndef _MSC_VER >> +extern int _CRT_glob = 0; /* 0 turns off globbing; 1 turns it on */ >> +#endif >> >> >> fairywren happily passes the tests that Robert has since reverted. >> >> >> I'm rather tempted to call this CRT behaviour a mis-feature, especially >> as a default. I think we should certainly disable it in the development >> branch, and consider back-patching it, although it is a slight change in >> behaviour, albeit one that we didn't know about much less want or >> document. Still, we been building with mingw compilers for about 20 >> years and haven't hit this before so far as we know, so maybe not. > I'd lean toward back-patching. We position MSVC and MinGW as two ways to > build roughly the same PostgreSQL, not as two routes to different user-facing > behavior. Since the postgresql.org/download binaries use MSVC, aligning with > their behavior is good. It's fair to mention in the release notes, of course. OK, good point. > Does your win32.h patch build without warnings or errors? Yes. > Even if MinGW has > some magic to make that work, I suspect we'll want a non-header home. Perhaps > src/common/exec.c? It's best to keep this symbol out of libpq and other > DLLs, though I bet exports.txt would avoid functional problems. exec.c looks like it should work fine. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
On 2022-04-24 Su 15:37, Andrew Dunstan wrote: > On 2022-04-24 Su 14:19, Noah Misch wrote: > >> Even if MinGW has >> some magic to make that work, I suspect we'll want a non-header home. Perhaps >> src/common/exec.c? It's best to keep this symbol out of libpq and other >> DLLs, though I bet exports.txt would avoid functional problems. > > exec.c looks like it should work fine. > > OK, in the absence of further comment I'm going to do it that way. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com