Re: Re: In-core regression tests for replication, cascading, archiving, PITR, etc.

On 10/07/2015 10:29 AM, Michael Paquier wrote:
> On Wed, Oct 7, 2015 at 4:16 PM, Amir Rohan <amir(dot)rohan(at)zoho(dot)com> wrote:
>> On 10/07/2015 09:27 AM, Michael Paquier wrote:
>>> On Wed, Oct 7, 2015 at 7:51 AM, Michael Paquier
>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>> On Wed, Oct 7, 2015 at 7:43 AM, Michael Paquier
>>>> <michael(dot)paquier(at)gmail(dot)com> wrote:
>>>>> On Wed, Oct 7, 2015 at 5:58 AM, Robert Haas wrote:
>>>>>> On Sat, Oct 3, 2015 at 7:38 AM, Michael Paquier wrote:
>>>>>> It seems that these days 'make check' creates a directory in /tmp
>>>>>> called /tmp/pg_regress-RANDOMSTUFF. Listening on TCP ports is
>>>>>> disabled, and the socket goes inside this directory with a name like
>>>>>> .s.PGSQL.PORT. You can connect with psql -h
>>>>>> /tmp/pg_regress-RANDOMSTUFF -p PORT, but not over TCP. This basically
>>>>>> removes the risk of TCP port number collisions, as well as the risk of
>>>>>> your temporary instance being hijacked by a malicious user on the same
>>>>>> machine.
>>>>>
>>>>> Right, that's for example /var/folders/ on OSX, and this is defined
>>>>> once per test run via $tempdir_short. PGHOST is set to that as well.
>>>>
>>>> Er, mistake here. That's actually once per standard_initdb, except
>>>> that all the tests I have included in my patch run it just once to
>>>> create a master node. It seems that it would be wiser to set one
>>>> socket dir per node then, remove the port assignment stuff, and use
>>>> tempdir_short as a key to define a node as well as in the connection
>>>> string to this node. I'll update the patch later today...
>>>
>>> So, my conclusion regarding multiple calls of make_master is that we
>>> should not allow to do it. On Unix/Linux we could have a separate unix
>>> socket directory for each node, but not on Windows where
>>> listen_addresses is set to look after 127.0.0.1. On Unix/Linux, PGHOST
>>> is set by the master node to a tempdir once and for all. Hence, to
>>> make the code more consistent, I think that we should keep the port
>>> lookup machinery here. An updated patch is attached.
>>>
>> If parallel tests are supported, get_free_port is still racy even
>> with last_port_found because it's:
>> 1) process-local.
>> 2) even if it were shared, there's the race window between the
>> available-check and the listen() I mentioned upthread.
>>
>> If parallel tests are explicitly disallowed, a comment to that
>> effect (and a note on things known to break) might help someone
>> down the road.
>
> Actually, no, port lookup will not map and parallel tests would work
> fine thinking more about it, each set of tests uses its own PGHOST to
> a private unix socket directory so even if multiple tests use the same
> port number they won't interact with each other because they connect
> to different socket paths. MinGW is a problem though, and an existing
> one in the perl test scripts, I recall that it can use make -j and
> that's on Windows where PGHOST is mapping to 127.0.0.1 only.
>

ah, the portnum is actually a real tcp port only on windows, and
the race is limited to that case as you say. Note that in the
tcp case, using psql to check is wrong:
$ nc -l 8001 # listen on 8001
$ psql -X -h lo -p 8001 postgres < /dev/null psql: could not connect to
server: Connection refused
Is the server running on host "lo" (127.0.0.1) and accepting
TCP/IP connections on port 8001?

The port isn't free, but psql is really only checking if pg is there
and reports that the port is available. That's a fairly mild issue, though.

>> Also, the removal of poll_query_until from pg_rewind looks suspiciously
>> like a copy-paste gone bad. Pardon if I'm missing something.
>
> Perhaps. Do you have a suggestion regarding that? It seems to me that
> this is more useful in TestLib.pm as-is.
>

My mistake, the patch only shows some internal function being deleted
but RewindTest.pm (obviously) imports TestLib. You're right, TestLib is
a better place for it.