Regression tests versus the buildfarm environment

On Wed, Aug 11, 2010 at 06:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:

I am not sure if there's anything very good we can do about the
problem of pg_regress misidentifying the postmaster it's managed to
connect to. A real solution would probably be much more trouble than
it's worth, anyway. However, it does seem like we ought to be able to
do something about two buildfarm critters defaulting to the same choice
of port number. The buildfarm infrastructure goes to great lengths to
pick nonconflicting port numbers for the "installed" postmasters it
runs; but we're ignoring all that effort and just using a hardwired
port number for "make check". This is dumb.

pg_regress does have a --port argument that can be used to override
that default. I don't know whether the buildfarm script calls
pg_regress directly or does "make check". If the latter, we'd need to
twiddle the Makefiles to allow a port number to get passed in. But
this seems well worthwhile to me.

Comments?

We just put in the possibility to name the client connections. Would it be
interesting to be able to name the server installation itself?

On 08/11/2010 12:42 AM, Tom Lane wrote:

There's an interesting buildfarm failure here:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=polecat&amp;dt=2010-08-10%2023:46:10
It appears to me that this was caused by the concurrent run of another
buildfarm animal on the same physical machine, namely:
http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=colugos&amp;dt=2010-08-11%2000:02:58
Both animals are trying to test HEAD, which means that pg_regress
defaults to the same postmaster port number in both builds:

if (temp_install&& !port_specified_by_user)

/*
* To reduce chances of interference with parallel installations, use
* a port number starting in the private range (49152-65535)
* calculated from the version number.
*/
port = 0xC000 | (PG_VERSION_NUM& 0x3FFF);

We observe colugos successfully starting on that port:

============== starting postmaster ==============
running on port 57332 with pid 47019
============== creating database "regression" ==============
CREATE DATABASE
ALTER DATABASE
... etc etc ...

polecat comes along what must be only moments later, and tries to use
the same port for its temp install:

============== starting postmaster ==============
running on port 57332 with pid 47022
============== creating database "regression" ==============
ERROR: duplicate key value violates unique constraint "pg_database_datname_index"
DETAIL: Key (datname)=(regression) already exists.
command failed: "/usr/local/src/build-farm-3.2/builds/HEAD/pgsql.15278/src/test/regress/./tmp_check/install//usr/local/src/build-farm-3.2/builds/HEAD/inst/bin/psql" -X -c "CREATE DATABASE \"regression\" TEMPLATE=template0 ENCODING='SQL_ASCII' LC_COLLATE='C' LC_CTYPE='C'" "postgres"
pg_ctl: PID file "/usr/local/src/build-farm-3.2/builds/HEAD/pgsql.15278/src/test/regress/./tmp_check/data/postmaster.pid" does not exist
Is server running?

pg_regress: could not stop postmaster: exit code was 256

Now the postmaster log shows that the second postmaster correctly
recognized that the port number was already in use, so it bailed out:

================== pgsql.15278/src/test/regress/log/postmaster.log ===================
[4c61f2d2.b7ae:1] FATAL: lock file "/tmp/.s.PGSQL.57332.lock" already exists
[4c61f2d2.b7ae:2] HINT: Is another postmaster (PID 47019) using socket file "/tmp/.s.PGSQL.57332"?

However, pg_regress failed to have a clue about what had happened,
and bulled ahead trying to run the regression tests (against the
postmaster started by the other pg_regress instance). A look at the
code shows that it is merely trying to run psql, and if psql reports
that it can connect to the specified port, then pg_regress thinks the
postmaster started OK. Of course, psql was really reporting that it
could connect to the other instance's postmaster.

I've seen similar multiple-postmaster-interference symptoms before in
the buildfarm, but never really understood the cause.

I am not sure if there's anything very good we can do about the
problem of pg_regress misidentifying the postmaster it's managed to
connect to. A real solution would probably be much more trouble than
it's worth, anyway. However, it does seem like we ought to be able to
do something about two buildfarm critters defaulting to the same choice
of port number. The buildfarm infrastructure goes to great lengths to
pick nonconflicting port numbers for the "installed" postmasters it
runs; but we're ignoring all that effort and just using a hardwired
port number for "make check". This is dumb.

pg_regress does have a --port argument that can be used to override
that default. I don't know whether the buildfarm script calls
pg_regress directly or does "make check". If the latter, we'd need to
twiddle the Makefiles to allow a port number to get passed in. But
this seems well worthwhile to me.

