Re: [Proposal] Add foreign-server health checks infrastructure

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