Subject: Re: [COMMITTERS] pgsql: Use asynchronous connect API in
 libpqwalreceiver
To: Tom Lane <tgl@sss.pgh.pa.us>,
 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
References: <E1cjo1g-0000mr-ST@gemulon.postgresql.org>
 <19403.1488586564@sss.pgh.pa.us>
 <34d98fbf-9195-04d2-b85a-634d6952ffdf@2ndquadrant.com>
 <19815.1488600693@sss.pgh.pa.us>
Cc: pgsql-hackers@postgreSQL.org, Andrew Dunstan <andrew@dunslane.net>
From: Petr Jelinek <petr.jelinek@2ndquadrant.com>
Message-ID: <4144256b-dc4f-892c-a720-71f6e7b9ac87@2ndquadrant.com>
Date: Sat, 4 Mar 2017 07:45:58 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.7.0
MIME-Version: 1.0
In-Reply-To: <19815.1488600693@sss.pgh.pa.us>
Content-Type: multipart/mixed;
 boundary="------------BC0F38B40D7F28D62E9C6EDA"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

This is a multi-part message in MIME format.
--------------BC0F38B40D7F28D62E9C6EDA
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 8bit

On 04/03/17 05:11, Tom Lane wrote:
> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
>> On 3/3/17 19:16, Tom Lane wrote:
>>> Peter Eisentraut <peter_e@gmx.net> writes:
>>>> Use asynchronous connect API in libpqwalreceiver
> 
>>> Buildfarm member bowerbird has been failing in the pg_rewind test since
>>> this patch went in.  It looks like it's failing to complete connections
>>> from the standby; which suggests that something platform-specific is
>>> missing from this commit, but I dunno what.
> 
>> Hmm, I wonder how widely tested the async connection API is on Windows
>> at all.  I only see bowerbird and jacana running bin-check on Windows.
> 
> Yeah, I was wondering if this is just exposing a pre-existing bug.
> However, the "normal" path operates by repeatedly invoking PQconnectPoll
> (cf. connectDBComplete) so it's not immediately obvious how such a bug
> would've escaped detection.
> 

I can see one difference though (I didn't see this code before) and that
is, the connectDBComplete starts with waiting for socket to become
writable and only then calls PQconnectPoll, while my patch starts with
PQconnectPoll call. And I see following comment in connectDBstart
> 	/*
> 	 * The code for processing CONNECTION_NEEDED state is in PQconnectPoll(),
> 	 * so that it can easily be re-executed if needed again during the
> 	 * asynchronous startup process.  However, we must run it once here,
> 	 * because callers expect a success return from this routine to mean that
> 	 * we are in PGRES_POLLING_WRITING connection state.
> 	 */

So I guess I implemented it wrong in a subtle way that breaks on windows.

If that's the case, the attached should fix it, but I have no way of
testing it on windows, I can only say that it still works on my machine
so at least it hopefully does not make things worse.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services

--------------BC0F38B40D7F28D62E9C6EDA
Content-Type: text/plain; charset=UTF-8;
 name="0001-Reorder-the-asynchronous-libpq-calls-for-replication.patch"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
 filename*0="0001-Reorder-the-asynchronous-libpq-calls-for-replication.pa";
 filename*1="tch"

=46rom 7478c898954099c79c0a34743d2292f740120009 Mon Sep 17 00:00:00 2001
From: Petr Jelinek <pjmodos@pjmodos.net>
Date: Sat, 4 Mar 2017 07:43:17 +0100
Subject: [PATCH] Reorder the asynchronous libpq calls for replication
 connection

---
 .../replication/libpqwalreceiver/libpqwalreceiver.c   | 19 +++++++++++----=
----
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/=
src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 048d2aa..b24eb3f 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -113,7 +113,7 @@ libpqrcv_connect(const char *conninfo, bool logical, co=
nst char *appname,
 				 char **err)
 {
 	WalReceiverConn *conn;
-	PostgresPollingStatusType status;
+	PostgresPollingStatusType status =3D PGRES_POLLING_WRITING;
 	const char *keys[5];
 	const char *vals[5];
 	int			i =3D 0;
@@ -155,12 +155,14 @@ libpqrcv_connect(const char *conninfo, bool logical, =
const char *appname,
 		return NULL;
 	}
=20
-	/* Poll connection. */
-	do
+	/*
+	 * Poll connection until we have OK or FAILED status.
+	 *
+	 * Note the initial state after PQconnectStartParams is
+	 * PGRES_POLLING_WRITING.
+	 */
+	while (status !=3D PGRES_POLLING_OK && status !=3D PGRES_POLLING_FAILED)
 	{
-		/* Determine current state of the connection. */
-		status =3D PQconnectPoll(conn->streamConn);
-
 		/* Sleep a bit if waiting for socket. */
 		if (status =3D=3D PGRES_POLLING_READING ||
 			status =3D=3D PGRES_POLLING_WRITING)
@@ -189,8 +191,9 @@ libpqrcv_connect(const char *conninfo, bool logical, co=
nst char *appname,
 				CHECK_FOR_INTERRUPTS();
 		}
=20
-		/* Otherwise loop until we have OK or FAILED status. */
-	} while (status !=3D PGRES_POLLING_OK && status !=3D PGRES_POLLING_FAILED=
);
+		/* Determine new state of the connection. */
+		status =3D PQconnectPoll(conn->streamConn);
+	}
=20
 	if (PQstatus(conn->streamConn) !=3D CONNECTION_OK)
 	{
--=20
2.7.4


--------------BC0F38B40D7F28D62E9C6EDA
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

--------------BC0F38B40D7F28D62E9C6EDA--