Hi Kuroda-san,
Thank you for updating the patch!
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.
IIUC the macro is needed. In FreeBSD, macOS and other platforms do not
have the
macro POLLRDHUP so they cannot compile. I checked by my CI and got
following error.
```
...
FAILED: src/interfaces/libpq/libpq.a.p/fe-misc.c.o
...
../src/interfaces/libpq/fe-misc.c:1149:22: error: use of undeclared
identifier 'POLLRDHUP'
input_fd.events |= POLLRDHUP;
````
It must be invisible from them.
Sorry, my mistake...
9. the document of postgres_fdw
The document of postgres_fdw_verify_connection_states_* is a little
bit old. Could you update it?
Updated. IIUC postgres_fdw_verify_connection_states returns
* true, if the connection is verified.
* false, if the connection seems to be disconnected.
* NULL, if this is not the supported platform or connection has not
been established.
And postgres_fdw_verify_connection_states_all returns
* true if all the connections are verified.
* false, if one of connections seems to be disconnected.
* NULL, if this is not the supported platform or this backend has
never established connections
I think 'backend has never established connections' is a little bit
strong.
I think the following cases also return NULL. The case where a
connection was established, however the connection is now closed
by the postgres_fdw_disconnect() or something. NULL is also returned if
the connection is invalidated. So, I think 'NULL, if no valid
connection is established or the function is not supported on
the platform.' is better. What do you think?
Followings are my comments for v34. Please check.
0001:
1. the document of pqConnCheck
+ <literal>0</literal> if the socket is valid, and returns
<literal>-1</literal>
+ if the connection has already been closed or an error has
occurred.
1.1 if the socket is valid -> returns 0 if the 'connection is not
closed'?
1.2 returns -1 if the connection has already been closed <- Let me ask
a question.
Isn't this a situation where we would like to check using this
function? Is 'error has occurred' insufficient?
2. the comment of pqSocketCheck
+ * means that the socket has not matched POLLRDHUP event and the socket
has
+ * still survived.
socket has still survived -> connection is not closed by the socket
peer?
3. the comment of pqSocketPoll
+ * returned and forConnCheck is requested, it means that the socket has
not
+ * matched POLLRDHUP event and the socket has still survived.
socket has still survived -> connection is not closed by the socket
peer?
0002:
4. the comment of verify_cached_connections
+ * This function emits warnings if a disconnection is found. This
return true
+ * if disconnections cannot be found, otherwise return false.
return ture -> return's' true
return false -> return's' false
5. the comment of postgres_fdw_verify_connection_states
+ * if the local session seems to be disconnected from other servers.
NULL is
+ * returned if a connection to the specified foreign server has not
been
+ * established yet, or this function is not available on this platform.
Considering the above discussion, 'NULL is returned if a valid
connection to the specified foreign server is not established or
this function...' seems better. What do you think?
6. the document of postgres_fdw_verify_connection_states
<literal>NULL</literal>
+ is returned if a connection to the specified foreign server has
not been
+ established yet, or this function is not available on this
platform
The same as comment no.5.
7. the document of postgres_fdw_verify_connection_states_all
<literal>NULL</literal>
+ is returned if the local session does not have connection caches,
or this
+ function is not available on this platform.
I think there is a case where a connection cache exists but valid
connections do not exist and NULL is returned (disconnection case).
Almost the same document as the postgres_fdw_verify_connection_states
case (comment no.5) seems better.
8. the document of postgres_fdw_can_verify_connection_states
+ This function checks whether
<function>postgres_fdw_verify_connection_states</function>
+ and <function>postgres_fdw_verify_connection_states</function>
work well
Should the latter (or former) postgres_fdw_verify_connection_states be
postgres_fdw_verify_connection_states_all?
regards,
--
Katsuragi Yuta
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
