Horton

Thread

Commits

  1. Inhibit mingw CRT's auto-globbing of command line arguments

  2. Remove some recently-added pg_dump test cases.

  3. Allow db.schema.table patterns, but complain about random garbage.

  4. In PostgresNode.pm, don't pass SQL to psql on the command line

  5. Factor pattern-construction logic out of processSQLNamePattern.

  1. pgsql: Allow db.schema.table patterns, but complain about random garbag

    Robert Haas <rhaas@postgresql.org> — 2022-04-20T15:52:12Z

    Allow db.schema.table patterns, but complain about random garbage.

psql, pg_dump, and pg_amcheck share code to process object name
patterns like 'foo*.bar*' to match all tables with names starting in
'bar' that are in schemas starting with 'foo'. Before v14, any number
of extra name parts were silently ignored, so a command line '\d
foo.bar.baz.bletch.quux' was interpreted as '\d bletch.quux'.  In v14,
as a result of commit 2c8726c4b0a496608919d1f78a5abc8c9b6e0868, we
instead treated this as a request for table quux in a schema named
'foo.bar.baz.bletch'. That caused problems for people like Justin
Pryzby who were accustomed to copying strings of the form
db.schema.table from messages generated by PostgreSQL itself and using
them as arguments to \d.

Accordingly, revise things so that if an object name pattern contains
more parts than we're expecting, we throw an error, unless there's
exactly one extra part and it matches the current database name.
That way, thisdb.myschema.mytable is accepted as meaning just
myschema.mytable, but otherdb.myschema.mytable is an error, and so
is some.random.garbage.myschema.mytable.

Mark Dilger, per report from Justin Pryzby and discussion among
various people.

Discussion: https://www.postgresql.org/message-id/20211013165426.GD27491%40telsasoft.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/d2d35479796c3510e249d6fc72adbd5df918efbf

Modified Files
--------------
doc/src/sgml/ref/psql-ref.sgml       |  17 +-
src/bin/pg_amcheck/pg_amcheck.c      |  27 +-
src/bin/pg_amcheck/t/002_nonesuch.pl |  99 ++++-
src/bin/pg_dump/pg_dump.c            |  65 ++-
src/bin/pg_dump/pg_dumpall.c         |  13 +-
src/bin/pg_dump/t/002_pg_dump.pl     | 107 +++++
src/bin/psql/describe.c              | 504 ++++++++++++++--------
src/fe_utils/string_utils.c          | 129 ++++--
src/include/fe_utils/string_utils.h  |   6 +-
src/test/regress/expected/psql.out   | 804 +++++++++++++++++++++++++++++++++++
src/test/regress/sql/psql.sql        | 242 +++++++++++
11 files changed, 1796 insertions(+), 217 deletions(-)

  2. Re: pgsql: Allow db.schema.table patterns, but complain about random garbag

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-22T13:15:31Z

    On 2022-04-20 We 11:52, Robert Haas wrote:
> Allow db.schema.table patterns, but complain about random garbage.
>
> psql, pg_dump, and pg_amcheck share code to process object name
> patterns like 'foo*.bar*' to match all tables with names starting in
> 'bar' that are in schemas starting with 'foo'. Before v14, any number
> of extra name parts were silently ignored, so a command line '\d
> foo.bar.baz.bletch.quux' was interpreted as '\d bletch.quux'.  In v14,
> as a result of commit 2c8726c4b0a496608919d1f78a5abc8c9b6e0868, we
> instead treated this as a request for table quux in a schema named
> 'foo.bar.baz.bletch'. That caused problems for people like Justin
> Pryzby who were accustomed to copying strings of the form
> db.schema.table from messages generated by PostgreSQL itself and using
> them as arguments to \d.
>
> Accordingly, revise things so that if an object name pattern contains
> more parts than we're expecting, we throw an error, unless there's
> exactly one extra part and it matches the current database name.
> That way, thisdb.myschema.mytable is accepted as meaning just
> myschema.mytable, but otherdb.myschema.mytable is an error, and so
> is some.random.garbage.myschema.mytable.


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.


cheers


andrew


--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  3. Re: pgsql: Allow db.schema.table patterns, but complain about random garbag

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-04-22T14:04:05Z

    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.

			regards, tom lane

  4. Re: pgsql: Allow db.schema.table patterns, but complain about random garbag

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-22T14:24:27Z

    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.


cheers


andrew



--
Andrew Dunstan
EDB: https://www.enterprisedb.com

  5. Re: pgsql: Allow db.schema.table patterns, but complain about random garbag

    Robert Haas <robertmhaas@gmail.com> — 2022-04-22T20:06:03Z

    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

  6. Re: pgsql: Allow db.schema.table patterns, but complain about random garbag

    Thomas Munro <thomas.munro@gmail.com> — 2022-04-22T21:12:20Z

    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.)

  7. Re: pgsql: Allow db.schema.table patterns, but complain about random garbag

    Noah Misch <noah@leadboat.com> — 2022-04-23T02:59:27Z

    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

  8. Re: pgsql: Allow db.schema.table patterns, but complain about random garbag

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-24T17:09:08Z

    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

  9. Re: pgsql: Allow db.schema.table patterns, but complain about random garbag

    Noah Misch <noah@leadboat.com> — 2022-04-24T18:19:34Z

    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.

  10. Re: pgsql: Allow db.schema.table patterns, but complain about random garbag

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-24T19:37:06Z

    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

  11. Re: pgsql: Allow db.schema.table patterns, but complain about random garbag

    Andrew Dunstan <andrew@dunslane.net> — 2022-04-25T18:57:12Z

    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