[Patch] PQconnectPoll() is blocked if target_session_attrs is read-write

Started by Matsumura, Ryoover 6 years ago5 messages
#1Matsumura, Ryo
matsumura.ryo@jp.fujitsu.com
1 attachment(s)

Hi

# I rewrote my previous mail.

PQconnectPoll() is used as method for asynchronous using externally or internally.
If a caller confirms a socket ready for writing or reading that is
requested by return value of previous PQconnectPoll(), next PQconnectPoll()
must not be blocked. But if the caller specifies target_session_attrs to
'read-write', PQconnectPoll() may be blocked.

Detail:
If target_session_attrs is set to read-write, PQconnectPoll() calls
PQsendQuery("SHOW transaction_read_only") althogh previous return value was
PGRES_POLLING_READING not WRITING.
In result, PQsendQuery() may be blocked in pqsecure_raw_write().

I attach a patch.

Regards
Ryo Matsumura

Attachments:

libpq_state_change_bugfix.ver1.0.patchapplication/octet-stream; name=libpq_state_change_bugfix.ver1.0.patchDownload
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d70cf1f..cf48f74 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2186,6 +2186,7 @@ PQconnectPoll(PGconn *conn)
 			/* Special cases: proceed without waiting. */
 		case CONNECTION_SSL_STARTUP:
 		case CONNECTION_NEEDED:
+		case CONNECTION_CHECK_WRITABLE_NEEDED:
 		case CONNECTION_CHECK_WRITABLE:
 		case CONNECTION_CONSUME:
 		case CONNECTION_GSS_STARTUP:
@@ -3444,26 +3445,6 @@ keep_going:						/* We will come back to here until there is
 					conn->target_session_attrs != NULL &&
 					strcmp(conn->target_session_attrs, "read-write") == 0)
 				{
-					/*
-					 * Save existing error messages across the PQsendQuery
-					 * attempt.  This is necessary because PQsendQuery is
-					 * going to reset conn->errorMessage, so we would lose
-					 * error messages related to previous hosts we have tried
-					 * and failed to connect to.
-					 */
-					if (!saveErrorMessage(conn, &savedMessage))
-						goto error_return;
-
-					conn->status = CONNECTION_OK;
-					if (!PQsendQuery(conn,
-									 "SHOW transaction_read_only"))
-					{
-						restoreErrorMessage(conn, &savedMessage);
-						goto error_return;
-					}
-					conn->status = CONNECTION_CHECK_WRITABLE;
-					restoreErrorMessage(conn, &savedMessage);
-					return PGRES_POLLING_READING;
 				}
 
 				/* We can release the address list now. */
@@ -3514,19 +3495,8 @@ keep_going:						/* We will come back to here until there is
 				conn->target_session_attrs != NULL &&
 				strcmp(conn->target_session_attrs, "read-write") == 0)
 			{
-				if (!saveErrorMessage(conn, &savedMessage))
-					goto error_return;
-
-				conn->status = CONNECTION_OK;
-				if (!PQsendQuery(conn,
-								 "SHOW transaction_read_only"))
-				{
-					restoreErrorMessage(conn, &savedMessage);
-					goto error_return;
-				}
-				conn->status = CONNECTION_CHECK_WRITABLE;
-				restoreErrorMessage(conn, &savedMessage);
-				return PGRES_POLLING_READING;
+				conn->status = CONNECTION_CHECK_WRITABLE_NEEDED;
+				return PGRES_POLLING_WRITING;
 			}
 
 			/* We can release the address list now. */
@@ -3566,6 +3536,31 @@ keep_going:						/* We will come back to here until there is
 				conn->status = CONNECTION_OK;
 				return PGRES_POLLING_OK;
 			}
+
+		case CONNECTION_CHECK_WRITABLE_NEEDED:
+			{
+				/*
+				 * Save existing error messages across the PQsendQuery
+				 * attempt.  This is necessary because PQsendQuery is
+				 * going to reset conn->errorMessage, so we would lose
+				 * error messages related to previous hosts we have tried
+				 * and failed to connect to.
+				 */
+				if (!saveErrorMessage(conn, &savedMessage))
+					goto error_return;
+
+				conn->status = CONNECTION_OK;
+				if (!PQsendQuery(conn,
+								 "SHOW transaction_read_only"))
+				{
+					restoreErrorMessage(conn, &savedMessage);
+					goto error_return;
+				}
+				conn->status = CONNECTION_CHECK_WRITABLE;
+				restoreErrorMessage(conn, &savedMessage);
+				return PGRES_POLLING_READING;
+			}
+
 		case CONNECTION_CHECK_WRITABLE:
 			{
 				const char *displayed_host;
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 26198fc..817f5da 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -67,7 +67,9 @@ typedef enum
 								 * connection. */
 	CONNECTION_CONSUME,			/* Wait for any pending message and consume
 								 * them. */
-	CONNECTION_GSS_STARTUP		/* Negotiating GSSAPI. */
+	CONNECTION_GSS_STARTUP,		/* Negotiating GSSAPI. */
+	CONNECTION_CHECK_WRITABLE_NEEDED	/* Need to check if we could make a
+										 * writable connection. */
 } ConnStatusType;
 
 typedef enum
