From: Fujii Masao <masao.fujii@oss.nttdata.com>
To: Hayato Kuroda (Fujitsu) <kuroda.hayato@fujitsu.com>
Cc: Katsuragi Yuta <katsuragiy@oss.nttdata.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Ted Yu <yuzhihong@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@lists.postgresql.org <pgsql-hackers@lists.postgresql.org>
Cc: Önder Kalacı <onderkalaci@gmail.com>
Cc: Shinya11.Kato@oss.nttdata.com <Shinya11.Kato@oss.nttdata.com>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Cc: Shubham Khanna <khannashubham1197@gmail.com>
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
Date: Fri, 19 Jul 2024 01:30:29 +0900
Message-ID: <613adbf9-c86e-41dc-ab97-27dc4e7281c7@oss.nttdata.com> (raw)
In-Reply-To: <TYAPR01MB56923ACAE33FD04A3560F413F5AC2@TYAPR01MB5692.jpnprd01.prod.outlook.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>
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: masao.fujii@oss.nttdata.com, kuroda.hayato@fujitsu.com, katsuragiy@oss.nttdata.com, vignesh21@gmail.com, yuzhihong@gmail.com, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org, onderkalaci@gmail.com, Shinya11.Kato@oss.nttdata.com, horikyota.ntt@gmail.com, khannashubham1197@gmail.com
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
In-Reply-To: <613adbf9-c86e-41dc-ab97-27dc4e7281c7@oss.nttdata.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox