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
