Thread

Commits

Fix portability problem in pgbench.
- e6c3ba7fbfd5 12.0 landed
- 27d6bc68f98e 11.2 landed
Avoid assuming that we know the spelling of getopt_long's error messages.
- 434040159558 11.2 landed
- 251f57460f12 12.0 landed

PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T05:04:10Z

Although we've got a few NetBSD and OpenBSD buildfarm critters,
none of them are doing --enable-tap-tests.  If they were, we'd
have noticed the pgbench regression tests falling over:

not ok 3 - pgbench option error: bad option stderr /(?^:(unrecognized|illegal) option)/
#   Failed test 'pgbench option error: bad option stderr /(?^:(unrecognized|illegal) option)/'
#   at t/002_pgbench_no_server.pl line 190.
#                   'pgbench: unknown option -- bad-option
# Try "pgbench --help" for more information.
# '
#     doesn't match '(?^:(unrecognized|illegal) option)'

Sure enough, manual testing confirms that on these platforms
that error message is spelled differently:

$ pgbench --bad-option
pgbench: unknown option -- bad-option
Try "pgbench --help" for more information.


I am, TBH, inclined to fix this by removing that test case rather
than teaching it another spelling to accept.  I think it's very
hard to make the case that tests like this one are anything but
a waste of developer and buildfarm time.  When they are also a
portability hazard, it's time to cut our losses.  (I also note
that this test has caused us problems before, cf 869aa40a2 and
933851033.)

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-17T09:46:34Z

Hello Tom,

> Although we've got a few NetBSD and OpenBSD buildfarm critters,
> none of them are doing --enable-tap-tests.  If they were, we'd
> have noticed the pgbench regression tests falling over:
>
> [...]
>
> I am, TBH, inclined to fix this by removing that test case rather
> than teaching it another spelling to accept.  I think it's very
> hard to make the case that tests like this one are anything but
> a waste of developer and buildfarm time.  When they are also a
> portability hazard, it's time to cut our losses.  (I also note
> that this test has caused us problems before, cf 869aa40a2 and
> 933851033.)

I'd rather keep it by simply adding the "|unknown" alternative. 30 years 
of programming have taught me that testing limit & error cases is useful, 
although you never know when it will be proven so.

Client application coverage is currently abysmal, especially "psql" 
despite the many script used for testing (39% of lines, 42% of 
functions!), pgbench is under 90%. Generally we really need more tests, 
not less. TAP tests are a good compromise because they are not always 
run, and ISTM sometimes (i.e. you asked for it) is enough.

I agree that some tests can be useless, but I do not think that it applies 
to this one. This test also checks that under a bad option pgbench stops 
with an appropriate 1 exit status. Recently a patch updated the exit 
status of pgbench in many cases to distinguish between different kind 
errors, thus having non-regression in this area was shown to be a good 
idea. Moreover, knowing that the exit status on getopt errors is 
consistent across platform has value, and knowing that there is some 
variability is not uninteresting.

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Mikael Kjellström <mikael.kjellstrom@mksoft.nu> — 2019-01-17T21:12:57Z

On 2019-01-17 06:04, Tom Lane wrote:
> Although we've got a few NetBSD and OpenBSD buildfarm critters,
> none of them are doing --enable-tap-tests.  If they were, we'd
> have noticed the pgbench regression tests falling over:

For what it's worth I've enabled tap-tests for my OpenBSD 5.9 (curculio) 
and NetBSD 7 (sidewinder) animals now.

/Mikael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T21:16:21Z

=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= <mikael.kjellstrom@mksoft.nu> writes:
> On 2019-01-17 06:04, Tom Lane wrote:
>> Although we've got a few NetBSD and OpenBSD buildfarm critters,
>> none of them are doing --enable-tap-tests.  If they were, we'd
>> have noticed the pgbench regression tests falling over:

> For what it's worth I've enabled tap-tests for my OpenBSD 5.9 (curculio) 
> and NetBSD 7 (sidewinder) animals now.

Oh, thanks!  I'm guessing they'll fail their next runs, but I'll
wait to see confirmation of that before I do anything about the
test bug.

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Mikael Kjellström <mikael.kjellstrom@mksoft.nu> — 2019-01-17T21:19:25Z

On 2019-01-17 22:16, Tom Lane wrote:

>> For what it's worth I've enabled tap-tests for my OpenBSD 5.9 (curculio)
>> and NetBSD 7 (sidewinder) animals now.
> 
> Oh, thanks!  I'm guessing they'll fail their next runs, but I'll
> wait to see confirmation of that before I do anything about the
> test bug.

They should run the next time within the hour or hour and a half so I 
guess we will find out soon enough.

/Mikael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Mikael Kjellström <mikael.kjellstrom@mksoft.nu> — 2019-01-17T21:38:10Z

On 2019-01-17 22:19, Mikael Kjellström wrote:

