[patch]socket_timeout in interfaces/libpq
Hello all.
First, I'd like to appreciate with all your reviewing and discussion in the last CommitFest[1]/messages/by-id/EDA4195584F5064680D8130B1CA91C45367328@G01JPEXMBYT04.
I don't think that the rest one of my proposals has been rejected completely, so I want to restart discussion.
It is a timeout parameter in interfaces/libpq.
Consider some situations where some happening occurred a server and it became significant busy. e.g., what I and Tsunakawa-san have illustrated[2]/messages/by-id/EDA4195584F5064680D8130B1CA91C45367328@G01JPEXMBYT04[3]/messages/by-id/0A3221C70F24FB45833433255569204D1FBC7561@G01JPEXMBYT05.
These server's bad condition(i.e., non-functional server) could cause clients' infinite waiting because it is not always possible for current timeout parameters in backend side to fire.
Under such server's bad condition, control should be passed to the client after a certain period of time, and just a timeout disconnection corresponds to it.
Also, in such situations the backend parameters may not work, so we need to implement the timeout parameters on the client side.
It is preferable to implement this parameter in PQwait() where clients can wait endlessly.
However this can do unintended timeout when setting socket_timeout < statement_timeout(etc. some other timeout parameters).
Thus documentation warns this.
FYI, a similarly parameter socketTimeout is in pgJDBC[4]https://jdbc.postgresql.org/documentation/head/connect.html#connection-parameters.
Do you have any thoughts?
P.S.
Fabien-san,
I'll build another thread and let's discussion there about \c's taking care of connection parameters you have pointed out!
[1]: /messages/by-id/EDA4195584F5064680D8130B1CA91C45367328@G01JPEXMBYT04
[2]: /messages/by-id/EDA4195584F5064680D8130B1CA91C45367328@G01JPEXMBYT04
[3]: /messages/by-id/0A3221C70F24FB45833433255569204D1FBC7561@G01JPEXMBYT05
[4]: https://jdbc.postgresql.org/documentation/head/connect.html#connection-parameters
Best regards,
---------------------
Ryohei Nagaura
Attachments:
socket_timeout.patchapplication/octet-stream; name=socket_timeout.patchDownload
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 7f01fcc148..d39b044d3b 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1263,6 +1263,30 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
</listitem>
</varlistentry>
+ <varlistentry id="socket_timeout" xreflabel="socket_timeout">
+ <term><literal>socket_timeout</literal></term>
+ <listitem>
+ <para>
+ Maximum wait in seconds (write as a decimal integer, e.g. 10) for
+ socket read/write operation before closing the connection. A value
+ of zero (the default) turns this off, which means wait indefinitely.
+ The minimum allowed timeout is 2 seconds, so a value of 1 is
+ interpreted as 2.
+ </para>
+ <note>
+ <para>
+ Although this can be used as a stopgap timeout measure, it is
+ recommended to set a value higher than the other timeout
+ parameters (<literal>statement_timeout</literal>,
+ <literal>TCP_KEEP_ALIVES</literal>,
+ <literal>TCP_USER_TIMEOUT</literal>) because setting a smaller
+ value will make the other configured timeout parameters meaningless
+ and can cause undesirable disconnection.
+ </para>
+ </note>
+ </listitem>
+ </varlistentry>
+
<varlistentry id="libpq-connect-tty" xreflabel="tty">
<term><literal>tty</literal></term>
<listitem>
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index c800d7921e..7e6dccc676 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -214,6 +214,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
"Connect-timeout", "", 10, /* strlen(INT32_MAX) == 10 */
offsetof(struct pg_conn, connect_timeout)},
+ {"socket_timeout", NULL, NULL, NULL,
+ "Socket-timeout", "", 10, /* strlen(INT32_MAX) == 10 */
+ offsetof(struct pg_conn, pgsocket_timeout)},
+
{"dbname", "PGDATABASE", NULL, NULL,
"Database-Name", "", 20,
offsetof(struct pg_conn, dbName)},
@@ -423,6 +427,8 @@ static char *passwordFromFile(const char *hostname, const char *port, const char
const char *username, const char *pgpassfile);
static void pgpassfileWarning(PGconn *conn);
static void default_threadlock(int acquire);
+static bool parse_int_param(const char *value, int *result, PGconn *conn,
+ const char *context);
/* global variable because fe-auth.c needs to access it */
@@ -1289,6 +1295,24 @@ connectOptions2(PGconn *conn)
goto oom_error;
}
+ if (conn->pgsocket_timeout)
+ {
+ if (!parse_int_param(conn->pgsocket_timeout,
+ &conn->socket_timeout, conn, "socket_timeout"))
+ {
+ conn->status = CONNECTION_BAD;
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("invalid integer value for socket_timeout\n"));
+ return false;
+ }
+ /*
+ * Rounding could cause communication to fail;
+ * insist on at least two seconds.
+ */
+ if(conn->socket_timeout > 0 && conn->socket_timeout < 2)
+ conn->socket_timeout = 2;
+ }
+
/*
* Validate target_session_attrs option.
*/
@@ -3903,6 +3927,8 @@ freePGconn(PGconn *conn)
free(conn->pgtty);
if (conn->connect_timeout)
free(conn->connect_timeout);
+ if (conn->pgsocket_timeout)
+ free(conn->pgsocket_timeout);
if (conn->pgtcp_user_timeout)
free(conn->pgtcp_user_timeout);
if (conn->pgoptions)
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 2d44845ccb..4a18b726ad 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -1020,6 +1020,32 @@ pqFlush(PGconn *conn)
int
pqWait(int forRead, int forWrite, PGconn *conn)
{
+ int result;
+
+ if (conn->socket_timeout > 0)
+ {
+ time_t finish_time;
+
+ finish_time = time(NULL) + conn->socket_timeout;
+ result = pqSocketCheck(conn, forRead, forWrite, finish_time);
+
+ if (result < 0)
+ return EOF;
+
+ if (result == 0)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ libpq_gettext("timeout expired\n"));
+ conn->status = CONNECTION_BAD;
+ pqsecure_close(conn);
+ closesocket(conn->sock);
+ conn->sock = PGINVALID_SOCKET;
+ return EOF;
+ }
+
+ return 0;
+ }
+
return pqWaitTimed(forRead, forWrite, conn, (time_t) -1);
}
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index c0b8e3f8ce..a329fbff8b 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -336,6 +336,8 @@ struct pg_conn
char *pgtty; /* tty on which the backend messages is
* displayed (OBSOLETE, NOT USED) */
char *connect_timeout; /* connection timeout (numeric string) */
+ char *pgsocket_timeout; /* socket timeout (numeric string) */
+ int socket_timeout; /* socket timeout (numeric)*/
char *pgtcp_user_timeout; /* tcp user timeout (numeric string) */
char *client_encoding_initial; /* encoding to use */
char *pgoptions; /* options to start the backend with */
On Wed, Jun 26, 2019 at 04:13:36AM +0000, nagaura.ryohei@fujitsu.com wrote:
I don't think that the rest one of my proposals has been rejected
completely, so I want to restart discussion.
I recall on the matter that there was consensus that nobody really
liked this option because it enforced a cancellation on the
connection.
--
Michael
Hi, Michael-san.
From: Michael Paquier <michael@paquier.xyz>
On Wed, Jun 26, 2019 at 04:13:36AM +0000, nagaura.ryohei@fujitsu.com wrote:I don't think that the rest one of my proposals has been rejected
completely, so I want to restart discussion.I recall on the matter that there was consensus that nobody really liked this option
because it enforced a cancellation on the connection.
It seems that you did not think so at that time.
# Please refer to [1]/messages/by-id/20190406065428.GA2145@paquier.xyz Best regards, --------------------- Ryohei Nagaura
I don't think all the reviewers are completely negative.
I think some couldn't judge because lack of what kind of problem I was going to solve and the way to solve it, so I restarted to describe them in this time.
[1]: /messages/by-id/20190406065428.GA2145@paquier.xyz Best regards, --------------------- Ryohei Nagaura
Best regards,
---------------------
Ryohei Nagaura
On Wed, Jun 26, 2019 at 11:56:28AM +0000, nagaura.ryohei@fujitsu.com wrote:
It seems that you did not think so at that time.
# Please refer to [1]I don't think all the reviewers are completely negative.
I recall having a negative impression on the patch when first looking
at it, and still have the same impression when looking at the last
version. Just with a quick look, assuming that you can bypass all
cleanup operations normally taken by pqDropConnection() through a
hijacking of pqWait() is not fine as this routine explicitely assumes
to *never* have a timeout for its wait.
--
Michael
On Tue, Sep 10, 2019 at 03:38:21PM +0900, Michael Paquier wrote:
I recall having a negative impression on the patch when first looking
at it, and still have the same impression when looking at the last
version. Just with a quick look, assuming that you can bypass all
cleanup operations normally taken by pqDropConnection() through a
hijacking of pqWait() is not fine as this routine explicitely assumes
to *never* have a timeout for its wait.
By the way, Fabien, you are marked as a reviewer of this patch since
the end of June. Are you planning to review it?
--
Michael
By the way, Fabien, you are marked as a reviewer of this patch since the
end of June. Are you planning to review it?
Not this round.
--
Fabien.
On Wed, Sep 11, 2019 at 04:25:07PM +0200, Fabien COELHO wrote:
Not this round.
You have registered yourself as a reviewer of this patch since the end
of June. Could you please avoid that? Sometimes people skips patches
when they see someone already registered to review it.
The patch applies cleanly so I am movingit to next CF.
(FWIW, I still have the same impression as upthread, looking again at
the patch, but let's see if there are other opinions.)
--
Michael
Michaï¿œl,
Not this round.
You have registered yourself as a reviewer of this patch since the end
of June. Could you please avoid that? Sometimes people skips patches
when they see someone already registered to review it.
Yep. ISTM that I did a few reviews on early versions of the patch, which
was really a set of 3 patches.
The patch applies cleanly so I am movingit to next CF.
(FWIW, I still have the same impression as upthread, looking again at
the patch, but let's see if there are other opinions.)
AFAICR, I was partly dissuated to pursue reviews by your comment that
somehow the feature had no clear consensus, so I thought that the patch
was implicitely rejected.
Although I work for free, I try to avoid working for nothing:-)
It is still unclear from your above comment whether the patch would ever
get committed, so this does not motivate spending time on it.
--
Fabien.
Hi, Michael-san.
Sorry, I have missed your e-mail...
From: Michael Paquier <michael@paquier.xyz>
On Wed, Jun 26, 2019 at 11:56:28AM +0000, nagaura.ryohei@fujitsu.com wrote:It seems that you did not think so at that time.
# Please refer to [1]I don't think all the reviewers are completely negative.
I recall having a negative impression on the patch when first looking at it, and still
have the same impression when looking at the last version. Just with a quick
look, assuming that you can bypass all cleanup operations normally taken by
pqDropConnection() through a hijacking of pqWait() is not fine as this routine
explicitely assumes to *never* have a timeout for its wait.
I couldn't understand what you meant.
Do you say that we shouldn't change pqWait() behavior?
Or should I modify my patch to use pqDropConnection()?
Best regards,
---------------------
Ryohei Nagaura
On 11/29/19 12:22 AM, nagaura.ryohei@fujitsu.com wrote:
From: Michael Paquier <michael@paquier.xyz>
On Wed, Jun 26, 2019 at 11:56:28AM +0000, nagaura.ryohei@fujitsu.com wrote:It seems that you did not think so at that time.
# Please refer to [1]I don't think all the reviewers are completely negative.
I recall having a negative impression on the patch when first looking at it, and still
have the same impression when looking at the last version. Just with a quick
look, assuming that you can bypass all cleanup operations normally taken by
pqDropConnection() through a hijacking of pqWait() is not fine as this routine
explicitely assumes to *never* have a timeout for its wait.I couldn't understand what you meant.
Do you say that we shouldn't change pqWait() behavior?
Or should I modify my patch to use pqDropConnection()?
This patch no longer applies: http://cfbot.cputube.org/patch_27_2175.log
CF entry has been updated to Waiting on Author.
More importantly it looks like there is still no consensus on this
patch, which is an uncommitted part of a previous patch [1]/messages/by-id/raw/20190406065428.GA2145@paquier.xyz.
Unless somebody chimes in I'll mark this Returned with Feedback at the
end of the CF.
Regards,
--
-David
david@pgmasters.net
[1]: /messages/by-id/raw/20190406065428.GA2145@paquier.xyz
/messages/by-id/raw/20190406065428.GA2145@paquier.xyz
On 3/24/20 10:58 AM, David Steele wrote:
On 11/29/19 12:22 AM, nagaura.ryohei@fujitsu.com wrote:
I couldn't understand what you meant.
Do you say that we shouldn't change pqWait() behavior?
Or should I modify my patch to use pqDropConnection()?This patch no longer applies: http://cfbot.cputube.org/patch_27_2175.log
CF entry has been updated to Waiting on Author.
More importantly it looks like there is still no consensus on this
patch, which is an uncommitted part of a previous patch [1].Unless somebody chimes in I'll mark this Returned with Feedback at the
end of the CF.
Marked Returned with Feedback.
Regards,
--
-David
david@pgmasters.net