PSA: we lack TAP test coverage on NetBSD and OpenBSD

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.

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

=?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

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

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

=?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

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

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

=?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

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

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

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

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&amp;dt=2019-01-17%2022%3A57%3A14

should I leave it as it is for now?

/Mikael

=?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

=?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

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

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

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

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