#2Kyotaro Horiguchi
horikyota.ntt@gmail.com
In reply to: Matsumura, Ryo (#1)
Re: [Patch] PQconnectPoll() is blocked if target_session_attrs is read-write

Hello.

At Mon, 22 Jul 2019 02:28:22 +0000, "Matsumura, Ryo" <matsumura.ryo@jp.fujitsu.com> wrote in <03040DFF97E6E54E88D3BFEE5F5480F74AC15BBD@G01JPEXMBYT04>

Hi

# I rewrote my previous mail.

PQconnectPoll() is used as method for asynchronous using externally or internally.
If a caller confirms a socket ready for writing or reading that is
requested by return value of previous PQconnectPoll(), next PQconnectPoll()
must not be blocked. But if the caller specifies target_session_attrs to
'read-write', PQconnectPoll() may be blocked.

Detail:
If target_session_attrs is set to read-write, PQconnectPoll() calls
PQsendQuery("SHOW transaction_read_only") althogh previous return value was
PGRES_POLLING_READING not WRITING.
In result, PQsendQuery() may be blocked in pqsecure_raw_write().

I attach a patch.

Regards
Ryo Matsumura

First, this patch looks broken.

patched> if (conn->sversion >= 70400 &&
patched> conn->target_session_attrs != NULL &&
patched> strcmp(conn->target_session_attrs, "read-write") == 0)
patched> {
patched> }

Perhaps you did cut-n-paste instead of copy-n-paste.

I'm not sure such a small write just after reading can block, but
doing that makes things tidy.

You also need to update the corresponding documentation.

regards.

--
Kyotaro Horiguchi
NTT Open Source Software Center

#3Matsumura, Ryo
matsumura.ryo@jp.fujitsu.com
In reply to: Kyotaro Horiguchi (#2)
1 attachment(s)
RE: [Patch] PQconnectPoll() is blocked if target_session_attrs is read-write

Kyotaro-san

Thank you for your review.

First, this patch looks broken.

I took a serious mistake.

You also need to update the corresponding documentation.

I attach a new patch that includes updating of document.

Regards
Ryo Matasumura

Attachments:

libpq_state_change_bugfix.ver1.1.patchapplication/octet-stream; name=libpq_state_change_bugfix.ver1.1.patchDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 7f01fcc..b24183e 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -429,6 +429,15 @@ PostgresPollingStatusType PQconnectPoll(PGconn *conn);
          </listitem>
         </varlistentry>
 
+        <varlistentry id="libpq-connection-check-writable-needed">
+         <term><symbol>CONNECTION_CHECK_WRITABLE_NEEDED</symbol></term>
+         <listitem>
+          <para>
+           Waiting to send a query that checks if connection is able to handle write transactions.
+          </para>
+         </listitem>
+        </varlistentry>
+
         <varlistentry id="libpq-connection-check-writable">
          <term><symbol>CONNECTION_CHECK_WRITABLE</symbol></term>
          <listitem>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index d70cf1f..c995bd9 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2186,6 +2186,7 @@ PQconnectPoll(PGconn *conn)
 			/* Special cases: proceed without waiting. */
 		case CONNECTION_SSL_STARTUP:
 		case CONNECTION_NEEDED:
+		case CONNECTION_CHECK_WRITABLE_NEEDED:
 		case CONNECTION_CHECK_WRITABLE:
 		case CONNECTION_CONSUME:
 		case CONNECTION_GSS_STARTUP:
@@ -3444,26 +3445,8 @@ keep_going:						/* We will come back to here until there is
 					conn->target_session_attrs != NULL &&
 					strcmp(conn->target_session_attrs, "read-write") == 0)
 				{
-					/*
-					 * Save existing error messages across the PQsendQuery
-					 * attempt.  This is necessary because PQsendQuery is
-					 * going to reset conn->errorMessage, so we would lose
-					 * error messages related to previous hosts we have tried
-					 * and failed to connect to.
-					 */
-					if (!saveErrorMessage(conn, &savedMessage))
-						goto error_return;
-
-					conn->status = CONNECTION_OK;
-					if (!PQsendQuery(conn,
-									 "SHOW transaction_read_only"))
-					{
-						restoreErrorMessage(conn, &savedMessage);
-						goto error_return;
-					}
-					conn->status = CONNECTION_CHECK_WRITABLE;
-					restoreErrorMessage(conn, &savedMessage);
-					return PGRES_POLLING_READING;
+					conn->status = CONNECTION_CHECK_WRITABLE_NEEDED;
+					return PGRES_POLLING_WRITING;
 				}
 
 				/* We can release the address list now. */
@@ -3514,19 +3497,8 @@ keep_going:						/* We will come back to here until there is
 				conn->target_session_attrs != NULL &&
 				strcmp(conn->target_session_attrs, "read-write") == 0)
 			{
-				if (!saveErrorMessage(conn, &savedMessage))
-					goto error_return;
-
-				conn->status = CONNECTION_OK;
-				if (!PQsendQuery(conn,
-								 "SHOW transaction_read_only"))
-				{
-					restoreErrorMessage(conn, &savedMessage);
-					goto error_return;
-				}
-				conn->status = CONNECTION_CHECK_WRITABLE;
-				restoreErrorMessage(conn, &savedMessage);
-				return PGRES_POLLING_READING;
+				conn->status = CONNECTION_CHECK_WRITABLE_NEEDED;
+				return PGRES_POLLING_WRITING;
 			}
 
 			/* We can release the address list now. */
@@ -3566,6 +3538,31 @@ keep_going:						/* We will come back to here until there is
 				conn->status = CONNECTION_OK;
 				return PGRES_POLLING_OK;
 			}
+
+		case CONNECTION_CHECK_WRITABLE_NEEDED:
+			{
+				/*
+				 * Save existing error messages across the PQsendQuery
+				 * attempt.  This is necessary because PQsendQuery is
+				 * going to reset conn->errorMessage, so we would lose
+				 * error messages related to previous hosts we have tried
+				 * and failed to connect to.
+				 */
+				if (!saveErrorMessage(conn, &savedMessage))
+					goto error_return;
+
+				conn->status = CONNECTION_OK;
+				if (!PQsendQuery(conn,
+								 "SHOW transaction_read_only"))
+				{
+					restoreErrorMessage(conn, &savedMessage);
+					goto error_return;
+				}
+				conn->status = CONNECTION_CHECK_WRITABLE;
+				restoreErrorMessage(conn, &savedMessage);
+				return PGRES_POLLING_READING;
+			}
+
 		case CONNECTION_CHECK_WRITABLE:
 			{
 				const char *displayed_host;
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index 26198fc..817f5da 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -67,7 +67,9 @@ typedef enum
 								 * connection. */
 	CONNECTION_CONSUME,			/* Wait for any pending message and consume
 								 * them. */
-	CONNECTION_GSS_STARTUP		/* Negotiating GSSAPI. */
+	CONNECTION_GSS_STARTUP,		/* Negotiating GSSAPI. */
+	CONNECTION_CHECK_WRITABLE_NEEDED	/* Need to check if we could make a
+										 * writable connection. */
 } ConnStatusType;
 
 typedef enum
#4Tsunakawa, Takayuki
tsunakawa.takay@jp.fujitsu.com
In reply to: Matsumura, Ryo (#1)
RE: [Patch] PQconnectPoll() is blocked if target_session_attrs is read-write

From: Matsumura, Ryo [mailto:matsumura.ryo@jp.fujitsu.com]

Detail:
If target_session_attrs is set to read-write, PQconnectPoll() calls
PQsendQuery("SHOW transaction_read_only") althogh previous return value
was PGRES_POLLING_READING not WRITING.

The current code probably assumes that PQsendQuery() to send "SHOW transaction_read_only" shouldn't block, because the message is small enough to fit in the socket send buffer. Likewise, the code in CONNECTION_AWAITING_RESPONSE case sends authentication data using pg_fe_sendauth() without checking for the write-ready status. OTOH, the code in CONNECTION_MADE case waits for write-ready status in advance before sending the startup packet. That's because the startup packet could get large enough to cause pqPacketSend() to block.

So, I don't think the fix is necessary.

In result, PQsendQuery() may be blocked in pqsecure_raw_write().

FWIW, if PQsendQuery() blocked during connection establishment, I think it should block in poll() called from .... from pqWait(), because the libpq's socket is set non-blocking.

Regards
Takayuki Tsunakawa

#5Matsumura, Ryo
matsumura.ryo@jp.fujitsu.com
In reply to: Tsunakawa, Takayuki (#4)
RE: [Patch] PQconnectPoll() is blocked if target_session_attrs is read-write

Tsunakawa-san

Thank you for your comment.
I understand the sense. I don't require an explicit rule.

Regards
Ryo Matsumura