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

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

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

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

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 /messages/by-id/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 /messages/by-id/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