pgbench test failing on 14beta1 on Debian/i386

Christoph Berg <myon@debian.org> writes:

I can reproducibly get build failures in pgbench on 32-bit i386
Debian, both on sid and buster. (The older Debian stretch and Ubuntu
bionic are unaffected. Other architectures are also fine.)

The test that's failing came in with

6b258e3d688db14aadb58dde2a72939362310684
Author: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Tue Apr 6 11:50:42 2021 +0100

pgbench: Function to generate random permutations.

regards, tom lane

On Wed, May 19, 2021 at 9:51 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Christoph Berg <myon@debian.org> writes:

I can reproducibly get build failures in pgbench on 32-bit i386
Debian, both on sid and buster. (The older Debian stretch and Ubuntu
bionic are unaffected. Other architectures are also fine.)

The test that's failing came in with

6b258e3d688db14aadb58dde2a72939362310684
Author: Dean Rasheed <dean.a.rasheed@gmail.com>
Date: Tue Apr 6 11:50:42 2021 +0100

pgbench: Function to generate random permutations.

FWIW this is reproducible on my local Debian/gcc box with -m32, but
not on my FreeBSD/clang box with -m32. permute() produces different
values here:

\set t debug(permute(:size-1, :size, 5432) = 5301702756001087507 and \
permute(:size-2, :size, 5432) = 8968485976055840695 and \
permute(:size-3, :size, 5432) = 6708495591295582115 and \
permute(:size-4, :size, 5432) = 2801794404574855121 and \
permute(:size-5, :size, 5432) = 1489011409218895840 and \
permute(:size-6, :size, 5432) = 2267749475878240183 and \
permute(:size-7, :size, 5432) = 1300324176838786780)

I don't understand any of this stuff at all, but I added a bunch of
printfs and worked out that the first point its local variables
diverge is here:

/* Random offset */
r = (uint64) getrand(&random_state2, 0, size - 1);

... after 4 earlier getrand() produced matching values. Hmm.

On Wed, May 19, 2021 at 11:34 AM Thomas Munro <thomas.munro@gmail.com> wrote:

I don't understand any of this stuff at all, but I added a bunch of
printfs and worked out that the first point its local variables
diverge is here:

/* Random offset */
r = (uint64) getrand(&random_state2, 0, size - 1);

Forgot to post the actual values:

r = 2563421694876090368
r = 2563421694876090365

Smells a bit like a precision problem in the workings of pg_erand48(),
but as soon as I saw floating point numbers I closed my laptop and ran
for the door.

Thomas Munro <thomas.munro@gmail.com> writes:

Forgot to post the actual values:
r = 2563421694876090368
r = 2563421694876090365
Smells a bit like a precision problem in the workings of pg_erand48(),
but as soon as I saw floating point numbers I closed my laptop and ran
for the door.

Yup. This test has a touching, but entirely unwarranted, faith in
pg_erand48() producing bit-for-bit the same values everywhere.

regards, tom lane

Forgot to post the actual values:
r = 2563421694876090368
r = 2563421694876090365
Smells a bit like a precision problem in the workings of pg_erand48(),
but as soon as I saw floating point numbers I closed my laptop and ran
for the door.

Yup. This test has a touching, but entirely unwarranted, faith in
pg_erand48() producing bit-for-bit the same values everywhere.

Indeed.

I argued against involving any floats computation on principle, but Dean
was confident it could work, and it did simplify the code, so it did not
look that bad an option.

I see two simple approaches:

(1) use another PRNG inside pgbench, eg Knuth's which was used in some
previous submission and is very simple and IMHO better than the rand48
stuff.

(2) extend pg_*rand48() to provide an unsigned 64 bits out of the 48 bits
state.

Any preference?

--
Fabien.

On Wed, May 19, 2021 at 09:06:16AM +0200, Fabien COELHO wrote:

I see two simple approaches:

(1) use another PRNG inside pgbench, eg Knuth's which was used in some
previous submission and is very simple and IMHO better than the rand48
stuff.

(2) extend pg_*rand48() to provide an unsigned 64 bits out of the 48 bits
state.

Or, (3) remove this test? I am not quite sure what there is to gain
with this extra test considering all the other tests with permute()
already present in this script.
--
Michael

On Wed, 19 May 2021 at 00:35, Thomas Munro <thomas.munro@gmail.com> wrote:

FWIW this is reproducible on my local Debian/gcc box with -m32,

