Обсуждение: pgsql: psql: add an optional execution-count limit to \watch.

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

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

diff --git a/src/test/perl/PostgreSQL/Test/Utils.pm b/src/test/perl/PostgreSQL/Test/Utils.pm
index 878e12b15e..4c6cde8974 100644
--- a/src/test/perl/PostgreSQL/Test/Utils.pm
+++ b/src/test/perl/PostgreSQL/Test/Utils.pm
@@ -43,6 +43,7 @@ package PostgreSQL::Test::Utils;

 use strict;
 use warnings;
+use locale;

 use Carp;
 use Config;
@@ -55,6 +56,7 @@ use File::Spec;
 use File::stat qw(stat);
 use File::Temp ();
 use IPC::Run;
+use POSIX qw(locale_h);
 use PostgreSQL::Test::SimpleTee;

 # We need a version of Test::More recent enough to support subtests
@@ -102,6 +104,7 @@ BEGIN
     delete $ENV{LANGUAGE};
     delete $ENV{LC_ALL};
     $ENV{LC_MESSAGES} = 'C';
+    setlocale(LC_ALL, "");

     # This list should be kept in sync with pg_regress.c.
     my @envkeys = qw (

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