> On 2019-01-17 22:16, Tom Lane wrote:
> 
>>> For what it's worth I've enabled tap-tests for my OpenBSD 5.9 (curculio)
>>> and NetBSD 7 (sidewinder) animals now.
>>
>> Oh, thanks!  I'm guessing they'll fail their next runs, but I'll
>> wait to see confirmation of that before I do anything about the
>> test bug.
> 
> They should run the next time within the hour or hour and a half so I 
> guess we will find out soon enough.

Hm, that didn't go so well.

It says:

configure: error: Additional Perl modules are required to run TAP tests

so how do I find out with Perl modules that are required?

/Mikael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T21:42:38Z

=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= <mikael.kjellstrom@mksoft.nu> writes:
> It says:
> configure: error: Additional Perl modules are required to run TAP tests
> so how do I find out with Perl modules that are required?

If you look into the configure log it should say just above that,
but I'm betting you just need p5-IPC-Run.

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Mikael Kjellström <mikael.kjellstrom@mksoft.nu> — 2019-01-17T21:47:33Z


On 2019-01-17 22:42, Tom Lane wrote:

> =?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= <mikael.kjellstrom@mksoft.nu> writes:
>> It says:
>> configure: error: Additional Perl modules are required to run TAP tests
>> so how do I find out with Perl modules that are required?
> 
> If you look into the configure log it should say just above that,
> but I'm betting you just need p5-IPC-Run.

Yes it seems to be IPC::Run that is missing.

I've installed it manually through CPAN.

Let's see if it works better this time.

/Mikael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Mikael Kjellström <mikael.kjellstrom@mksoft.nu> — 2019-01-17T22:12:43Z

On 2019-01-17 22:47, Mikael Kjellström wrote:
> 
> 
> On 2019-01-17 22:42, Tom Lane wrote:
> 
>> =?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= <mikael.kjellstrom@mksoft.nu> writes:
>>> It says:
>>> configure: error: Additional Perl modules are required to run TAP tests
>>> so how do I find out with Perl modules that are required?
>>
>> If you look into the configure log it should say just above that,
>> but I'm betting you just need p5-IPC-Run.
> 
> Yes it seems to be IPC::Run that is missing.
> 
> I've installed it manually through CPAN.
> 
> Let's see if it works better this time.

Hmmm, nope:

================== 
pgsql.build/src/bin/pg_ctl/tmp_check/log/003_promote_standby.log 
===================
2019-01-17 23:09:20.343 CET [9129] LOG:  listening on Unix socket 
"/tmp/g66P1fpMFK/.s.PGSQL.64980"
2019-01-17 23:09:20.343 CET [9129] FATAL:  could not create semaphores: 
No space left on device
2019-01-17 23:09:20.343 CET [9129] DETAIL:  Failed system call was 
semget(64980002, 17, 03600).
2019-01-17 23:09:20.343 CET [9129] HINT:  This error does *not* mean 
that you have run out of disk space.  It occurs when either the system 
limit for the maximum number of semaphore sets (SEMMNI), or the system 
wide maximum number of semaphores (SEMMNS), would be exceeded.  You need 
to raise the respective kernel parameter.  Alternatively, reduce 
PostgreSQL's consumption of semaphores by reducing its max_connections 
parameter.
	The PostgreSQL documentation contains more information about 
configuring your system for PostgreSQL.
2019-01-17 23:09:20.345 CET [9129] LOG:  database system is shut down

will try and increase SEMMNI and see if that helps.

/Mikael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T22:23:02Z

=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= <mikael.kjellstrom@mksoft.nu> writes:
>> Let's see if it works better this time.

> Hmmm, nope:

> 2019-01-17 23:09:20.343 CET [9129] FATAL:  could not create semaphores: 
> No space left on device

Yeah, you might've been able to get by with OpenBSD/NetBSD's default
semaphore settings before, but they really only let one postmaster
run at a time; and the TAP tests want to start more than one.
For me it seems to work to append this to /etc/sysctl.conf:

kern.seminfo.semmni=100
kern.seminfo.semmns=2000

and either reboot, or install those settings manually with sysctl.

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Mikael Kjellström <mikael.kjellstrom@mksoft.nu> — 2019-01-17T22:37:34Z

On 2019-01-17 23:23, Tom Lane wrote:

> Yeah, you might've been able to get by with OpenBSD/NetBSD's default
> semaphore settings before, but they really only let one postmaster
> run at a time; and the TAP tests want to start more than one.
> For me it seems to work to append this to /etc/sysctl.conf:
> 
> kern.seminfo.semmni=100
> kern.seminfo.semmns=2000
> 
> and either reboot, or install those settings manually with sysctl.

Looks that way.

I've increased the values and rebooted the machines.

Let's hope 5th time is the charm :-)

/Mikael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Mikael Kjellström <mikael.kjellstrom@mksoft.nu> — 2019-01-17T22:54:46Z

On 2019-01-17 23:37, Mikael Kjellström wrote:
> 
> On 2019-01-17 23:23, Tom Lane wrote:
> 
>> Yeah, you might've been able to get by with OpenBSD/NetBSD's default
>> semaphore settings before, but they really only let one postmaster
>> run at a time; and the TAP tests want to start more than one.
>> For me it seems to work to append this to /etc/sysctl.conf:
>>
>> kern.seminfo.semmni=100
>> kern.seminfo.semmns=2000
>>
>> and either reboot, or install those settings manually with sysctl.
> 
> Looks that way.
> 
> I've increased the values and rebooted the machines.
> 
> Let's hope 5th time is the charm :-)

Nope!

But it looks like in NetBSD the options are called:

netbsd7-pgbf# sysctl -a | grep semmn
kern.ipc.semmni = 10
kern.ipc.semmns = 60
kern.ipc.semmnu = 30

so I will try and set that in /etc/sysctl.conf and reboot and see what 
happens.

/Mikael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Mikael Kjellström <mikael.kjellstrom@mksoft.nu> — 2019-01-17T23:00:49Z


On 2019-01-17 23:54, Mikael Kjellström wrote:

> But it looks like in NetBSD the options are called:
> 
> netbsd7-pgbf# sysctl -a | grep semmn
> kern.ipc.semmni = 10
> kern.ipc.semmns = 60
> kern.ipc.semmnu = 30
> 
> so I will try and set that in /etc/sysctl.conf and reboot and see what 
> happens.

That seems to have done the trick:

netbsd7-pgbf# sysctl -a | grep semmn
kern.ipc.semmni = 100
kern.ipc.semmns = 2000
kern.ipc.semmnu = 30

I just started another run on sidewinder (NetBSD 7), let's see how that 
goes.

but the OpenBSD machine went further and now fails on:

pgbenchCheck instead.

Is that the failure you expected to get?

/Mikael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Mikael Kjellström <mikael.kjellstrom@mksoft.nu> — 2019-01-17T23:10:01Z

On 2019-01-18 00:00, Mikael Kjellström wrote:


> I just started another run on sidewinder (NetBSD 7), let's see how that 
> goes.
> 
> but the OpenBSD machine went further and now fails on:
> 
> pgbenchCheck instead.
> 
> Is that the failure you expected to get?

And now also the NetBSD machine failed on pgbenchCheck.

https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=sidewinder&dt=2019-01-17%2022%3A57%3A14

should I leave it as it is for now?

/Mikael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T23:11:20Z

=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= <mikael.kjellstrom@mksoft.nu> writes:
>> But it looks like in NetBSD the options are called:

Sorry about that, I copied-and-pasted from the openbsd machine I was
looking at without remembering that netbsd is just a shade different.

> but the OpenBSD machine went further and now fails on:
> pgbenchCheck instead.
> Is that the failure you expected to get?

Yup, sure is.  Thanks!

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-17T23:31:27Z

=?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= <mikael.kjellstrom@mksoft.nu> writes:
> And now also the NetBSD machine failed on pgbenchCheck.

Indeed, as expected.

> should I leave it as it is for now?

Please.  I'll push a fix for the broken test case in a bit --- I
just wanted to confirm that somebody else's machines agreed that
it's broken.

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Mikael Kjellström <mikael.kjellstrom@mksoft.nu> — 2019-01-17T23:46:45Z

On 2019-01-18 00:31, Tom Lane wrote:
> =?UTF-8?Q?Mikael_Kjellstr=c3=b6m?= <mikael.kjellstrom@mksoft.nu> writes:
>> And now also the NetBSD machine failed on pgbenchCheck.
> 
> Indeed, as expected.

Ok.


>> should I leave it as it is for now?
> 
> Please.  I'll push a fix for the broken test case in a bit --- I
> just wanted to confirm that somebody else's machines agreed that
> it's broken.

Ok, I will leave it on then.

/Mikael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-18T00:21:08Z

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> I am, TBH, inclined to fix this by removing that test case rather
>> than teaching it another spelling to accept.  I think it's very
>> hard to make the case that tests like this one are anything but
>> a waste of developer and buildfarm time.  When they are also a
>> portability hazard, it's time to cut our losses.  (I also note
>> that this test has caused us problems before, cf 869aa40a2 and
>> 933851033.)

> I'd rather keep it by simply adding the "|unknown" alternative. 30 years 
> of programming have taught me that testing limit & error cases is useful, 
> although you never know when it will be proven so.

Sorry, I don't buy this line of argument.  Reasonable test design requires
making cost/benefit tradeoffs: the cost to run the test over and over,
and the cost to maintain the test itself (e.g. fix portability issues in
it) have to be balanced against the probability of it finding something
useful.  I judge that the chance of this particular test finding something
is small, and I've had quite enough of the maintenance costs.

Just to point up that we're still not clearly done with the maintenance
costs of supposing that we know how every version of getopt_long will
spell this error message, I note that my Linux box seems to have two
variants of it:

$ pgbench -z 
pgbench: invalid option -- 'z'
Try "pgbench --help" for more information.
$ pgbench --z
pgbench: unrecognized option '--z'
Try "pgbench --help" for more information.

of which the "invalid" alternative is also not in our list right now.
Who's to say how many more versions of getopt_long are out there,
or what the maintainers thereof might do in the future?

> I agree that some tests can be useless, but I do not think that it applies 
> to this one. This test also checks that under a bad option pgbench stops 
> with an appropriate 1 exit status.

It's possible that it's worth the trouble to check for exit status 1,
but I entirely fail to see the point of checking exactly what is the
spelling of a message that is issued by code not under our control.

Looking closer at the test case:

    [
        'bad option',
        '-h home -p 5432 -U calvin -d --bad-option',
        [ qr{(unrecognized|illegal) option}, qr{--help.*more information} ]
    ],

ISTM that just removing the first qr{} pattern, and checking only that
we get the text that *is* under our control, is a reasonable compromise
here.

            regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Michael Paquier <michael@paquier.xyz> — 2019-01-18T00:43:38Z

On Thu, Jan 17, 2019 at 07:21:08PM -0500, Tom Lane wrote:
> Sorry, I don't buy this line of argument.  Reasonable test design requires
> making cost/benefit tradeoffs: the cost to run the test over and over,
> and the cost to maintain the test itself (e.g. fix portability issues in
> it) have to be balanced against the probability of it finding something
> useful.  I judge that the chance of this particular test finding something
> is small, and I've had quite enough of the maintenance costs.

Yes, I agree with Tom's line of thoughts here.  It seems to me that
just dropping this part of the test is just but fine.
--
Michael

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-18T06:18:44Z

BTW, if you're wondering why curculio is still failing the pgbench
test, all is explained here:

https://man.openbsd.org/srandom

Or at least most is explained there.  While curculio is unsurprisingly
failing all four seeded_random tests, when I try it locally on an
OpenBSD 6.4 installation, only the uniform, exponential, and gaussian
cases reliably "fail".  zipfian usually doesn't.  It looks like the
zipfian code almost always produces 4000 regardless of the seed value,
though occasionally it produces 4001.  Bad parameters for that
algorithm, perhaps?

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-18T08:26:49Z

>> I'd rather keep it by simply adding the "|unknown" alternative. 30 years
>> of programming have taught me that testing limit & error cases is useful,
>> although you never know when it will be proven so.
>
> Sorry, I don't buy this line of argument.

> Reasonable test design requires making cost/benefit tradeoffs: the cost 
> to run the test over and over, and the cost to maintain the test itself 
> (e.g. fix portability issues in it) have to be balanced against the 
> probability of it finding something useful.  I judge that the chance of 
> this particular test finding something is small, and I've had quite 
> enough of the maintenance costs.
>
> Just to point up that we're still not clearly done with the maintenance
> costs of supposing that we know how every version of getopt_long will
> spell this error message, I note that my Linux box seems to have two
> variants of it:
>
> $ pgbench -z
> pgbench: invalid option -- 'z'
> Try "pgbench --help" for more information.
> $ pgbench --z
> pgbench: unrecognized option '--z'
> Try "pgbench --help" for more information.
>
> of which the "invalid" alternative is also not in our list right now.
> Who's to say how many more versions of getopt_long are out there,
> or what the maintainers thereof might do in the future?

ISTM that the getopt implementers imagination should run out in the end:-) 
invalid, unknown, unrecognized, unexpected, incorrect... Ok English has 
many words:-)

>> I agree that some tests can be useless, but I do not think that it applies
>> to this one. This test also checks that under a bad option pgbench stops
>> with an appropriate 1 exit status.
>
> It's possible that it's worth the trouble to check for exit status 1,
> but I entirely fail to see the point of checking exactly what is the
> spelling of a message that is issued by code not under our control.
>
> Looking closer at the test case:
>
>    [
>        'bad option',
>        '-h home -p 5432 -U calvin -d --bad-option',
>        [ qr{(unrecognized|illegal) option}, qr{--help.*more information} ]
>    ],
>
> ISTM that just removing the first qr{} pattern, and checking only that
> we get the text that *is* under our control, is a reasonable compromise
> here.

Possibly. I'd be a little happier if it checks for a non-empty error 
message, eg qr{...} or qr{option} (the message should say something about 
the option).

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-18T08:37:26Z

> BTW, if you're wondering why curculio is still failing the pgbench
> test,

Hmmm, that is interesting! It shows that at least some TAP tests are 
useful.

> all is explained here:
>
> https://man.openbsd.org/srandom
>
> Or at least most is explained there.

Yep. They try to be more serious than other systems about PRNG, which is 
not bad in itself.

> While curculio is unsurprisingly failing all four seeded_random tests, 
> when I try it locally on an OpenBSD 6.4 installation, only the uniform, 
> exponential, and gaussian cases reliably "fail".  zipfian usually 
> doesn't.

> It looks like the zipfian code almost always produces 4000 regardless of 
> the seed value, though occasionally it produces 4001.  Bad parameters 
> for that algorithm, perhaps?

Welcome to the zipfian highly skewed distribution! I'll check the 
parameters used in the test, maybe it should use something less extreme.

srandom is only used for initializing the state of various internal rand48 
LCG PRNG for pgbench.

Maybe on OpenBSD pg should switch srandom to srandom_deterministic?

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-18T17:56:22Z

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> all is explained here:
>> https://man.openbsd.org/srandom
>> Or at least most is explained there.

> Yep. They try to be more serious than other systems about PRNG, which is 
> not bad in itself.

> Maybe on OpenBSD pg should switch srandom to srandom_deterministic?

Dunno.  I'm fairly annoyed by their idea that they're smarter than POSIX.
However, for most of our uses of srandom, this behavior isn't awful;
it's only pgbench that has an expectation that the platform random()
can be made to behave deterministically.  And TBH I think that's just
an expectation that's going to bite us.

I'd suggest that maybe we should get rid of the use of both random()
and srandom() in pgbench, and go over to letting set_random_seed()
fill the pg_erand48 state directly.  In the integer-seed case you
could use something equivalent to pg_srand48.  (In the other cases
probably you could do better, certainly the strong-random case could
just fill all 6 bytes directly.)  That would get us to a place where
the behavior of --random-seed=N is not only deterministic but
platform-independent, which seems like an improvement.

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-18T22:01:07Z

>> Maybe on OpenBSD pg should switch srandom to srandom_deterministic?
>
> Dunno.  I'm fairly annoyed by their idea that they're smarter than POSIX.
> However, for most of our uses of srandom, this behavior isn't awful;
> it's only pgbench that has an expectation that the platform random()
> can be made to behave deterministically.  And TBH I think that's just
> an expectation that's going to bite us.
>
> I'd suggest that maybe we should get rid of the use of both random()
> and srandom() in pgbench, and go over to letting set_random_seed()
> fill the pg_erand48 state directly.  In the integer-seed case you
> could use something equivalent to pg_srand48.  (In the other cases
> probably you could do better, certainly the strong-random case could
> just fill all 6 bytes directly.)  That would get us to a place where
> the behavior of --random-seed=N is not only deterministic but
> platform-independent, which seems like an improvement.

That's a point. Althought I'm not found of round48, indeed having 
something platform independent for testing makes definite sense.

I'll look into it.

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-20T10:07:46Z

Hello Tom,

>>> Maybe on OpenBSD pg should switch srandom to srandom_deterministic?
>> 
>> Dunno.  I'm fairly annoyed by their idea that they're smarter than POSIX.

Hmmm. I'm afraid that is not that hard.

>> However, for most of our uses of srandom, this behavior isn't awful;
>> it's only pgbench that has an expectation that the platform random()
>> can be made to behave deterministically.  And TBH I think that's just
>> an expectation that's going to bite us.
>> 
>> I'd suggest that maybe we should get rid of the use of both random()
>> and srandom() in pgbench, and go over to letting set_random_seed()
>> fill the pg_erand48 state directly.  In the integer-seed case you
>> could use something equivalent to pg_srand48.  (In the other cases
>> probably you could do better, certainly the strong-random case could
>> just fill all 6 bytes directly.)  That would get us to a place where
>> the behavior of --random-seed=N is not only deterministic but
>> platform-independent, which seems like an improvement.
>
> That's a point. Althought I'm not found of round48, indeed having something 
> platform independent for testing makes definite sense.
>
> I'll look into it.

Here is a POC which defines an internal interface for a PRNG, and use it 
within pgbench, with several possible implementations which default to 
rand48.

I must admit that I have a grudge against standard rand48:

  - it is a known poor PRNG which was designed at a time when LCG where
    basically the only low cost PRNG available. Newer designs were very
    recent when the standard was set.
  - it is a LCG, i.e. its low bits cycle quickly, so should not be used.
  - so the 48 bit state size is relevant for generating 32 bits ints
    and floats.
  - however it eis used to generate more bits...
  - the double function uses all 48 bits, whereas 52 need to be filled...
  - and it is used to generate integers, which means that for large range
    some values are inaccessible.
  - 3 * 16 bits integers state looks silly on 32/64 bit architectures.
  - ...

Given that postgres needs doubles (52 bits mantissa) and possibly 64 bits 
integers, IMO the internal state should be 64 bits as a bare minimum, 
which anyway is also the minimal bite on 64 bit architectures, which is 
what is encoutered in practice.

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-20T20:26:21Z

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>>> I'd suggest that maybe we should get rid of the use of both random()
>>> and srandom() in pgbench, and go over to letting set_random_seed()
>>> fill the pg_erand48 state directly.

> Here is a POC which defines an internal interface for a PRNG, and use it 
> within pgbench, with several possible implementations which default to 
> rand48.

I seriously dislike this patch.  pgbench's random support is quite
overengineered already IMO, and this proposes to add a whole batch of
new code and new APIs to fix a very small bug.

> I must admit that I have a grudge against standard rand48:

I think this is nonsense, particularly the claim that anything in PG
cares about the lowest-order bits of random doubles.  I'm aware that
there are applications where that does matter, but people aren't doing
high-precision weather simulations in pgbench.

BTW, did you look at the question of the range of zipfian?  I confirmed
here that as used in the test case, it's generating a range way smaller
than the other ones: repeating the insertion snippet 1000x produces stats
like this:

regression=# select seed,rand,min(val),max(val),count(distinct val) from seeded_random group by 1,2 order by 2,1;
    seed    |    rand     | min  | max  | count 
------------+-------------+------+------+-------
 1957482663 | exponential | 2000 | 2993 |   586
 1958556409 | exponential | 2000 | 2995 |   569
 1959867462 | exponential | 2000 | 2997 |   569
 1957482663 | gaussian    | 3009 | 3997 |   493
 1958556409 | gaussian    | 3027 | 3956 |   501
 1959867462 | gaussian    | 3018 | 3960 |   511
 1957482663 | uniform     | 1001 | 1999 |   625
 1958556409 | uniform     | 1000 | 1999 |   642
 1959867462 | uniform     | 1001 | 1999 |   630
 1957482663 | zipfian     | 4000 | 4081 |    19
 1958556409 | zipfian     | 4000 | 4022 |    18
 1959867462 | zipfian     | 4000 | 4156 |    23

I have no idea whether that indicates an actual bug, or just poor
choice of parameter in the test's call.  But the very small number
of distinct outputs is disheartening at least.

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-20T21:54:41Z

Hello Tom,

>> Here is a POC which defines an internal interface for a PRNG, and use it
>> within pgbench, with several possible implementations which default to
>> rand48.
>
> I seriously dislike this patch.  pgbench's random support is quite
> overengineered already IMO, and this proposes to add a whole batch of
> new code and new APIs to fix a very small bug.

My intention is rather to discuss postgres' PRNG, in passing. Full success 
on this point:-)

>> I must admit that I have a grudge against standard rand48:
>
> I think this is nonsense, particularly the claim that anything in PG
> cares about the lowest-order bits of random doubles.  I'm aware that
> there are applications where that does matter, but people aren't doing
> high-precision weather simulations in pgbench.

Sure. My point is not that it is an actual issue for pgbench, but as the 
same PRNG is used more or less everywhere in postgres, I think that it 
should be a good one rather than a known bad one.

Eg, about double:

   \set i debug(random(1, POWER(2,49)) % 2)

Always return 1 because of the 48 bit precision, i.e. the output is never 
even.

   \set i debug(random(1, POWER(2,48)) % 2)

Return 0 1 0 1 0 1 0 1 0 1 0 1 0 1 ... because it is a LCG.

   \set i debug(random(1, POWER(2,48)) % 4)

Cycles over (3 2 1 0)*

   \set i debug(random(1, power(2, 47)) % 4)

Cycles over (0 0 1 1 2 2 3 3)*, and so on.

> BTW, did you look at the question of the range of zipfian?

Yep.

> I confirmed here that as used in the test case, it's generating a range 
> way smaller than the other ones: repeating the insertion snippet 1000x 
> produces stats like this: [...]

> I have no idea whether that indicates an actual bug, or just poor
> choice of parameter in the test's call.  But the very small number
> of distinct outputs is disheartening at least.

Zipf distribution is highly skewed, somehow close to an exponential. To 
reduce the decreasing probability the parameter must be closer to 1, eg 
1.05 or something. However as far as the test is concerned I do not see 
this as a significant issue. I was rather planning to submit a 
documentation improvement to provide more precise hints about how the 
distribution behaves depending on the parameter, and possibly reduce the 
parameter used in the test in passing, but I see this as not very urgent.

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-22T10:16:02Z

Hello Tom,

>> BTW, did you look at the question of the range of zipfian?
>
> Yep.
>
>> I confirmed here that as used in the test case, it's generating a range way 
>> smaller than the other ones: repeating the insertion snippet 1000x produces 
>> stats like this: [...]
>
>> I have no idea whether that indicates an actual bug, or just poor
>> choice of parameter in the test's call.  But the very small number
>> of distinct outputs is disheartening at least.
>
> Zipf distribution is highly skewed, somehow close to an exponential. To 
> reduce the decreasing probability the parameter must be closer to 1, eg 1.05 
> or something. However as far as the test is concerned I do not see this as a 
> significant issue. I was rather planning to submit a documentation 
> improvement to provide more precise hints about how the distribution behaves 
> depending on the parameter, and possibly reduce the parameter used in the 
> test in passing, but I see this as not very urgent.

Attached a documentation patch and a scripts to check the distribution 
(here for N = 10 & s = 2.5), the kind of thing I used when checking the 
initial patch:

   sh> psql < zipf_init.sql
   sh> pgbench -t 500000 -c 2 -M prepared -f zipf_test.sql -P 1
   -- close to 29000 tps on my laptop
   sh> psql < zipf_end.sql
  ┌────┬────────┬────────────────────┬────────────────────────┐
  │ i  │  cnt   │       ratio        │        expected        │
  ├────┼────────┼────────────────────┼────────────────────────┤
  │  1 │ 756371 │                  • │                      • │
  │  2 │ 133431 │ 5.6686302283577280 │ 5.65685424949238019521 │
  │  3 │  48661 │ 2.7420521567579787 │     2.7556759606310754 │
  │  4 │  23677 │ 2.0552012501583816 │     2.0528009571186693 │
  │  5 │  13534 │ 1.7494458401063987 │     1.7469281074217107 │
  │  6 │   8773 │ 1.5426877920893651 │     1.5774409656148784 │
  │  7 │   5709 │ 1.5366964442108951 │     1.4701680288054869 │
  │  8 │   4247 │ 1.3442429950553332 │     1.3963036312159316 │
  │  9 │   3147 │ 1.3495392437241818 │     1.3423980299088363 │
  │ 10 │   2450 │ 1.2844897959183673 │     1.3013488313450120 │
  └────┴────────┴────────────────────┴────────────────────────┘
   sh> psql < zipf_clean.sql

Given these results, I do not think that it is useful to change 
random_zipfian TAP test parameter from 2.5 to something else.

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-22T15:46:29Z

Fabien COELHO <coelho@cri.ensmp.fr> writes:
> Given these results, I do not think that it is useful to change 
> random_zipfian TAP test parameter from 2.5 to something else.

I'm not following this argument.  The test case is basically useless
for its intended purpose with that parameter, because it's highly
likely that the failure mode it's supposedly checking for will be
masked by the "random" function's tendency to spit out the same
value all the time.  We might as well drop zipfian from the test
altogether and save ourselves some buildfarm cycles.

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-22T16:19:11Z

Hello Tom,

>> Given these results, I do not think that it is useful to change
>> random_zipfian TAP test parameter from 2.5 to something else.
>
> I'm not following this argument.  The test case is basically useless
> for its intended purpose with that parameter, because it's highly
> likely that the failure mode it's supposedly checking for will be
> masked by the "random" function's tendency to spit out the same
> value all the time.

The first value is taken about 75% of the time for N=1000 and s=2.5, which 
means that a non deterministic implementation would succeed about 0.75² ~ 
56% of the time on that one. Then there is other lower probability random 
successes. ISTM that if a test fails every three run it would be detected, 
so the purpose of testing random_zipfian determinism is somehow served.

Also, the drawing procedure is less efficient when the parameter is close 
to 1 because it is more likely to loop, and there are other values tested, 
0.5 and 1.3 (note that the code has two methods, depending on whether the 
parameter is below or above 1), so I think that having something different 
is better.

If you want something more drastic, using 1.5 instead of 2.5 would reduce 
the probability of accidentaly passing the test by chance to about 20%, so 
it would fail 80% of the time.

> We might as well drop zipfian from the test altogether and save 
> ourselves some buildfarm cycles.

All 4 random functions are tested together on the same run, removing a 
particular one does not seem desirable to me.

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-22T16:44:57Z

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>>> Here is a POC which defines an internal interface for a PRNG, and use it
>>> within pgbench, with several possible implementations which default to
>>> rand48.

>> I seriously dislike this patch.  pgbench's random support is quite
>> overengineered already IMO, and this proposes to add a whole batch of
>> new code and new APIs to fix a very small bug.

> My intention is rather to discuss postgres' PRNG, in passing. Full success 
> on this point:-)

Our immediate problem is to fix a portability failure, which we need to
back-patch into at least one released branch, ergo conservatism is
warranted.  I had in mind something more like the attached.

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-22T17:12:29Z

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> I'm not following this argument.  The test case is basically useless
>> for its intended purpose with that parameter, because it's highly
>> likely that the failure mode it's supposedly checking for will be
>> masked by the "random" function's tendency to spit out the same
>> value all the time.

> The first value is taken about 75% of the time for N=1000 and s=2.5, which 
> means that a non deterministic implementation would succeed about 0.75² ~ 
> 56% of the time on that one.

Right, that's about what we've been seeing on OpenBSD.

> Also, the drawing procedure is less efficient when the parameter is close 
> to 1 because it is more likely to loop,

That might be something to fix, but I agree it's a reason not to go
overboard trying to flatten the test case's distribution right now.

