Horton

Thread

Commits

  1. Fix Utils.pm's locale-munging so that Perl itself is also affected.

  2. Fix locale-dependent test case.

  3. psql: add an optional execution-count limit to \watch.

  1. pgsql: psql: add an optional execution-count limit to \watch.

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-06T17:18:30Z

    psql: add an optional execution-count limit to \watch.

\watch can now be told to stop after N executions of the query.

With the idea that we might want to add more options to \watch
in future, this patch generalizes the command's syntax to a list
of name=value options, with the interval allowed to omit the name
for backwards compatibility.

Andrey Borodin, reviewed by Kyotaro Horiguchi, Nathan Bossart,
Michael Paquier, Yugo Nagata, and myself

Discussion: https://postgr.es/m/CAAhFRxiZ2-n_L1ErMm9AZjgmUK=qS6VHb+0SaMn8sqqbhF7How@mail.gmail.com

Branch
------
master

Details
-------
https://git.postgresql.org/pg/commitdiff/00beecfe839c878abb366b68272426ed5296bc2b

Modified Files
--------------
doc/src/sgml/ref/psql-ref.sgml     |  10 +++-
src/bin/psql/command.c             | 118 +++++++++++++++++++++++++++++++------
src/bin/psql/help.c                |   2 +-
src/bin/psql/t/001_basic.pl        |  33 ++++++++---
src/test/regress/expected/psql.out |   2 +-
src/test/regress/sql/psql.sql      |   2 +-
6 files changed, 135 insertions(+), 32 deletions(-)

  2. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-07T12:04:00Z

    Hi!

On Thu, Apr 6, 2023 at 8:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> psql: add an optional execution-count limit to \watch.
>
> \watch can now be told to stop after N executions of the query.

This commit makes tests fail for me.  psql parses 'i' option of
'\watch' using locale-aware strtod(), but 001_basic.pl uses hard-coded
decimal separator.  The proposed fix is attached.

------
Regards,
Alexander Korotkov

  3. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-07T14:00:23Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> On Thu, Apr 6, 2023 at 8:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> psql: add an optional execution-count limit to \watch.

> This commit makes tests fail for me.  psql parses 'i' option of
> '\watch' using locale-aware strtod(), but 001_basic.pl uses hard-coded
> decimal separator.

Huh, yeah, I see it too if I set LANG=ru_RU.utf8 before running psql's
TAP tests.  It seems unfortunate that none of the buildfarm has noticed
this.  I guess all the TAP tests are run under C locale?

> The proposed fix is attached.

LGTM, will push in a bit (unless you want to?)

			regards, tom lane

  4. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-07T14:06:34Z

    On Fri, Apr 7, 2023 at 5:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > On Thu, Apr 6, 2023 at 8:18 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> psql: add an optional execution-count limit to \watch.
>
> > This commit makes tests fail for me.  psql parses 'i' option of
> > '\watch' using locale-aware strtod(), but 001_basic.pl uses hard-coded
> > decimal separator.
>
> Huh, yeah, I see it too if I set LANG=ru_RU.utf8 before running psql's
> TAP tests.  It seems unfortunate that none of the buildfarm has noticed
> this.  I guess all the TAP tests are run under C locale?

I wonder if we can setup as least some buildfarm members to exercise
TAP tests on non-C locales.

> > The proposed fix is attached.
>
> LGTM, will push in a bit (unless you want to?)

Please push.

------
Regards,
Alexander Korotkov

  5. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Aleksander Alekseev <aleksander@timescale.com> — 2023-04-10T12:48:57Z

    Hi,

> I wonder if we can setup as least some buildfarm members to exercise
> TAP tests on non-C locales.
>
> > > The proposed fix is attached.
> >
> > LGTM, will push in a bit (unless you want to?)
>
> Please push.

The test still fails under the following conditions:

```
$ env | grep UTF-8
LC_ADDRESS=ru_RU.UTF-8
LC_NAME=ru_RU.UTF-8
LC_MONETARY=ru_RU.UTF-8
LC_PAPER=ru_RU.UTF-8
LANG=en_US.UTF-8
LC_IDENTIFICATION=ru_RU.UTF-8
LC_TELEPHONE=ru_RU.UTF-8
LC_MEASUREMENT=ru_RU.UTF-8
LC_CTYPE=en_US.UTF-8
LC_TIME=ru_RU.UTF-8
LC_ALL=en_US.UTF-8
LC_NUMERIC=ru_RU.UTF-8
```

This is up-to-dated Ubuntu 22.04 with pretty much default settings
except for the timezone changed to MSK and enabled Russian keyboard
layout.

Here is a proposed fix. I realize this is a somewhat suboptimal
solution, but it makes the test pass regardless of the locale
settings.

-- 
Best regards,
Aleksander Alekseev

  6. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-10T13:54:42Z

    Aleksander Alekseev <aleksander@timescale.com> writes:
> The test still fails under the following conditions:

> $ env | grep UTF-8
> LANG=en_US.UTF-8
> LC_ALL=en_US.UTF-8
> LC_NUMERIC=ru_RU.UTF-8

Hmm, so psql is honoring the LC_NUMERIC setting in that environment,
but perl isn't.  For me, it appears that adding 'use locale;' to
the test script will fix it ... can you confirm if it's OK for you?

			regards, tom lane

  7. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Aleksander Alekseev <aleksander@timescale.com> — 2023-04-10T14:30:00Z

    Hi Tom,

> Aleksander Alekseev <aleksander@timescale.com> writes:
> > The test still fails under the following conditions:
>
> > $ env | grep UTF-8
> > LANG=en_US.UTF-8
> > LC_ALL=en_US.UTF-8
> > LC_NUMERIC=ru_RU.UTF-8
>
> Hmm, so psql is honoring the LC_NUMERIC setting in that environment,
> but perl isn't.  For me, it appears that adding 'use locale;' to
> the test script will fix it ... can you confirm if it's OK for you?

Right, src/bin/psql/t/001_basic.pl has "use locale;" since cd82e5c7
and it fails nevertheless.

If I set LC_NUMERIC manually:

```
LC_NUMERIC=en_US.UTF-8 meson test -C build --suite postgresql:psql
```

... the test passes. I can confirm that Perl doesn't seem to be
honoring LC_NUMERIC:

```
$ LC_ALL=en_US.UTF-8 LC_NUMERIC=en_US.UTF-8 perl -e 'use locale;
printf("%g\n", 0.01)'
0.01
$ LC_ALL=en_US.UTF-8 LC_NUMERIC=ru_RU.UTF-8 perl -e 'use locale;
printf("%g\n", 0.01)'
0.01
$ LC_ALL=ru_RU.UTF-8 LC_NUMERIC=en_US.UTF-8 perl -e 'use locale;
printf("%g\n", 0.01)'
0,01
$ LC_ALL=ru_RU.UTF-8 LC_NUMERIC=ru_RU.UTF-8 perl -e 'use locale;
printf("%g\n", 0.01)'
0,01
```

The Perl version is 5.34.0.

It is consistent with `perdoc perllocale`:

```
    The initial program is started up using the locale specified from the
    environment, as currently, described in "ENVIRONMENT". [...]

ENVIRONMENT
[...]
    "LC_ALL"    "LC_ALL" is the "override-all" locale environment variable.
                If set, it overrides all the rest of the locale environment
                variables.
```

So it looks like what happens is LC_ALL overwrites LC_NUMERIC for perl
but not for psql.

-- 
Best regards,
Aleksander Alekseev

  8. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-10T15:09:41Z

    Aleksander Alekseev <aleksander@timescale.com> writes:
>> Hmm, so psql is honoring the LC_NUMERIC setting in that environment,
>> but perl isn't.  For me, it appears that adding 'use locale;' to
>> the test script will fix it ... can you confirm if it's OK for you?

> Right, src/bin/psql/t/001_basic.pl has "use locale;" since cd82e5c7
> and it fails nevertheless.
> ...
> So it looks like what happens is LC_ALL overwrites LC_NUMERIC for perl
> but not for psql.

Oh, right, there already is one :-(.  After some more research,
I believe I see the problem: Utils.pm does

BEGIN
{
	# Set to untranslated messages, to be able to compare program output
	# with expected strings.
	delete $ENV{LANGUAGE};
	delete $ENV{LC_ALL};
	$ENV{LC_MESSAGES} = 'C';

Normally, with your settings, LC_ALL=en_US.UTF-8 would dominate
everything.  After removing that from the environment, the child
psql process will honor LC_NUMERIC=ru_RU.UTF-8 and expect \watch's
argument to be "0,01".  However, I bet that perl has already made
its decisions about what its internal locale is, so it still thinks
it should print "0.01".

I am betting that we need to make Utils.pm do

	setlocale(LC_ALL, "");

after the above-quoted bit, else it isn't doing what it is supposed to
if the calling script has already done "use locale;", as indeed
psql/t/001_basic.pl (and a small number of other places) do.

The attached makes check-world pass for me under these conflicting
environment settings, but it's kind of a scary change.  Thoughts?

			regards, tom lane

  9. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Aleksander Alekseev <aleksander@timescale.com> — 2023-04-10T16:44:54Z

    Hi,

> The attached makes check-world pass for me under these conflicting
> environment settings, but it's kind of a scary change.  Thoughts?

FWIW my MacOS and Linux laptops have no complaints about the patch.

-- 
Best regards,
Aleksander Alekseev

  10. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-18T17:34:12Z

    Aleksander Alekseev <aleksander@timescale.com> writes:
>> The attached makes check-world pass for me under these conflicting
>> environment settings, but it's kind of a scary change.  Thoughts?

> FWIW my MacOS and Linux laptops have no complaints about the patch.

I realized that we don't actually need to "use locale" in Utils.pm
itself for this to work, which greatly assuages my fears of unexpected
side-effects.  Pushed that way; I shall now retire to a safe distance
and watch the buildfarm.

			regards, tom lane

  11. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Anton Voloshin <a.voloshin@postgrespro.ru> — 2024-04-25T18:22:49Z

    Hello, hackers.

On 18/04/2023 20:34, Tom Lane wrote (on pgsql-committers):
 > I shall now retire to a safe distance and watch the buildfarm.

Unfortunately, on fresh perl (5.38.2 verified) and on ru_RU.UTF-8 
locale, it breaks basic float comparison: 0 < 0.5 is no longer true.

This is the reproduction on REL_16_STABLE (but it affects master
as well), using fresh Ubuntu 24.04 container.

0. I've used lxc to get a fresh container:
$ lxc launch ubuntu-daily:noble u2404
But I don't think lxc or containerization in general matters in this 
case. Also, I think any environment with fresh enough Perl would work, 
Ubuntu 24.04 is just an easy example.

(obviously, install necessary dev packages)

1. Generate ru_RU.UTF-8 locale:
a. In /etc/locale.gen, uncomment the line:
# ru_RU.UTF-8 UTF-8

b. Run locale-gen as root. For me, it says:
$ sudo locale-gen
Generating locales (this might take a while)...
   en_US.UTF-8... done
   ru_RU.UTF-8... done
Generation complete.

2. Apply 0001-demo-of-weird-Perl-setlocale-effect-on-float-numbers.patch
(adding src/test/authentication/t/999_broken.pl)

3. Run the test
LANG=ru_RU.UTF-8 make check -C src/test/authentication 
PROVE_TESTS=t/999_broken.pl PROVE_FLAGS=--verbose

The test is, basically:
use PostgreSQL::Test::Utils;
use Test::More tests => 1;
ok(0 < 0.5, "0 < 0.5");

If I comment-out the "use PostgreSQL::Test::Utils" line, the test works. 
Otherwise it fails to notice that 0 is less than 0.5.

Alternatively, the test fails if I replace that "use" line with
BEGIN {
	use POSIX qw(locale_h);
	setlocale(LC_NUMERIC, "");
}

"BEGIN" part is essential: mere use/setlocale is fine.

Also, adding
use locale;
or even
use locale ':numeric';
fixes the test, but I doubt whether it's a good idea to add that to 
Utils.pm.

Obviously, one of the reasons is that according to ru_RU.UTF-8 locale 
for LC_NUMERIC, fractional part separator is ",", not ".". So one could, 
technically, parse "0.5" as "0" and then unparsed ".5" tail. I think it 
might even be a Perl bug, because, according to my quick browsing of man 
perlfunc (setlocale) and man perllocale, this should not affect the code 
outside "use locale", not in such a fundamental way. After all, we're 
talking not about strtod etc, but about floating-point numbers in the 
source code.

P.S. $ perl --version

This is perl 5, version 38, subversion 2 (v5.38.2) built for 
x86_64-linux-gnu-thread-multi
(with 44 registered patches, see perl -V for more detail)

P.P.S. I'm replying to pgsql-hackers, even though part of previous 
discussion have been on pgsql-committers. Hopefully, it's OK.

-- 
Anton Voloshin
Postgres Professional, The Russian Postgres Company
https://postgrespro.ru

  12. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-26T02:20:47Z

    Anton Voloshin <a.voloshin@postgrespro.ru> writes:
> On 18/04/2023 20:34, Tom Lane wrote (on pgsql-committers):
>>> I shall now retire to a safe distance and watch the buildfarm.

> Unfortunately, on fresh perl (5.38.2 verified) and on ru_RU.UTF-8 
> locale, it breaks basic float comparison: 0 < 0.5 is no longer true.

Haven't we worked around that everywhere it matters, in commits such
as 8421f6bce and 605062227?  For me, check-world passes under
LANG=ru_RU, even with perl 5.38.2 (where I do confirm that your
test script fails).  The buildfarm isn't unhappy either.

> Obviously, one of the reasons is that according to ru_RU.UTF-8 locale 
> for LC_NUMERIC, fractional part separator is ",", not ".". So one could, 
> technically, parse "0.5" as "0" and then unparsed ".5" tail. I think it 
> might even be a Perl bug, because, according to my quick browsing of man 
> perlfunc (setlocale) and man perllocale, this should not affect the code 
> outside "use locale", not in such a fundamental way. After all, we're 
> talking not about strtod etc, but about floating-point numbers in the 
> source code.

I agree that it's a Perl bug, mainly because your test case doesn't
fail in Perls as recent as v5.32.1 (released about 3 years ago).
It's impossible to believe that they intentionally broke basic
Perl constant syntax now, after so many years.  Particularly in
this way --- what are we supposed to do, write "if (0 < 0,5)"?
That means something else.

			regards, tom lane

  13. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Anton Voloshin <a.voloshin@postgrespro.ru> — 2024-04-26T14:38:53Z

    On 26/04/2024 05:20, Tom Lane wrote:
> Haven't we worked around that everywhere it matters, in commits such
> as 8421f6bce and 605062227?

Yes, needing 8421f6bce and 605062227 was, perhaps, surprising, but 
reasonable. Unlike breaking floating point constants in the source code. 
But, I guess, you're right and, since it does look like a Perl bug, 
we'll have to work around that in all places where we use floating-point 
constants in Perl code, which are surprisingly few.

 > For me, check-world passes under
 > LANG=ru_RU, even with perl 5.38.2 (where I do confirm that your
 > test script fails).  The buildfarm isn't unhappy either.

Indeed, check-world seems to run fine on my machine and on the bf as well.

Grepping and browsing through, I've only found three spots with \d\.\d 
directly in Perl code as a float, only one of them needs correction.

1. src/test/perl/PostgreSQL/Test/Kerberos.pm in master
src/test/kerberos/t/001_auth.pl in REL_16_STABLE
 >     if ($krb5_version >= 1.15)

I guess adding use locale ':numeric' would be easiest workaround here.
Alternatively, we could also split version into krb5_major_version and 
krb5_minor_version while parsing krb5-config --version's output above, 
but I don't think that's warranted. So I suggest something along the 
lines of 0001-use-numeric-locale-in-kerberos-test-rel16.patch and 
*-master.patch (attached, REL_16 and master need this change in 
different places).

I did verify by providing fake 'krb5-config' that before the fix, with 
LANG=ru_RU.UTF-8 and Perl 5.38.2 and with, say, krb5 "version" 1.13 it 
would still add the "listen" lines to kdc.conf by mistake (presumably, 
confusing some versions of kerberos).

2 and 3. contrib/intarray/bench/create_test.pl
 >     if (rand() < 0.7)
and
 >     if ($#sect < 0 || rand() < 0.1)

PostgreSQL::Test::Utils is not used there, so it's OK, no change needed.

I did not find any other float constants in .pl/.pm files in master (I 
could have missed something).

 > Particularly in
 > this way --- what are we supposed to do, write "if (0 < 0,5)"?
 > That means something else.

Yep. I will try to report this to Perl community later.

-- 
Anton Voloshin
Postgres Professional, The Russian Postgres Company
https://postgrespro.ru

  14. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Anton Voloshin <a.voloshin@postgrespro.ru> — 2024-04-26T17:04:23Z

    On 26/04/2024 17:38, Anton Voloshin wrote:
> I will try to report this to Perl community later.

Reported under https://github.com/Perl/perl5/issues/22176

Perl 5.36.3 seems to be fine (latest stable release before 5.38.x).
5.38.0 and 5.38.2 are broken.

-- 
Anton Voloshin
Postgres Professional, The Russian Postgres Company
https://postgrespro.ru

  15. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-04-26T17:20:26Z

    Anton Voloshin <a.voloshin@postgrespro.ru> writes:
> On 26/04/2024 17:38, Anton Voloshin wrote:
>> I will try to report this to Perl community later.

> Reported under https://github.com/Perl/perl5/issues/22176

Thanks for doing that.

> Perl 5.36.3 seems to be fine (latest stable release before 5.38.x).
> 5.38.0 and 5.38.2 are broken.

If the misbehavior is that new, I'm inclined to do nothing about it,
figuring that they'll fix it sooner not later.  If we were seeing
failures in main-line check-world tests then maybe it'd be worth
band-aiding those, but AFAICS we're not.

			regards, tom lane

  16. Re: pgsql: psql: add an optional execution-count limit to \watch.

    Andrew Dunstan <andrew@dunslane.net> — 2024-04-27T12:15:36Z

    On 2023-04-07 Fr 10:00, Tom Lane wrote:
> Alexander Korotkov<aekorotkov@gmail.com>  writes:
>> On Thu, Apr 6, 2023 at 8:18 PM Tom Lane<tgl@sss.pgh.pa.us>  wrote:
>>> psql: add an optional execution-count limit to \watch.
>> This commit makes tests fail for me.  psql parses 'i' option of
>> '\watch' using locale-aware strtod(), but 001_basic.pl uses hard-coded
>> decimal separator.
> Huh, yeah, I see it too if I set LANG=ru_RU.utf8 before running psql's
> TAP tests.  It seems unfortunate that none of the buildfarm has noticed
> this.  I guess all the TAP tests are run under C locale?


[just noticed this, redirecting to -hackers]


When run under meson, yes unless the LANG/LC_* settings are explicitly 
in the build_env. I'm fixing that so we will allow them to pass through. 
When run with configure/make they run with whatever is in the calling 
environment unless overridden in the build_env.

We do have support for running installchecks with multiple locales.This 
is done by passing --locale=foo to initdb.

We could locale-enable the non-install checks (for meson builds, that's 
the 'misc-check' step, for configure/make builds it's more or less 
everything between the install stages and the (first) initdb step. We'd 
have to do that via appropriate environment settings, I guess. Would it 
be enough to set LANG, or do we need to set the LC_foo settings 
individually? Not sure how we manage it on Windows. Maybe just not 
enable it for the first go-round.


cheers


andrew


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