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