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