public inbox for [email protected]
help / color / mirror / Atom feedFrom: Katsuragi Yuta <[email protected]>
To: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: 'Ted Yu' <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: vignesh C <[email protected]>
Cc: [email protected]
Cc: Önder Kalacı <[email protected]>
Cc: Fujii Masao <[email protected]>
Cc: [email protected]
Cc: Kyotaro Horiguchi <[email protected]>
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
Date: Wed, 08 Feb 2023 16:10:25 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <TYAPR01MB586675882721E646693E13FBF5CC9@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>
<[email protected]>
<TYAPR01MB586635DB5F1C5CE4660175FEF51A9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<TYAPR01MB58660C8F72746C2A8C6381F2F5EA9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<TYAPR01MB586613F662443D5A3005CD17F5FF9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<[email protected]>
<TYAPR01MB586612A29BAF468042F04B20F5FF9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<CALte62zppS-YxEhes5jSsHppQsRrgHBw-krViS1HJJPSsV9onw@mail.gmail.com>
<TYAPR01MB5866413814861A5016839506F5FC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<TYAPR01MB5866C20861167C9CBEDF0A36F5FC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<[email protected]>
<TYAPR01MB5866E17F6C47AB41DCB8ADBEF5CA9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<CALte62y83j9p8xOoOS=ftG8PZTwUCUBHAp=WiKM7i6NUon3cNg@mail.gmail.com>
<TYAPR01MB58668DD17F19CAFC97A3134EF5C89@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<[email protected]>
<TYAPR01MB5866969DBAEEEC5AE07591BFF5CE9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<TYAPR01MB5866B55A252EF7005AA81D5BF5CC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<TYAPR01MB586675882721E646693E13FBF5CC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
On 2023-01-27 15:57, Hayato Kuroda (Fujitsu) wrote:
> I found cfbot failure, PSA fixed version.
> Sorry for noise.
>
> Best Regards,
> Hayato Kuroda
> FUJITSU LIMITED
Hi Kuroda-san,
Thank you for updating the patch! Sorry for the late reply.
0001:
+ while (result < 0 && errno == EINTR);
+
+ if (result < 0)
+ return -1;
this `return -1` is not indented properly.
0002:
+ <term><function>postgres_fdw_verify_connection_states(server_name
text) returns boolean</function></term>
...
+ extension to the <symbol>poll</symbol> system call, including
Linux. This
+ returns <literal>true</literal> if checked connections are still
valid,
+ or the checking is not supported on this platform.
<literal>false</literal>
+ is returned if the local session seems to be disconnected from
other
+ servers. Example usage of the function:
Here, 'still valid' seems a little bit confusing because this 'valid' is
not
the same as postgres_fdw_get_connections's 'valid' [1].
Should 'still valid' be 'existing connection is not closed by the remote
peer'?
But this description does not cover all the cases where this function
returns true...
I think one choice is to write all the cases like 'returns true if any
of the
following condition is satisfied. 1) existing connection is not closed
by the
remote peer 2) there is no connection for specified server yet 3) the
checking
is not supported...'. If my understanding is not correct, please point
it out.
BTW, is it reasonable to return true if ConnectionHash is not
initialized or
there is no ConnCacheEntry for specified remote server? What do you
think
about returning NULL in that case?
0003:
I think it is better that the test covers all the new functions.
How about adding a test for postgres_fdw_verify_connection_states_all?
+-- ===================================================================
+-- test for postgres_fdw_verify_foreign_servers function
+-- ===================================================================
Writing all the functions' name like 'test for
postgres_fdw_verify_connection_states
and postgres_fdw_can_verify_connection_states' looks straightforward.
What do you think about this?
0004:
Sorry, I have not read 0004. I will read.
[1]:
https://github.com/postgres/postgres/blob/master/doc/src/sgml/postgres-fdw.sgml#L764-L765
regards,
--
Katsuragi Yuta
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
view thread (19+ messages) latest in thread
reply
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: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox