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
