Re: [Proposal] Add foreign-server health checks infrastructure

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