|From:||"Jamison, Kirk" <k(dot)jamison(at)jp(dot)fujitsu(dot)com>|
|To:||"Nagaura, Ryohei" <nagaura(dot)ryohei(at)jp(dot)fujitsu(dot)com>|
|Cc:||'Kyotaro HORIGUCHI' <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>, "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>, "coelho(at)cri(dot)ensmp(dot)fr" <coelho(at)cri(dot)ensmp(dot)fr>, "robertmhaas(at)gmail(dot)com" <robertmhaas(at)gmail(dot)com>, "MikalaiKeida(at)ibagroup(dot)eu" <MikalaiKeida(at)ibagroup(dot)eu>, "AYahorau(at)ibagroup(dot)eu" <AYahorau(at)ibagroup(dot)eu>, "michael(at)paquier(dot)xyz" <michael(at)paquier(dot)xyz>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>|
|Subject:||RE: Timeout parameters|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
Thank you for the updated patches.
It became a long thread now, but it's okay, you've done a good amount of work.
There are 3 patches in total: 2 for tcp_user_timeout parameter, 1 for socket_timeout.
Since I've been following the updates, I compiled a summary of addressed changes
you made from all the reviewers' comments for TCP_USER_TIMEOUT:
For both client and server: apply ok, build ok, compile ok.
I confirmed that docs were patterned from keepalives in both
config.sgml (backend) and libpq.sgml (interface).
- changed "forcefully" to "forcibly"
It seems OK now to me.
1. SERVER TCP_backend patch (v19)
- as per advice of Fabien, default postgres configuration file
has been modified (postgresql.conf.sample);
- as per advice of Horiguchi-san, I confirmed that the variable
is now grouped along with keepalives* under the TCP timeout settings.
- confirmed the removal of "errno != ENOPROTOOPT" in backend's pq_settcpusertimeout()
- confirmed the hook from show_tcp_user_timeout to pq_gettcpusertimeout(),
based from the comment of Horiguchi-san to follow the tcp_keepalive* options' behavior.
- as per advice of Tsunakawa-san, GUC_NOT_IN_SAMPLE has been removed
and the default value is changed to 0.
- confirmed the modification of pq_settcpusertimeout():
a. to follow keepalives' pq_set* behavior as well
b. correct the error of sizeof(timeout)) << 0 to just a single '<';
'port->keepalives_count = count' to 'port->tcp_user_timeout = timeout'
2. CLIENT TCP_interface patch (v18)
- confirmed that tcp_user_timeout is placed after keepalives options
- confirmed the change / integration for the comment below in fe-connect.c:PQconnectPoll():
>I don't see a point in the added part having own "if
>(!IS_AF_UNIX" block separately from tcp_keepalive options.
- confirmed the removal of the following:
a. "errno != ENOPROTOOPT" in setTCPUserTimeout()
b. unused parameter: char *tcp_user_timeout;
c. error for non-supported systems
Overall, I tested the patch using the same procedure I did in the above email,
and it worked well. Given that you addressed the comments from all
the reviewers, I've judged these 2 patches as committable now.
B. socket_timeout parameter
> FYI, I summarized a use case of this parameter.
> The connection is built successfully.
> Suppose that the server is hit by some kind of accident(e,g,. I or Tsunakawa-san suggested).
> At this time, there is no guarantee that the server OS can pass control to the postmaster.
> Therefore, it is considered natural way to disconnect the connection from the client side.
> The place of implement of disconnection is pqWait() because it is where users' infinite wait occurs.
apply, build, compile --> OK
Doc seems OK now to me.
The patch looks good to me as you addressed the code comments from Tsunakawa-san.
Although it's still the committer who will have the final say.
That said, I am now marking this one "Ready for Committer".
If others have new comments by weekend, feel free to change the status.
|Next Message||Konstantin Knizhnik||2019-03-29 08:42:55||Re: Multitenancy optimization|
|Previous Message||Peter Eisentraut||2019-03-29 08:25:23||Re: Syntax diagrams in user documentation|