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

On Sat, Jan 21, 2023 at 4:03 AM Hayato Kuroda (Fujitsu) <
kuroda(dot)hayato(at)fujitsu(dot)com> wrote:

> Dear Katsuragi-san,
>
> Thank you for reviewing! PSA new patch set.
>
> > ## v24-0001-Add-PQConncheck-and-PQCanConncheck-to-libpq.patch
> > + <varlistentry id="libpq-PQCanConncheck">
> > +
> > <term><function>PQCanConncheck</function><indexterm><primary>PQCan
> > Conncheck</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.
>
> It might be copy-and-paste error. Thanks for reporting.
> According to other parts, the sentence should be started like "Returns
> ...".
> So I followed the style and did cosmetic change.
>
> > +/* 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).
>
> Agreed, fixed.
>
> > ## 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).
>
> I checked postgres_fdw commit log, and it seemed that the version was
> updated when SQL functions are added. Fixed.
>
> > + <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..."?
>
> I checked the docs and fixed. Moreover, some inconsistent statements were
> also fixed.
>
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
>
> Hi,
For v25-0001-Add-PQConnCheck-and-PQCanConnCheck-to-libpq.patch ,
`pqConnCheck_internal` only has one caller which is quite short.
Can pqConnCheck_internal and PQConnCheck be merged into one func ?

+int
+PQCanConnCheck(void)

It seems the return value should be of bool type.

Cheers