Hello, Fabien-san.
> From: Fabien COELHO [mailto:coelho@cri.ensmp.fr]
> I have further remarks after Kirk-san extensive review on these patches.
Yes, I'm welcome.
Thank you very much for your review.
> * About TCP interface v18.
> For homogeneity with the surrounding cases, ISTM that "TCP_user_timeout"
> should be ""TCP-user-timeout".
Oops, it would be better. Thanks.
Note that I changed to "TCP-User-Timeout" not "TCP-user-timeout."
Also, I deleted this
+ /* TCP USER TIMEOUT */
> * About TCP backend v19 patch
> I still disagree with "on other systems, it must be zero.": I do not see why
> "it must be zero", I think that the parameter should simply be ignored if it
> does not apply or is not implemented on a platform?
No, please see the below with "src/backend/libpq/pqcomm.c".
I made consistent with Keepalives documentation and implementation.
i,e,. if the setting value is non-zero on a system that does not support
these parameters, an "not supported" error is output.
Therefore, "on other systems, it must be zero" in doc.
Also, the error message "not supported" is needed because if an error is not output when you set that parameter on such
asystem, it seems to be available in spite of not available.
In my patch, this situation corresponds to line 114.
> If there are consistency constraint with other timeout parameters, probably the
> documentation should mention it?
As I said above.
> * About socket_timeout v12 patch, I'm not sure there is a consensus.
> I still think that there should be an attempt at cancelling before
> severing.
When some accidents hits the server, infinite wait of the user may occur.
This feature is a way to get around it.
It is not intended to reduce server load.
Since sending a cancellation request will cause the user to wait[1], I do not think that it follows the intention.
Do you think that you still need to send a cancellation request?
> Robert pointed out that it is not a timeout wrt the query, but this is not
> clearly explained in the documentation nor the comments.
Sorry, I couldn't recognize your intension.
Is it that you think the description "this is not a query timeout" is needed?
> The doc says that
> it is the time for socket read/write operations, but it is somehow the
> time between messages, some of which may not be linked to read/write
> operations. I feel that the documentation is not very precise about what
> it really does.
What socket_timeout really does is a timeout by poll().
poll() monitors a socket file descriptor,
so the document "the time for socket read/write operations" is correct.
> ISTM that the implementation could make the cancelling as low as 1 second
> because of rounding. This could be said somewhere, maybe in the doc,
> surely in a comment.
It is said in doc. Right?
+ The minimum allowed timeout is 2 seconds, so a value of 1 is
+ interpreted as 2.
Or "because of rounding" is needed?
> I still think that this parameter should be preservered on psql's
> reconnections when explicitely set to non zero.
Would you please tell me your vision ?
Do you want to inherit all parameters with one option?
Or one to one?
[1] https://www.postgresql.org/message-id/0A3221C70F24FB45833433255569204D1FBC7FD1%40G01JPEXMBYT05
Best regards,
---------------------
Ryohei Nagaura