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

On 2023-01-27 15:57, Hayato Kuroda (Fujitsu) wrote:
> I found cfbot failure, PSA fixed version.
> Sorry for noise.
> 
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED

Hi Kuroda-san,

Thank you for updating the patch! Sorry for the late reply.

0001:
+       while (result < 0 && errno == EINTR);
+
+       if (result < 0)
+                       return -1;

this `return -1` is not indented properly.


0002:
+    <term><function>postgres_fdw_verify_connection_states(server_name 
text) returns boolean</function></term>
...
+      extension to the <symbol>poll</symbol> system call, including 
Linux. This
+      returns <literal>true</literal> if checked connections are still 
valid,
+      or the checking is not supported on this platform. 
<literal>false</literal>
+      is returned if the local session seems to be disconnected from 
other
+      servers. Example usage of the function:

Here, 'still valid' seems a little bit confusing because this 'valid' is 
not
the same as postgres_fdw_get_connections's 'valid' [1].

Should 'still valid' be 'existing connection is not closed by the remote 
peer'?
But this description does not cover all the cases where this function 
returns true...
I think one choice is to write all the cases like 'returns true if any 
of the
following condition is satisfied. 1) existing connection is not closed 
by the
remote peer 2) there is no connection for specified server yet 3) the 
checking
is not supported...'. If my understanding is not correct, please point 
it out.

BTW, is it reasonable to return true if ConnectionHash is not 
initialized or
there is no ConnCacheEntry for specified remote server? What do you 
think
about returning NULL in that case?


0003:
I think it is better that the test covers all the new functions.
How about adding a test for postgres_fdw_verify_connection_states_all?


+-- ===================================================================
+-- test for postgres_fdw_verify_foreign_servers function
+-- ===================================================================

Writing all the functions' name like 'test for 
postgres_fdw_verify_connection_states
and postgres_fdw_can_verify_connection_states' looks straightforward.
What do you think about this?


0004:
Sorry, I have not read 0004. I will read.


[1]: 
https://github.com/postgres/postgres/blob/master/doc/src/sgml/postgres-fdw.sgml#L764-L765

regards,

-- 
Katsuragi Yuta
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION