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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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