RE: Timeout parameters

From: "Nagaura, Ryohei" <nagaura(dot)ryohei(at)jp(dot)fujitsu(dot)com>
To: 'Kyotaro HORIGUCHI' <horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp>
Cc: "coelho(at)cri(dot)ensmp(dot)fr" <coelho(at)cri(dot)ensmp(dot)fr>, "Jamison, Kirk" <k(dot)jamison(at)jp(dot)fujitsu(dot)com>, "Tsunakawa, Takayuki" <tsunakawa(dot)takay(at)jp(dot)fujitsu(dot)com>, "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-28 08:36:10
Message-ID: EDA4195584F5064680D8130B1CA91C4540F159@G01JPEXMBYT04
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hello, Horiguchi-san.

Thank you for review.

> In TCP_backend patch:
> I think this is not mentioning backend. Why don't you copy'n paste then
> modify the description of tcp_keepalives_idle? Perhaps it needs a similar
> caveat related to Windows.

> +static const char*
> +show_tcp_user_timeout(void)
> The reason for, say, tcp_keepalive_idle uses the hook is that it doesn't
> show the GUC variable, but the actual value read by getsockopt. This is
> just showing the GUC value. I think this should behave the same way with
> tcp_keepalive* options. If not, we should have an explanation of the
> reason there.
Oh, Thank you for teaching.
I add a function pq_gettcpusertimeout() same as keepalives*.

> In TCP_interface patch:
> I would suggest the same thing as the backend part. Copy'n-paste-modify
> keepalives_idle would be better.
Same as backend-side, I made keepalives documentation reference.
I refered keepalives* documentation and modified my doc.

> I suppose that the reason ENOPROTOOPT is excluded from error condition
> is that the system call may fail with that errno on older kernels, but
> I don't think that that justifies ignoring the failure.
Thank you for your point!
Removed in both patches.

> I don't see a point in the added part having own "if (!IS_AF_UNIX" block
> separately from tcp_keepalive options.
Sorry, my coding was bad...
I integrated it with coding about keepalives.

> + /* TCP USER TIMEOUT */
> + {"tcp_user_timeout", NULL, NULL, NULL,
> + "TCP_user_timeout", "", 10, /* strlen(INT32_MAX) ==
> 10 */
> + offsetof(struct pg_conn, pgtcp_user_timeout)},
> +
>
> Similary, why isn't this placed just after tcp_keepalives options?
Moved!

> + char *tcp_user_timeout; /* tcp user timeout */
> The latter doesn't seem to be used.
This is also my bad coding...
Removed!

Best regards,
---------------------
Ryohei Nagaura

Attachment Content-Type Size
TCP_backend_v14.patch application/octet-stream 7.2 KB
TCP_interface_v13.patch application/octet-stream 3.7 KB
socket_timeout_v9.patch application/octet-stream 5.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Christoph Berg 2019-03-28 08:38:16 Re: Enable data checksums by default
Previous Message Sergei Kornilov 2019-03-28 08:07:59 Re: REINDEX CONCURRENTLY 2.0