[Patch] PQconnectPoll() is blocked if target_session_attrs is read-write
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
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
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
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