Comments?

The buildfarm calls "make check".

Why not just add the configured port (DEF_PGPORT) into the calculation
of the port to run on?

cheers

andrew

Andrew Dunstan <andrew@dunslane.net> writes:

On 08/11/2010 12:42 AM, Tom Lane wrote:

... However, it does seem like we ought to be able to
do something about two buildfarm critters defaulting to the same choice
of port number.

Why not just add the configured port (DEF_PGPORT) into the calculation
of the port to run on?

No, that would be just about the worst possible choice. It'd be
guaranteed to fail in the standard scenario that you are running
"make check" before updating an existing installation.

I think what we want to do here is arrange for the buildfarm script to
select the same port that it's going to use later for an "installed"
postmaster, but it has to go via a different path than DEF_PGPORT.

The first thought that comes to mind is to adjust the makefiles
like this:

ifdef REGRESSION_TEST_PORT
... add --port $(REGRESSION_TEST_PORT) to pg_regress flags ...
endif

and then the buildfarm script could use

make REGRESSION_TEST_PORT=nnn check

But I'm not sure what the cleanest way is if we have to pass that
down from the top-level makefile. Make doesn't pass down variables
automatically does it?

Another possibility is to allow a regression test port number to
be configured via configure; though that seems like a slightly
larger change than I'd want to push into the back branches.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:

A look at the code shows that it is merely trying to run psql, and
if psql reports that it can connect to the specified port, then
pg_regress thinks the postmaster started OK. Of course, psql was
really reporting that it could connect to the other instance's
postmaster.

Clearly picking unique ports for `make check` is the ultimate
solution, but I'm curious whether this would have been caught sooner
with less effort if the pg_ctl TODO titled "Have the postmaster
write a random number to a file on startup that pg_ctl checks
against the contents of a pg_ping response on its initial connection
(without login)" had been implemented.

http://archives.postgresql.org/pgsql-bugs/2009-10/msg00110.php

It sounds like it's related; but was curious to confirm.

-Kevin

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

A look at the code shows that it is merely trying to run psql, and
if psql reports that it can connect to the specified port, then
pg_regress thinks the postmaster started OK. Of course, psql was
really reporting that it could connect to the other instance's
postmaster.

Clearly picking unique ports for `make check` is the ultimate
solution, but I'm curious whether this would have been caught sooner
with less effort if the pg_ctl TODO titled "Have the postmaster
write a random number to a file on startup that pg_ctl checks
against the contents of a pg_ping response on its initial connection
(without login)" had been implemented.

It would certainly make the failure more transparent. As I mentioned,
there are previous buildfarm failures that look like they might be
caused by a similar conflict, but it's seldom possible to be sure.
A cross-check like that would be much safer.

BTW, I don't know why anyone would think that "a random number" would
offer any advantage here. I'd use the postmaster PID, which is
guaranteed to be unique across the space that you're worried about.
In fact, you could implement this off the existing postmaster.pid,
no need for any new file. What's lacking is the pg_ping protocol.

regards, tom lane

On 08/11/2010 09:43 AM, Tom Lane wrote:

Andrew Dunstan<andrew@dunslane.net> writes:

On 08/11/2010 12:42 AM, Tom Lane wrote:

... However, it does seem like we ought to be able to
do something about two buildfarm critters defaulting to the same choice
of port number.

Why not just add the configured port (DEF_PGPORT) into the calculation
of the port to run on?

No, that would be just about the worst possible choice. It'd be
guaranteed to fail in the standard scenario that you are running
"make check" before updating an existing installation.

One of us is missing something. I didn't say to run the checks using the
configured port. I had in mind something like:

port = 0xC000 | ((PG_VERSION_NUM + DEF_PGPORT) & 0x3FFF);

cheers

andrew

Tom Lane <tgl@sss.pgh.pa.us> wrote:

BTW, I don't know why anyone would think that "a random number"
would offer any advantage here. I'd use the postmaster PID, which
is guaranteed to be unique across the space that you're worried
about.

Well, in the post I cited, it was you who argued that the PID was a
bad choice, suggested a random number, and stated "That would have a
substantially lower collision probability than PID, if the number
generation process were well designed; and it wouldn't risk exposing
anything sensitive in the ping response."

-Kevin

Andrew Dunstan <andrew@dunslane.net> writes:

