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
