Message-ID: <613adbf9-c86e-41dc-ab97-27dc4e7281c7@oss.nttdata.com>
Date: Fri, 19 Jul 2024 01:30:29 +0900
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: Katsuragi Yuta <katsuragiy@oss.nttdata.com>,
 vignesh C <vignesh21@gmail.com>, Ted Yu <yuzhihong@gmail.com>,
 Tom Lane <tgl@sss.pgh.pa.us>,
 "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
 =?UTF-8?B?w5ZuZGVyIEthbGFjxLE=?= <onderkalaci@gmail.com>,
 "Shinya11.Kato@oss.nttdata.com" <Shinya11.Kato@oss.nttdata.com>,
 Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
 'Shubham Khanna' <khannashubham1197@gmail.com>
References: 
 <TYAPR01MB58669EAAC02493BFF9F39B06F5D99@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <f723a26e8c34001ae32e3399951b8066@oss.nttdata.com>
 <TYAPR01MB5866F8EA3C06E1B43E42E4E4F5B79@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CALDaNm2H9ZyiPAOJCKHo5dHYv-u6u+8bv_T9PkdrvqVHnxyrmQ@mail.gmail.com>
 <TYAPR01MB586622DBD4A0C3BA57136899F5B49@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <36981865ac13b6c40d53bf3f98667eee@oss.nttdata.com>
 <300fccae11de8a4ed1a2d970db1a8680@oss.nttdata.com>
 <TYAPR01MB5866DBFEC61E81CCE9F67142F5B99@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <b492a935-6c7e-8c08-e485-3c1d64d7d10f@oss.nttdata.com>
 <TYAPR01MB5866D498F593B4AD7D63C148F5939@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CAHv8RjJvxDc2U3xUfvBwirw9Wk4oq8FVggv1VUhXNw6zQU8VZQ@mail.gmail.com>
 <OS3PR01MB9882E5D54A472AB0A5A098D8F58EA@OS3PR01MB9882.jpnprd01.prod.outlook.com>
 <be9382f7-5072-4760-8b3f-31d6dffa8d62@oss.nttdata.com>
 <TYAPR01MB56923ACAE33FD04A3560F413F5AC2@TYAPR01MB5692.jpnprd01.prod.outlook.com>
Content-Language: en-US
From: Fujii Masao <masao.fujii@oss.nttdata.com>
In-Reply-To: 
 <TYAPR01MB56923ACAE33FD04A3560F413F5AC2@TYAPR01MB5692.jpnprd01.prod.outlook.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/613adbf9-c86e-41dc-ab97-27dc4e7281c7%40oss.nttdata.com>
Precedence: bulk


On 2024/07/18 19:49, Hayato Kuroda (Fujitsu) wrote:
>> Shouldn't this test also check if the returned user_name is valid?
> 
> You meant to say that we should print the user_name, right? Done.

Yes, I think it's better to test if the value in the user_name column is as expected.


> - I found an inconsistency of name between source and doc,
>    so I unified to postgres_fdw_can_verify_connection().

I'm unsure about the necessity of introducing a standalone function to check
POLLRDHUP availability. Instead of providing postgres_fdw_can_verify_connection(),
could we modify postgres_fdw_get_connections() to return NULL in the "closed"
column on platforms where POLLRDHUP isn't supported?


> - Also, test patch (0002) was combined into this.
> 0002:
> - user_name was added after the `valid` to preserve ordering of the old version.

Do we really need to keep this ordering? Wouldn't it be more intuitive to
have the user_name column next to server_name? In pg_stat_statements,
for example, the ordering isn't always preserved, as seen with
WAL-related columns being added in the middle.


> Thanks for reviewing the patch! PSA new version.

Thanks for updating the patches!


The regression test for postgres_fdw failed with the following diff.

----------------------
  SELECT * FROM postgres_fdw_get_connections() ORDER BY 1;
   server_name | valid | user_name | used_in_xact | closed
  -------------+-------+-----------+--------------+--------
- loopback    | f     | hayato    | t            |
+ loopback    | f     | runner    | t            |
               | f     |           | t            |
  (2 rows)
----------------------


+			 * If requested and the connection is not invalidated, check the
+			 * status of the remote connection from the backend process and
+			 * return the result. Otherwise returns NULL.
+			 */
+			if (require_verify && !entry->invalidated && entry->conn)

Should we also consider checking invalidated connections? Even though
a connection is marked as invalidated, it can still be used until
the current transaction ends. Therefore, if an invalidated connection
is used in this transaction (i.e., used_in_xact = true) and has
already been closed (closed = true), users might want to consider
rolling back the transaction promptly. Thought?


+		-- Set client_min_messages to ERROR temporary because the following
+		-- function only throws a WARNING on the supported platform.

Is this still true? From my reading of the code, it doesn't appear
that the function throws a WARNING.

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION