Обсуждение: broken master regress tests

On Fri, Aug 25, 2023 at 3:54 PM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> today build is broken on my Fedora 39

Has commit 252dcb32 somehow upset some kind of bleeding edge btrfs
filesystem?  That's a wild guess and I can't really imagine how but
apparently your database files are totally messed up...

On Sun, Aug 27, 2023 at 3:03 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> So it looks so IPC::Run::run is ignore parent environment

I guess the new initdb template captures lc_messages in
postgresql.conf, when it runs earlier?  I guess if you put
$node->append_conf('postgresql.conf', 'lc_messages=C'); into
src/bin/pg_amcheck/t/003_check.pl then it will work.  I'm not sure
what the correct fix should be, ie if the template mechanism should
notice this difference and not use the template, or if tests that
depend on the message locale should explicitly say so with
lc_messages=C or similar (why is this the only one?), or ...

On 2023-Aug-27, Thomas Munro wrote:

> On Sun, Aug 27, 2023 at 3:03 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> > So it looks so IPC::Run::run is ignore parent environment
> 
> I guess the new initdb template captures lc_messages in
> postgresql.conf, when it runs earlier?  I guess if you put
> $node->append_conf('postgresql.conf', 'lc_messages=C'); into
> src/bin/pg_amcheck/t/003_check.pl then it will work.  I'm not sure
> what the correct fix should be, ie if the template mechanism should
> notice this difference and not use the template, or if tests that
> depend on the message locale should explicitly say so with
> lc_messages=C or similar (why is this the only one?), or ...

So I tried this technique, but it gest old pretty fast: apparently
there's a *ton* of tests that depend on the locale.  I gave up after
patching the first five files, and noticing that in a second run there
another half a dozen failing tests that hadn't failed the first time
around.  (Not sure why this happened.)

So I think injecting --no-locale to the initdb line that creates the
template is a better approach; something like the attached.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/

Hi,

On 2023-08-29 17:54:24 +0200, Alvaro Herrera wrote:
> On 2023-Aug-27, Thomas Munro wrote:
> 
> > On Sun, Aug 27, 2023 at 3:03 AM Pavel Stehule <pavel.stehule@gmail.com> wrote:
> > > So it looks so IPC::Run::run is ignore parent environment
> > 
> > I guess the new initdb template captures lc_messages in
> > postgresql.conf, when it runs earlier?  I guess if you put
> > $node->append_conf('postgresql.conf', 'lc_messages=C'); into
> > src/bin/pg_amcheck/t/003_check.pl then it will work.  I'm not sure
> > what the correct fix should be, ie if the template mechanism should
> > notice this difference and not use the template, or if tests that
> > depend on the message locale should explicitly say so with
> > lc_messages=C or similar (why is this the only one?), or ...
> 
> So I tried this technique, but it gest old pretty fast: apparently
> there's a *ton* of tests that depend on the locale.  I gave up after
> patching the first five files, and noticing that in a second run there
> another half a dozen failing tests that hadn't failed the first time
> around.  (Not sure why this happened.)
> 
> So I think injecting --no-locale to the initdb line that creates the
> template is a better approach; something like the attached.

Makes sense, thanks for taking care of this.

Greetings,

Andres Freund

On Mon, 2023-09-11 at 19:23 -0700, Andres Freund wrote:
> > So I think injecting --no-locale to the initdb line that creates
> > the
> > template is a better approach; something like the attached.
>
> Makes sense, thanks for taking care of this.

After this, it seems "make check" no longer picks up the locale from
the system environment by default.

What is the new way to run the regression tests with an actual locale?
If it's no longer done by default, won't that dramatically reduce the
coverage of non-C locales?

Regards,
    Jeff Davis

Hi,

On 2023-10-10 17:08:25 -0700, Jeff Davis wrote:
> On Mon, 2023-09-11 at 19:23 -0700, Andres Freund wrote:
> > > So I think injecting --no-locale to the initdb line that creates
> > > the
> > > template is a better approach; something like the attached.
> > 
> > Makes sense, thanks for taking care of this.
> 
> After this, it seems "make check" no longer picks up the locale from
> the system environment by default.

Yea. I wonder if the better fix would have been to copy setenv("LC_MESSAGES", "C", 1);
to the initdb template creation. That afaict also fixes the issue, with a
smaller blast radius?

Greetings,

Andres Freund

On Tue, 2023-10-10 at 17:54 -0700, Andres Freund wrote:
> Yea. I wonder if the better fix would have been to copy
> setenv("LC_MESSAGES", "C", 1);
> to the initdb template creation. That afaict also fixes the issue,
> with a
> smaller blast radius?

Sounds good to me. Is there anything else we should do to notice that
tests are unexpectedly skipped, or is this a rare enough problem to not
worry about other cases?

Regards,
    Jeff Davis

On Tue, Oct 10, 2023 at 05:54:34PM -0700, Andres Freund wrote:
> On 2023-10-10 17:08:25 -0700, Jeff Davis wrote:
> > After this, it seems "make check" no longer picks up the locale from
> > the system environment by default.
> 
> Yea. I wonder if the better fix would have been to copy setenv("LC_MESSAGES", "C", 1);
> to the initdb template creation. That afaict also fixes the issue, with a
> smaller blast radius?

+1, that would restore the testing semantics known from v16-.  I think the
intent of the template was to optimize without changing semantics, and the
above proposal aligns with that.  Currently, for v17 alone, one needs
installcheck or build file hacks to reproduce a locale-specific test failure.

An alternative would be to declare that the tests are supported in one
encoding+locale only, then stop testing others in the buildfarm.  Even if we
did that, I'm fairly sure we'd standardize on UTF8, not SQL_ASCII, as the one
testable encoding.

