Hi Kuroda-san,
Thank you for updating the patch!
I rethought the pqSocketPoll part. Current interpretation of
arguments seems a little bit confusing because a specific pattern
of arguments has a different meaning. What do you think about
introducing a new argument like `int forConnCheck`? This seems
straightforward and readable.
I think it may be better, so fixed.
But now we must consider another thing - will we support combination
of conncheck
and {read|write} or timeout? Especially about timeout, if someone
calls pqSocketPoll()
with forConnCheck = 1 and end_time = -1, the process may be stuck
because it waits
till socket peer closed connection.
Currently the forConnCheck can be specified with other request, but
timeout must be zero.
Yes, we need to consider these.
I'm wondering whether we need a special care for the combination
of event and timeout. Surely, if forConnCheck is set and end_time = -1,
pqSocketPoll blocks until the connection close. However, the
behavior matches the meaning of the arguments and does not seem
confusing (also not an error state). Do we need to restrict this
kind of usage in the pqSocketPoll side? I think like the following
might be fine.
```
if (!forRead && !forWrite)
{
if (!(forConnCheck && PQconnCheckable()))
return 0;
}
```
While making the patch, I come up with idea that
postgres_fdw_verify_connection_states*
returns NULL if PQconnCheck() cannot work on this platform. This can be
done by
adding PQconnCheckable(). It may be reasonable because the checking is
not really
done on this environment. How do you think?
I agree with you.
Followings are comments for v33. Please check.
0001:
1. the comment of PQconnCheck
+/*
+ * Check whether the socket peer closed connection or not.
+ *
Check whether the socket peer closed 'the' connection or not?
2. the comment of pqSocketCheck
- * or both. Returns >0 if one or more conditions are met, 0 if it
timed
- * out, -1 if an error occurred.
+ * or both. Moreover, this function can check the health of socket on
some
+ * limited platforms if end_time is 0.
the health of socket -> the health of the connection?
if end_time is 0 -> if forConnCehck is specified?
3. the comment of pqSocketPoll
- * If neither forRead nor forWrite are set, immediately return a
timeout
- * condition (without waiting). Return >0 if condition is met, 0
- * if a timeout occurred, -1 if an error or interrupt occurred.
+ * Moreover, this function can check the health of socket on some
limited
+ * platforms if end_time is 0.
the health of socket -> the health of the connection?
if end_time is 0 -> if forConnCehck is specified?
4. the code of pqSocketPoll
+#if defined(POLLRDHUP)
+ if (forConnCheck)
+ input_fd.events |= POLLRDHUP;
+#endif
I think it is better to use PQconnCheckable() to remove the macro.
5. the code of pqSocketCheck and the definition of pqSocketPoll
- result = pqSocketPoll(conn->sock, forRead, forWrite, end_time);
+ result = pqSocketPoll(conn->sock, forRead, forWrite, forConnCheck,
end_time);
-pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time)
+pqSocketPoll(int sock, int forRead, int forWrite, int forConnCheck,
time_t end_time)
Should these be divided into two lines?
6. the comment of verify_cached_connections
+ * This function emits warnings if a disconnection is found, and this
returns
+ * false only when the lastly verified server seems to be disconnected.
It seems better to write the case where this function returns
true.
7. the comment of postgres_fdw_verify_connection_states
+ * This function emits a warning if a disconnection is found. This
returns
+ * false only when the verified server seems to be disconnected, and
reutrns
+ * NULL if the connection check had not been done.
It seems better to write the case where this function returns
true.
8. the code of
+ (errcode(ERRCODE_CONNECTION_FAILURE),
+ errmsg("%s", str.data),
+ errdetail("Socket close is detected."),
+ errhint("Plsease check the health of
server.")));
Is it better to use "Connection close is detected" rather than
"Socket close is detected"?
9. the document of postgres_fdw
The document of postgres_fdw_verify_connection_states_* is a little
bit old. Could you update it?
regards,
--
Katsuragi Yuta
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
