MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Fri, 20 Jan 2023 19:41:41 +0900
From: Katsuragi Yuta <katsuragiy@oss.nttdata.com>
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: 'Ted Yu' <yuzhihong@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>, vignesh C
 <vignesh21@gmail.com>, pgsql-hackers@lists.postgresql.org,
 =?UTF-8?Q?=C3=96nder_Kalac=C4=B1?= <onderkalaci@gmail.com>, Fujii Masao
 <masao.fujii@oss.nttdata.com>, Shinya11.Kato@oss.nttdata.com, Kyotaro
 Horiguchi <horikyota.ntt@gmail.com>
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
In-Reply-To: <TYAPR01MB5866C20861167C9CBEDF0A36F5FC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
References: <CACawEhW_R=6mKsB24QW3WpCZTQgtxAPH7J0q8yedKkCQY2xT0w@mail.gmail.com>
 <TYAPR01MB5866F419C4261177578AC1CCF55B9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CACawEhXAy4gjJ4G3-59Xv_yb8Xbtn1VMYP8SOj-0C9dG2zbAcQ@mail.gmail.com>
 <TYAPR01MB5866ACB4D3B845B4B2ED121BF55A9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CACawEhW19nPfbFpvfke9eidFDxAy+ic36wmY0s936T=xzxgHog@mail.gmail.com>
 <TYAPR01MB5866CFD6BAE6DDF01A27CBF1F5299@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CACawEhUzpqYJ8mQmSjYgX0ePtPpvb2u9Onjf6pCjUGkoZ=-xSg@mail.gmail.com>
 <TYAPR01MB58668728393648C2F7DC7C85F5399@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CACawEhW56PHQ83Q59x4U5zpi0rVJ6=0Vn-FeYSLZ13Y0yasebQ@mail.gmail.com>
 <TYAPR01MB5866AAA4EB0632B4001AA2C5F50E9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <20221206182638.w3vrfal4qix4hf2w@awork3.anarazel.de>
 <TYAPR01MB586635DB5F1C5CE4660175FEF51A9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <TYAPR01MB58660C8F72746C2A8C6381F2F5EA9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <TYAPR01MB586613F662443D5A3005CD17F5FF9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <204604.1673324688@sss.pgh.pa.us>
 <TYAPR01MB586612A29BAF468042F04B20F5FF9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CALte62zppS-YxEhes5jSsHppQsRrgHBw-krViS1HJJPSsV9onw@mail.gmail.com>
 <TYAPR01MB5866413814861A5016839506F5FC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <TYAPR01MB5866C20861167C9CBEDF0A36F5FC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <d8fb3a988694c1be5540c89dffd9b8db@oss.nttdata.com>
Archived-At: <https://www.postgresql.org/message-id/d8fb3a988694c1be5540c89dffd9b8db%40oss.nttdata.com>
Precedence: bulk

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