On 2023/03/13 16:05, Hayato Kuroda (Fujitsu) wrote:
> Thank you so much for your reviewing!
> Now we can wait comments from senior members and committers.
Thanks for working on this patch!
Regarding 0001 patch, on second thought, to me, it seems odd to expose
a function that doesn't have anything to directly do with PostgreSQL
as a libpq function. The function simply calls poll() on the socket
with POLLRDHUP if it is supported. While it is certainly convenient to
have this function, I'm not sure that it fits within the scope of libpq.
Thought?
Regarding 0002 patch, the behavior of postgres_fdw_verify_connection_states()
in regards to multiple connections using different user mappings seems
not well documented. The function seems to return false if either of
those connections has been closed.
This behavior means that it's difficult to identify which connection
has been closed when there are multiple ones to the given server.
To make it easier to identify that, it could be helpful to extend
the postgres_fdw_verify_connection_states() function so that it accepts
a unique connection as an input instead of just the server name.
One suggestion is to extend the function so that it accepts
both the server name and the user name used for the connection,
and checks the specified connection. If only the server name is specified,
the function should check all connections to the server and return false
if any of them are closed. This would be helpful since there is typically
only one connection to the server in most cases.
Additionally, it would be helpful to extend the postgres_fdw_get_connections()
function to also return the user name used for each connection,
as currently there is no straightforward way to identify it.
The function name "postgres_fdw_verify_connection_states" may seem
unnecessarily long to me. A simpler name like
"postgres_fdw_verify_connection" may be enough?
The patch may not be ready for commit due to the review comments,
and with the feature freeze approaching in a few days,
it may not be possible to include this feature in v16.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION