RE: Timeout parameters

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
Date: 2019-03-29 08:34:38
Message-ID: D09B13F772D2274BB348A310EE3027C648A159@g01jpexmbkw24
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi Nagaura-san,

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.

A. TCP_USER_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.

[DOCS]
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.

[CODE]
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
in src/interfaces/libpq/fe-connect.c,

- 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.

Regards,
Kirk Jamison

In response to

Browse pgsql-hackers by date

  From Date Subject
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