On Sat, Jan 21, 2023 at 4:03 AM Hayato Kuroda (Fujitsu) < kuroda.hay...@fujitsu.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
