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

On 2023-01-11 19:04, Hayato Kuroda (Fujitsu) wrote:
> Dear hackers,
> 
> I was not sure, but the cfbot could not be accepted the previous 
> version.
> I made the patch again from HEAD(5f6401) without any changes,
> so I did not count up the version number.
> 
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED

Hi,

Thanks for the patch!
I read the patch v24. These are my comments. Please check.

## v24-0001-Add-PQConncheck-and-PQCanConncheck-to-libpq.patch
+    <varlistentry id="libpq-PQCanConncheck">
+     
<term><function>PQCanConncheck</function><indexterm><primary>PQCanConncheck</primary></indexterm></term>
+     <listitem>
+      <para>
+       Returns the status of the socket.

Is this description right? I think this description is for
PQConncheck. Something like "Checks whether PQConncheck is
available on this platform." seems better.


+/* Check whether the postgres server is still alive or not */
+extern int PQConncheck(PGconn *conn);
+extern int PQCanConncheck(void);

Should the names of these functions be in the form of PQconnCheck?
Not PQConncheck (c.f. The section of fe-misc.c in libpq-fe.h).


## v24-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch
+PG_FUNCTION_INFO_V1(postgres_fdw_verify_connection_states);
+PG_FUNCTION_INFO_V1(postgres_fdw_verify_connection_states_all);
+PG_FUNCTION_INFO_V1(postgres_fdw_can_verify_connection_states);

This patch adds new functions to postgres_fdw for PostgreSQL 16.
So, I think it is necessary to update the version of postgres_fdw (v1.1 
to v1.2).


+    <term><function>postgres_fdw_verify_connection_states_all() returns 
boolean</function></term>
+    <listitem>
+     <para>
+      This function checks the status of remote connections that are 
established
+      by <filename>postgres_fdw</filename> from the local session to 
the foreign
+      servers. This check is performed by polling the socket and allows

It seems better to add a description that states this function
checks all the connections. Like the description of
postgres_fdw_disconnect_all(). For example, "This function
checks the status of 'all the' remote connections..."?

regards,

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