Confirmed, thanks for looking. I can reproduce it on my machine with
-m32. It's somewhat annoying that the buildfarm didn't pick it up
sooner :-(

On Wed, 19 May 2021 at 08:28, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 19, 2021 at 09:06:16AM +0200, Fabien COELHO wrote:

I see two simple approaches:

(1) use another PRNG inside pgbench, eg Knuth's which was used in some
previous submission and is very simple and IMHO better than the rand48
stuff.

(2) extend pg_*rand48() to provide an unsigned 64 bits out of the 48 bits
state.

Or, (3) remove this test? I am not quite sure what there is to gain
with this extra test considering all the other tests with permute()
already present in this script.

Yes, I think removing the test is the best option. It was originally
added because there was a separate code path for larger permutation
sizes that needed testing, but that's no longer the case so the test
really isn't adding anything.

Regards,
Dean

Confirmed, thanks for looking. I can reproduce it on my machine with
-m32. It's somewhat annoying that the buildfarm didn't pick it up
sooner :-(

On Wed, 19 May 2021 at 08:28, Michael Paquier <michael@paquier.xyz> wrote:

On Wed, May 19, 2021 at 09:06:16AM +0200, Fabien COELHO wrote:

I see two simple approaches:

(1) use another PRNG inside pgbench, eg Knuth's which was used in some
previous submission and is very simple and IMHO better than the rand48
stuff.

(2) extend pg_*rand48() to provide an unsigned 64 bits out of the 48 bits
state.

Or, (3) remove this test? I am not quite sure what there is to gain
with this extra test considering all the other tests with permute()
already present in this script.

Yes, I think removing the test is the best option. It was originally
added because there was a separate code path for larger permutation
sizes that needed testing, but that's no longer the case so the test
really isn't adding anything.

Hmmm…

It is the one test which worked in actually detecting an issue, so I would
not say that it is not adding anything, on the contrary, it did prove its
value! The permute function is expected to be deterministic on different
platforms and architectures, and it is not.

I agree that removing the test will hide the issue effectively:-) but
ISTM more appropriate to solve the underlying issue and keep the test.

I'd agree with a two phases approach: drop the test in the short term and
deal with the PRNG later. I'm sooooo unhappy with this 48 bit PRNG that I
may be motivated enough to attempt to replace it, or at least add a better
(faster?? larger state?? same/better quality?) alternative.

--
Fabien.

Hello Dean,

Or, (3) remove this test? I am not quite sure what there is to gain
with this extra test considering all the other tests with permute()
already present in this script.

Yes, I think removing the test is the best option. It was originally
added because there was a separate code path for larger permutation
sizes that needed testing, but that's no longer the case so the test
really isn't adding anything.

Hmmm…

It is the one test which worked in actually detecting an issue, so I would
not say that it is not adding anything, on the contrary, it did prove its
value! The permute function is expected to be deterministic on different
platforms and architectures, and it is not.

I agree that removing the test will hide the issue effectively:-) but ISTM
more appropriate to solve the underlying issue and keep the test.

I'd agree with a two phases approach: drop the test in the short term and
deal with the PRNG later. I'm sooooo unhappy with this 48 bit PRNG that I may
be motivated enough to attempt to replace it, or at least add a better
(faster?? larger state?? same/better quality?) alternative.

Attached patch disactivates the test with comments to outline that there
is an issue to fix… so it is *not* removed.

I'm obviously okay with providing an alternate PRNG, let me know if this
is the prefered option.

--
Fabien.

On Wed, 19 May 2021 at 11:32, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Or, (3) remove this test? I am not quite sure what there is to gain
with this extra test considering all the other tests with permute()
already present in this script.

Yes, I think removing the test is the best option. It was originally
added because there was a separate code path for larger permutation
sizes that needed testing, but that's no longer the case so the test
really isn't adding anything.

Hmmm…

It is the one test which worked in actually detecting an issue, so I would
not say that it is not adding anything, on the contrary, it did prove its
value! The permute function is expected to be deterministic on different
platforms and architectures, and it is not.

In fact what it demonstrates is that the results from permute(), like
all the other pgbench random functions, will vary by platform for
sufficiently large size parameters.

I'd agree with a two phases approach: drop the test in the short term and
deal with the PRNG later. I'm sooooo unhappy with this 48 bit PRNG that I
may be motivated enough to attempt to replace it, or at least add a better
(faster?? larger state?? same/better quality?) alternative.

I don't necessarily have a problem with that provided the replacement
is well-chosen and has a proven track record (i.e., let's not invent
our own PRNG).

For now though, I'll go remove the test.

Regards,
Dean

On Wed, 19 May 2021 at 12:07, Fabien COELHO <coelho@cri.ensmp.fr> wrote:

Attached patch disactivates the test with comments to outline that there
is an issue to fix… so it is *not* removed.

I opted to just remove the test rather than comment it out, since the
issue highlighted isn't specific to permute(). Also changing the PRNG
will completely change the results, so all the test values would
require rewriting, rather than it just being a case of uncommenting
the test and expecting it to work.

I'm obviously okay with providing an alternate PRNG, let me know if this
is the prefered option.

That's something for consideration in v15. If we do decide we want a
new PRNG, it should apply across the board to all pgbench random
functions.

Regards,
Dean

Or, (3) remove this test? I am not quite sure what there is to gain
with this extra test considering all the other tests with permute()
already present in this script.

Yes, I think removing the test is the best option. It was originally
added because there was a separate code path for larger permutation
sizes that needed testing, but that's no longer the case so the test
really isn't adding anything.

Hmmm…

It is the one test which worked in actually detecting an issue, so I would
not say that it is not adding anything, on the contrary, it did prove its
value! The permute function is expected to be deterministic on different
platforms and architectures, and it is not.

In fact what it demonstrates is that the results from permute(), like
all the other pgbench random functions, will vary by platform for
sufficiently large size parameters.

Indeed, it is the case if the underlying math use doubles & large numbers.
For integer-only computations it should be safe though, and permute should
be in this category.

I'd agree with a two phases approach: drop the test in the short term and
deal with the PRNG later. I'm sooooo unhappy with this 48 bit PRNG that I
may be motivated enough to attempt to replace it, or at least add a better
(faster?? larger state?? same/better quality?) alternative.

I don't necessarily have a problem with that provided the replacement
is well-chosen and has a proven track record (i.e., let's not invent
our own PRNG).

Yes, obviously, I'm not daft enough to reinvent a PRNG. The question is to
chose one, motivate the choice, and build the relevant API for what pg
needs, possibly with some benchmarking.

For now though, I'll go remove the test.

This also removes the reminder…

--
Fabien.

On 5/19/21 6:32 AM, Fabien COELHO wrote:

Confirmed, thanks for looking. I can reproduce it on my machine with
-m32. It's somewhat annoying that the buildfarm didn't pick it up
sooner :-(

On Wed, 19 May 2021 at 08:28, Michael Paquier <michael@paquier.xyz>
wrote:

On Wed, May 19, 2021 at 09:06:16AM +0200, Fabien COELHO wrote:

I see two simple approaches:

(1) use another PRNG inside pgbench, eg Knuth's which was used in some
previous submission and is very simple and IMHO better than the rand48
stuff.

(2) extend pg_*rand48() to provide an unsigned 64 bits out of the
48 bits
state.

Or, (3) remove this test?  I am not quite sure what there is to gain
with this extra test considering all the other tests with permute()
already present in this script.

Yes, I think removing the test is the best option. It was originally
added because there was a separate code path for larger permutation
sizes that needed testing, but that's no longer the case so the test
really isn't adding anything.

Hmmm…

It is the one test which worked in actually detecting an issue, so I
would not say that it is not adding anything, on the contrary, it did
prove its value! The permute function is expected to be deterministic
on different platforms and architectures, and it is not.

I agree that removing the test will hide the issue effectively:-) but
ISTM more appropriate to solve the underlying issue and keep the test.

I'd agree with a two phases approach: drop the test in the short term
and deal with the PRNG later. I'm sooooo unhappy with this 48 bit PRNG
that I may be motivated enough to attempt to replace it, or at least
add a better (faster?? larger state?? same/better quality?) alternative.

Yeah, this does seem to be something that should be fixed rather than
hidden.

cheers

andrew

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

In the meantime postgresql-14 has been accepted into Debian/experimental:

https://buildd.debian.org/status/logs.php?pkg=postgresql-14&amp;ver=14%7Ebeta1-1

Interestingly, the test is only failing on i386 and none of the other
architectures, which could hint at 80-bit extended precision FP
problems.

(The sparc64 error there is something else, I'll try rerunning it.
command failed: "psql" -X -c "CREATE DATABASE \"isolation_regression\" TEMPLATE=template0" "postgres")

Christoph

Christoph Berg <myon@debian.org> writes:

Interestingly, the test is only failing on i386 and none of the other
architectures, which could hint at 80-bit extended precision FP
problems.

Yeah, that's what I'd assumed it is. We suppress that where we can
with -fexcess-precision=standard or -msse2, but I'm guessing that
doesn't help here for some reason.

regards, tom lane