> If you want something more drastic, using 1.5 instead of 2.5 would reduce 
> the probability of accidentaly passing the test by chance to about 20%, so 
> it would fail 80% of the time.

I think your math is off; 1.5 works quite well here.  I saw one failure
to produce distinct values in 20 attempts.  It's not demonstrably slower
than 2.5 either.  (1.1 is measurably slower; probably not by enough for
anyone to care, but 1.5 is good enough for me.)

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-22T19:07:05Z

>> The first value is taken about 75% of the time for N=1000 and s=2.5, which
>> means that a non deterministic implementation would succeed about 0.75² ~
>> 56% of the time on that one.
>
> Right, that's about what we've been seeing on OpenBSD.
>
>> Also, the drawing procedure is less efficient when the parameter is close
>> to 1 because it is more likely to loop,
>
> That might be something to fix, but I agree it's a reason not to go
> overboard trying to flatten the test case's distribution right now.

Probably you would have to invent a new method to draw a zipfian 
distribution for that, which would be nice.

>> If you want something more drastic, using 1.5 instead of 2.5 would reduce
>> the probability of accidentaly passing the test by chance to about 20%, so
>> it would fail 80% of the time.
>
> I think your math is off;

Argh. Although I confirm my computation, ISTM that with 1.5 the first 
value as 39% chance of getting out so collision on 15% of cases, second 
value 14% so collision on 2%, ... total cumulated probability about 18%.

> 1.5 works quite well here.  I saw one failure to produce distinct values 
> in 20 attempts.

For 3 failure expected, that is possible.

> It's not demonstrably slower than 2.5 either.  (1.1 is measurably 
> slower; probably not by enough for anyone to care, but 1.5 is good 
> enough for me.)

Good if it fails quick enough for you.

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-22T19:58:50Z

>> It's not demonstrably slower than 2.5 either.  (1.1 is measurably slower; 
>> probably not by enough for anyone to care, but 1.5 is good enough for me.)
>
> Good if it fails quick enough for you.

Attached a patch with the zipf doc update & the TAP test parameter change.

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Fabien COELHO <coelho@cri.ensmp.fr> — 2019-01-22T20:35:27Z

Hello Tom,

>>>> Here is a POC which defines an internal interface for a PRNG, and use it
>>>> within pgbench, with several possible implementations which default to
>>>> rand48.
>
>>> I seriously dislike this patch.  pgbench's random support is quite
>>> overengineered already IMO, and this proposes to add a whole batch of
>>> new code and new APIs to fix a very small bug.
>
>> My intention is rather to discuss postgres' PRNG, in passing. Full success
>> on this point:-)
>
> Our immediate problem is to fix a portability failure, which we need to
> back-patch into at least one released branch, ergo conservatism is
> warranted.

Sure, the patch I sent is definitely not for backpatching, it is for 
discussion.

>  I had in mind something more like the attached.

Yep.

I'm not too happy that it mixes API levels, and about the int/double/int 
path.

Attached an updated version which relies on pg_jrand48 instead. Also, as 
the pseudo-random state is fully controlled, seeded test results are 
deterministic so the expected value can be fully checked.

I did a few sanity tests which were all ok.

I think that this version is appropriate for backpatching. I also think 
that it would be appropriate to consider having a better PRNG to replace 
rand48 in a future release.

-- 
Fabien.

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Tom Lane <tgl@sss.pgh.pa.us> — 2019-01-24T16:35:25Z

Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> I had in mind something more like the attached.

> Yep.
> I'm not too happy that it mixes API levels, and about the int/double/int 
> path.
> Attached an updated version which relies on pg_jrand48 instead.

Hm, I'm not sure that's really an improvement, but I pushed it like that
(and the other change along with it).

> Also, as 
> the pseudo-random state is fully controlled, seeded test results are 
> deterministic so the expected value can be fully checked.

I found that the "expected value" was different in v11 than HEAD,
which surprised me.  It looks like the reason is that HEAD sets up
more/different RandomStates from the same seed than v11 did.  Not
sure if it's a good thing for this behavior to change across versions.

			regards, tom lane

Re: PSA: we lack TAP test coverage on NetBSD and OpenBSD

Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-01-24T16:45:49Z

On 2019-Jan-24, Tom Lane wrote:

> > Also, as 
> > the pseudo-random state is fully controlled, seeded test results are 
> > deterministic so the expected value can be fully checked.
> 
> I found that the "expected value" was different in v11 than HEAD,
> which surprised me.  It looks like the reason is that HEAD sets up
> more/different RandomStates from the same seed than v11 did.  Not
> sure if it's a good thing for this behavior to change across versions.

The rationale behind this was that some internal uses of random numbers
messed up the determinism of user-invoked random functions; 409231919443
commit message says

    While at it, use separate random state for thread administratrivia such
    as deciding which script to run, how long to delay for throttling, or
    whether to log a message when sampling; this not only makes these tasks
    independent of each other, but makes the actual thread run
    deterministic.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services