Noah Misch <noah@leadboat.com> writes:
> An alternative would be to declare that the tests are supported in one
> encoding+locale only, then stop testing others in the buildfarm.

Surely that's not even a thinkably acceptable choice.

            regards, tom lane

On Tue, 2023-12-12 at 18:56 -0800, Noah Misch wrote:
> > Yea. I wonder if the better fix would have been to copy
> > setenv("LC_MESSAGES", "C", 1);
> > to the initdb template creation. That afaict also fixes the issue,
> > with a
> > smaller blast radius?
>
> +1, that would restore the testing semantics known from v16-.  I
> think the
> intent of the template was to optimize without changing semantics,
> and the
> above proposal aligns with that.  Currently, for v17 alone, one needs
> installcheck or build file hacks to reproduce a locale-specific test
> failure.

Attached.

I just changed --no-locale to --lc-messages=C, which I think solves it
in the right place with minimal blast radius. Andres, did you literally
mean C setenv() somewhere, or is this what you had in mind?

I also noticed that collate.linux.utf8.sql seems to be skipped on my
machine because of the "version() !~ 'linux-gnu'" check, even though
I'm running Ubuntu. Is that test getting run often enough?

And relatedly, is it worth thinking about extending pg_regress to
report skipped tests so it's easier to find these kinds of problems?

Regards,
    Jeff Davis

On Wed, 2023-12-20 at 17:48 -0800, Jeff Davis wrote:
> Attached.

It appears to increase the coverage. I committed it and I'll see how
the buildfarm reacts.

Regards,
    Jeff Davis

Hello Jeff,

22.12.2023 02:17, Jeff Davis wrote:
> On Wed, 2023-12-20 at 17:48 -0800, Jeff Davis wrote:
>> Attached.
> It appears to increase the coverage. I committed it and I'll see how
> the buildfarm reacts.

Starting from the commit 8793c6005, I observe a failure of test
collate.windows.win1252 on Windows Server 2016:
meson test regress/regress
1/1 postgresql:regress / regress/regress        ERROR 24.47s   exit status 1

regression.diffs contains:
@@ -993,6 +993,8 @@
  -- nondeterministic collations
  -- (not supported with libc provider)
  CREATE COLLATION ctest_det (locale = 'en_US', deterministic = true);
+ERROR:  could not create locale "en_US": No such file or directory
+DETAIL:  The operating system could not find any locale data for the locale name "en_US".
  CREATE COLLATION ctest_nondet (locale = 'en_US', deterministic = false);
  ERROR:  nondeterministic collations not supported with this provider
  -- cleanup

Though
CREATE COLLATION ctest_det (locale = 'English_United States', deterministic = true);
executed successfully on this OS.

AFAICS, before that commit SELECT getdatabaseencoding() in the test
returned SQL_ASCII, hence the test was essentially skipped, but now it
returns WIN1252, so problematic CREATE COLLATION(locale = 'en_US', ...)
is reached.

Best regards,
Alexander

On Thu, 2023-12-28 at 18:00 +0300, Alexander Lakhin wrote:
> AFAICS, before that commit SELECT getdatabaseencoding() in the test
> returned SQL_ASCII, hence the test was essentially skipped, but now
> it
> returns WIN1252, so problematic CREATE COLLATION(locale = 'en_US',
> ...)
> is reached.

We do want that test to run though, right?

I suspect that test line never worked reliably. The skip_test check at
the top guarantees that the collation named "en_US" exists, but that
doesn't mean that the OS understands the locale 'en_US'.

Perhaps we can change that line to use a similar trick as what's used
elsewhere in the file:

  do $$
  BEGIN
    EXECUTE 'CREATE COLLATION ctest_det (locale = ' ||
            quote_literal((SELECT collcollate FROM pg_collation WHERE
collname = ''en_US'')) || ', deterministic = true);';
  END
  $$;

The above may need some adjustment, but perhaps you can try it out?
Another option might be to use \gset to assign it to a variable, which
might be more readable, but I think it's better to just follow what the
rest of the file is doing.

Regards,
    Jeff Davis

28.12.2023 20:36, Jeff Davis wrote:
> We do want that test to run though, right?

Yes, I think so.

> I suspect that test line never worked reliably. The skip_test check at
> the top guarantees that the collation named "en_US" exists, but that
> doesn't mean that the OS understands the locale 'en_US'.
>
> Perhaps we can change that line to use a similar trick as what's used
> elsewhere in the file:
>
>    do $$
>    BEGIN
>      EXECUTE 'CREATE COLLATION ctest_det (locale = ' ||
>              quote_literal((SELECT collcollate FROM pg_collation WHERE
> collname = ''en_US'')) || ', deterministic = true);';
>    END
>    $$;
>
> The above may need some adjustment, but perhaps you can try it out?

Yes, this trick resolves the issue, it gives locale 'en-US' on that OS,
which works there. Please see the attached patch.

But looking at the result with the comment above that "do" block, I wonder
whether this successful CREATE COLLATION command is so important to perform
it that tricky way, if we want to demonstrate that nondeterministic
collations not supported.
So in case you decide just to remove this command, please see the second
patch.

Best regards,
Alexander

On Thu, 2023-12-28 at 22:00 +0300, Alexander Lakhin wrote:
> But looking at the result with the comment above that "do" block, I
> wonder
> whether this successful CREATE COLLATION command is so important to
> perform
> it that tricky way, if we want to demonstrate that nondeterministic
> collations not supported.

Thank you, pushed this version. There are other similar commands in the
file, so I think it's fine. It exercises a specific locale that might
be different from datcollate.

Regards,
    Jeff Davis