Dear Fujii-san, Tom, Thank you for giving a suggestion! PSA new version.
> 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? Current style is motivated by Onder [1] and later discussions. I thought it might be useful for other developers, but OK, I can remove changes on libpq modules. Horiguchi-san has suggested [2] that it might be overkill to use WaitEventSet() mechanism, so I kept using poll(). I reused the same naming as previous version because they actually do something Like libpq, but better naming is very welcome. > 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. I did not considered the situation because I have not came up with the situation that only one of connections to the same foreign server is broken. > 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. Just to confirm, your point "user name" means a local user, right? I made a patch for addressing them. > 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. Added, See 0003. IIUC there is no good way to extract user mapping from its OID, so I have added an function to do that and used 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? Renamed. > 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. It is sad for me, but it is more important for PostgreSQL to add nicer codes. I changed status to "Needs review" again. [1]: https://www.postgresql.org/message-id/CACawEhW19nPfbFpvfke9eidFDxAy%2Bic36wmY0s936T%3DxzxgHog%40mail.gmail.com [2]: https://www.postgresql.org/message-id/20221017.172642.45253962719866795.horikyota.ntt%40gmail.com Best Regards, Hayato Kuroda FUJITSU LIMITED
v38-0001-postgres_fdw-add-postgres_fdw_verify_connection-.patch
Description: v38-0001-postgres_fdw-add-postgres_fdw_verify_connection-.patch
v38-0002-add-test.patch
Description: v38-0002-add-test.patch
v38-0003-Extend-postgres_fdw_get_connections-to-return-us.patch
Description: v38-0003-Extend-postgres_fdw_get_connections-to-return-us.patch