On 08/11/2010 09:43 AM, Tom Lane wrote:

Andrew Dunstan<andrew@dunslane.net> writes:

Why not just add the configured port (DEF_PGPORT) into the calculation
of the port to run on?

No, that would be just about the worst possible choice. It'd be
guaranteed to fail in the standard scenario that you are running
"make check" before updating an existing installation.

One of us is missing something. I didn't say to run the checks using the
configured port. I had in mind something like:

port = 0xC000 | ((PG_VERSION_NUM + DEF_PGPORT) & 0x3FFF);

Oh, I see, modify the DEF_PGPORT don't just use it as-is. OK, except
that I think something like the above is still pretty risky for the
buildfarm, because you would still have conflicts for assorted
combinations of version numbers and branch_port settings.

How about just this:

port = 0xC000 | (DEF_PGPORT & 0x3FFF);

If anyone was actually using a DEF_PGPORT above 0xC000, this would mean
that they couldn't run "make check" on the same machine as their running
installation (at least not without adjusting pg_regress's port choice,
which I still think we need to tweak the makefiles to make easier).
But for ordinary buildfarm usage, this would be guaranteed not to
conflict as long as you'd chosen nonconflicting branch_ports for all
your branches and animals.

Or we could do something like

port = 0xC000 ^ (DEF_PGPORT & 0x7FFF);

which is absolutely guaranteed not to conflict with DEF_PGPORT, at the
cost of possibly shifting into the 32K-48K port number range if you
had set DEF_PGPORT above 48K.

regards, tom lane

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:

Tom Lane <tgl@sss.pgh.pa.us> wrote:

BTW, I don't know why anyone would think that "a random number"
would offer any advantage here. I'd use the postmaster PID, which
is guaranteed to be unique across the space that you're worried
about.

Well, in the post I cited, it was you who argued that the PID was a
bad choice, suggested a random number, and stated "That would have a
substantially lower collision probability than PID, if the number
generation process were well designed; and it wouldn't risk exposing
anything sensitive in the ping response."

Hmm. I don't remember why we'd think that the postmaster PID was
sensitive information ... but if you take that as true, then yeah
it couldn't be included in a pg_ping response.

regards, tom lane

On Wed, Aug 11, 2010 at 10:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew@dunslane.net> writes:

On 08/11/2010 09:43 AM, Tom Lane wrote:

Andrew Dunstan<andrew@dunslane.net>  writes:

Why not just add the configured port (DEF_PGPORT) into the calculation
of the port to run on?

No, that would be just about the worst possible choice.  It'd be
guaranteed to fail in the standard scenario that you are running
"make check" before updating an existing installation.

One of us is missing something. I didn't say to run the checks using the
configured port. I had in mind something like:

     port = 0xC000 | ((PG_VERSION_NUM + DEF_PGPORT) &  0x3FFF);

Oh, I see, modify the DEF_PGPORT don't just use it as-is.  OK, except
that I think something like the above is still pretty risky for the
buildfarm, because you would still have conflicts for assorted
combinations of version numbers and branch_port settings.

How about just this:

    port = 0xC000 | (DEF_PGPORT & 0x3FFF);

If anyone was actually using a DEF_PGPORT above 0xC000, this would mean
that they couldn't run "make check" on the same machine as their running
installation (at least not without adjusting pg_regress's port choice,
which I still think we need to tweak the makefiles to make easier).
But for ordinary buildfarm usage, this would be guaranteed not to
conflict as long as you'd chosen nonconflicting branch_ports for all
your branches and animals.

Or we could do something like

    port = 0xC000 ^ (DEF_PGPORT & 0x7FFF);

which is absolutely guaranteed not to conflict with DEF_PGPORT, at the
cost of possibly shifting into the 32K-48K port number range if you
had set DEF_PGPORT above 48K.

I like XOR a lot better than OR.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Vik Reykja <vikreykja@gmail.com> writes:

We just put in the possibility to name the client connections. Would it be
interesting to be able to name the server installation itself?

Wouldn't do anything for this problem --- it would just introduce
something else the buildfarm would have to worry about uniqueness of.

regards, tom lane

Robert Haas <robertmhaas@gmail.com> writes:

On Wed, Aug 11, 2010 at 10:15 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Or we could do something like

� � port = 0xC000 ^ (DEF_PGPORT & 0x7FFF);

which is absolutely guaranteed not to conflict with DEF_PGPORT, at the
cost of possibly shifting into the 32K-48K port number range if you
had set DEF_PGPORT above 48K.

I like XOR a lot better than OR.

Yeah, on reflection that seems better. Barring objection I will see
about making this happen in all live branches.

regards, tom lane

On 08/11/2010 10:23 AM, Robert Haas wrote:

Or we could do something like

port = 0xC000 ^ (DEF_PGPORT& 0x7FFF);

which is absolutely guaranteed not to conflict with DEF_PGPORT, at the
cost of possibly shifting into the 32K-48K port number range if you
had set DEF_PGPORT above 48K.

I like XOR a lot better than OR.

For years we told people to make sure they picked 4 digit port numbers
for the buildfarm, and while I removed that note recently it can be put
back. So I don't think there's much danger - let's got with XOR.

cheers

andrew

On ons, 2010-08-11 at 10:15 -0400, Tom Lane wrote:

One of us is missing something. I didn't say to run the checks using

the

configured port. I had in mind something like:

port = 0xC000 | ((PG_VERSION_NUM + DEF_PGPORT) & 0x3FFF);

Oh, I see, modify the DEF_PGPORT don't just use it as-is. OK, except
that I think something like the above is still pretty risky for the
buildfarm, because you would still have conflicts for assorted
combinations of version numbers and branch_port settings.

How about just this:

port = 0xC000 | (DEF_PGPORT & 0x3FFF);

The version number was put in there intentionally, for developers who
work on multiple branches at once. That's the whole reason this code
exists. Please don't remove it.

On ons, 2010-08-11 at 09:55 -0400, Tom Lane wrote:

BTW, I don't know why anyone would think that "a random number" would
offer any advantage here. I'd use the postmaster PID, which is
guaranteed to be unique across the space that you're worried about.
In fact, you could implement this off the existing postmaster.pid,
no need for any new file. What's lacking is the pg_ping protocol.

Why not just compare pg_backend_pid() with postmaster.pid?

Peter Eisentraut <peter_e@gmx.net> writes:

On ons, 2010-08-11 at 10:15 -0400, Tom Lane wrote:

How about just this:
port = 0xC000 | (DEF_PGPORT & 0x3FFF);

The version number was put in there intentionally, for developers who
work on multiple branches at once. That's the whole reason this code
exists. Please don't remove it.

I work on multiple branches all day every day. This wouldn't hinder
me in the slightest, because I use a different DEF_PGPORT for each
branch. If you don't, it's hard to see how you manage to deal with
multiple branches on one machine ... do you not ever actually install
them? Even if you don't, changing this would only mean that you
couldn't safely run "make check" concurrently in multiple branches.

regards, tom lane

Peter Eisentraut <peter_e@gmx.net> writes:

On ons, 2010-08-11 at 09:55 -0400, Tom Lane wrote:

BTW, I don't know why anyone would think that "a random number" would
offer any advantage here. I'd use the postmaster PID, which is
guaranteed to be unique across the space that you're worried about.
In fact, you could implement this off the existing postmaster.pid,
no need for any new file. What's lacking is the pg_ping protocol.

Why not just compare pg_backend_pid() with postmaster.pid?

How's that help? pg_backend_pid isn't going to return the postmaster's
PID ... maybe we could add a new function that does return the
postmaster's PID, though.

regards, tom lane

Peter Eisentraut <peter_e@gmx.net> wrote:

On ons, 2010-08-11 at 09:55 -0400, Tom Lane wrote:

BTW, I don't know why anyone would think that "a random number"
would offer any advantage here. I'd use the postmaster PID,
which is guaranteed to be unique across the space that you're
worried about. In fact, you could implement this off the
existing postmaster.pid, no need for any new file. What's
lacking is the pg_ping protocol.

Why not just compare pg_backend_pid() with postmaster.pid?

See the prior discussion in the archives. We started with that and
found problems, to which Tom suggested a random number as the best
solution. Let's at least start any further discussion informed by
what's gone before; if there was a flaw in the reasoning, please
point that out.

-Kevin

On ons, 2010-08-11 at 11:48 -0400, Tom Lane wrote:

How's that help? pg_backend_pid isn't going to return the
postmaster's
PID ... maybe we could add a new function that does return the
postmaster's PID, though.

Hmm, is there a portable way to find the parent PID of some other
process, given its PID?