Обсуждение: pgsql: createdb: Fix quoting of --encoding, --lc-ctype and --lc-collate

createdb: Fix quoting of --encoding, --lc-ctype and --lc-collate

The original coding failed to properly quote those arguments, leading to
failures when using quotes in the values used.  As the quoting can be
encoding-sensitive, the connection to the backend needs to be taken
before applying the correct quoting.

Author: Michael Paquier
Reviewed-by: Daniel Gustafsson
Discussion: https://postgr.es/m/20200214041004.GB1998@paquier.xyz
Backpatch-through: 9.5

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/008cf040962c98c7c55d54c28dcb43c3c1d83c92

Modified Files
--------------
src/bin/scripts/createdb.c        | 29 +++++++++++++++++++----------
src/bin/scripts/t/020_createdb.pl | 26 +++++++++++++++++++++++++-
2 files changed, 44 insertions(+), 11 deletions(-)

Buildfarm member curculio just failed with this complaint:

#   Failed test 'createdb with incorrect --lc-ctype stderr
/(?^s:^createdb: error: database creation failed: ERROR:  invalid
locale name)/'
#   at t/020_createdb.pl line 43.
#                   'createdb: error: database creation failed: ERROR:
 new LC_CTYPE (foo'; SELECT '1) is incompatible with the LC_CTYPE of
the template database (C)
# HINT:  Use the same LC_CTYPE as in the template database, or use
template0 as template.
# '
#     doesn't match '(?^s:^createdb: error: database creation failed:
ERROR:  invalid locale name)'
# Looks like you failed 1 test of 22.
[04:39:08] t/020_createdb.pl .........
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/22 subtests

...Robert

On Thu, Feb 27, 2020 at 09:43:14AM +0530, Robert Haas wrote:
> Buildfarm member curculio just failed with this complaint:

Thanks, I missed this report.

> #   Failed test 'createdb with incorrect --lc-ctype stderr
> /(?^s:^createdb: error: database creation failed: ERROR:  invalid
> locale name)/'
> #   at t/020_createdb.pl line 43.
> #                   'createdb: error: database creation failed: ERROR:
>  new LC_CTYPE (foo'; SELECT '1) is incompatible with the LC_CTYPE of
> the template database (C)
> # HINT:  Use the same LC_CTYPE as in the template database, or use
> template0 as template.

Now that's interesting.  The error I would have expected to find is
from createdb() so as we should actually bump on a failure for
setlocale(LC_CTYPE).  But this means that setlocale() with an
incorrect value actually succeeded, and that the fallback became "C".
Oh, actually, this behavior is mentioned here:
https://man.openbsd.org/setlocale.3

The best thing I can think of is just to remove the test case for
--lc-ctype.  Now looking at the code of OpenBSD there could be an
alternative: using a string longer than 256 characters if I read
lib/libc/locale/setlocale.c correctly, but I'd rather not rely on
that.

Thoughts?
--
Michael

On Thu, Feb 27, 2020 at 02:11:12PM +0900, Michael Paquier wrote:
> The best thing I can think of is just to remove the test case for
> --lc-ctype.

So, thinking about nothing better, I have just removed this test.
--
Michael

On 2020-Feb-27, Michael Paquier wrote:

> Now looking at the code of OpenBSD there could be an
> alternative: using a string longer than 256 characters if I read
> lib/libc/locale/setlocale.c correctly, but I'd rather not rely on
> that.

That seems awfully fragile.  Belated +1 for not relying on that :-)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Feb 27, 2020 at 02:11:12PM +0900, Michael Paquier wrote:
>> The best thing I can think of is just to remove the test case for
>> --lc-ctype.

> So, thinking about nothing better, I have just removed this test.

I just tried to run check-world on an OpenBSD 6.8 installation,
and look what I got:

t/020_createdb.pl ......... 14/19
#   Failed test 'createdb with incorrect --lc-collate stderr /(?^s:^createdb: error: database creation failed: ERROR:
invalidlocale name)/' 
#   at t/020_createdb.pl line 35.
#                   'createdb: error: database creation failed: ERROR:  new collation (foo'; SELECT '1) is incompatible
withthe collation of the template database (C) 
# HINT:  Use the same collation as in the template database, or use template0 as template.
# '
#     doesn't match '(?^s:^createdb: error: database creation failed: ERROR:  invalid locale name)'
# Looks like you failed 1 test of 19.
t/020_createdb.pl ......... Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/19 subtests

So on this machine, setlocale() is lax about LC_COLLATE values as
well as (presumably) LC_CTYPE.  Question then becomes why we did
not notice for nearly a year.  It looks like, of the not-very-many
OpenBSD machines in the buildfarm, the only one running the TAP
tests is curculio, and that's running OpenBSD 5.9 which is quite old.
Comparing their setlocale(3) man pages for different versions, it
looks like the current verbiage appeared in 6.3; older versions
have text similar to other BSDen.  So it looks like they actually
ripped out a lot of locale functionality in 6.3.

It seems like (1) we need to drop the --lc-collate version of this
test too, or else adjust both tests to accept the error we get on
OpenBSD as well as the normal error; (2) somebody really ought to
crank up a modern OpenBSD critter that's running the TAP tests.

            regards, tom lane

On Thu, Jan 07, 2021 at 05:25:24PM -0500, Tom Lane wrote:
> So on this machine, setlocale() is lax about LC_COLLATE values as
> well as (presumably) LC_CTYPE.  Question then becomes why we did
> not notice for nearly a year.  It looks like, of the not-very-many
> OpenBSD machines in the buildfarm, the only one running the TAP
> tests is curculio, and that's running OpenBSD 5.9 which is quite old.
> Comparing their setlocale(3) man pages for different versions, it
> looks like the current verbiage appeared in 6.3; older versions
> have text similar to other BSDen.  So it looks like they actually
> ripped out a lot of locale functionality in 6.3.

Indeed.  6.3 is the point where they began ignoring LC_COLLATE, and
the buildfarm has no coverage for it.  Here is the reference in the
upstream doc for others:
https://man.openbsd.org/OpenBSD-6.3/setlocale.3
https://man.openbsd.org/OpenBSD-6.2/setlocale.3

> It seems like (1) we need to drop the --lc-collate version of this
> test too, or else adjust both tests to accept the error we get on
> OpenBSD as well as the normal error; (2) somebody really ought to
> crank up a modern OpenBSD critter that's running the TAP tests.

Using an alternate output looks adapted to me.  Thanks for 9ffe227.
--
Michael