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

Dear Katsuragi-san,

Thank you for reviewing! PSA new version patches.

> 0001:
> +       while (result < 0 && errno == EINTR);
> +
> +       if (result < 0)
> +                       return -1;
> 
> this `return -1` is not indented properly.

This part is no longer needed. Please see another discussion[1].

> 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.

Modified like you pointed 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?

I'm not sure which one is better, but modified accordingly.

> 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?

Added.

> 0004:
> Sorry, I have not read 0004. I will read.

No problem:-).

[1]: https://www.postgresql.org/message-id/TYAPR01MB58664E039F45959AB321FA1FF5D99%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED