Dear Fujii-san, Hi, long time no see :-). Thanks for reviewing the patch! PSA new version.
> I've just started reviewing them. > > Here are the review comments for 0001 patch: > > Do we really need postgres_fdw_verify_connection_all()? The proposed feature > aims to check if all postgres_fdw connections used within the transaction > are still open. If any of those connections are closed, the transaction > can't be committed successfully, so users can roll back immediately upon > detecting a closed connection. > > However, postgres_fdw_verify_connection_all() checks all connections made in > the session, not just those used in the current transaction. This means > users can't determine if they should roll back the transaction based on > its return value. Therefore, I'm concerned that > postgres_fdw_verify_connection_all() might not be very useful. Thoughts? Right. My primal motivation is to detect the disconnection from remotes and abort current transaction as soon as possible. In this case, as I posted in [1], ...all() is not so helpful. I agreed to remove the function from patch set. Done. > Considering the purpose of this feature, wouldn't it be better to extend > postgres_fdw_get_connections() to include a "used_in_xact" column > (indicating whether the connection has been used in the current transaction) > and a "closed" column (indicating whether the connection has been closed)? > This approach might be more effective than introducing a new function > like the postgres_fdw_verify_connection family? > > If it's too much to check if the connection is closed by default whenever > calling postgres_fdw_get_connections(), we could modify it to accept > an argument indicating whether to perform this check. Thoughts? Sounds interesting. If we can accept to change the definition of pre-existing function, it seems better. To keep the default behavior, an input parameter should be added. Attached patch tried to implement. > Here are the review comments for 0003 patch: > > The source comment in postgres_fdw_get_connections() should mention > the return value user_name. Document was updated. > We also need to handle the case where the user mapping used by > the connection cache has been dropped. Otherwise, this could > lead to an error. > > ----------------------------- > =# BEGIN; > =*# SELECT * FROM postgres_fdw_get_connections(); > server_name | user_name | valid > -------------+-----------+------- > loopback | public | t > (1 row) > > =*# DROP USER MAPPING FOR public SERVER loopback ; > =*# SELECT * FROM postgres_fdw_get_connections(); > ERROR: cache lookup failed for user mapping 16409 > ----------------------------- Fixed. > -SELECT * FROM postgres_fdw_get_connections() ORDER BY 1; > +SELECT server_name, valid FROM postgres_fdw_get_connections() ORDER BY > 1; > > Shouldn't this test also check if the returned user_name is valid? You meant to say that we should print the user_name, right? Done. > + server_name | user_name | validvalid > +-------------+------------------------ > + loopback1 | postgres | t > + loopback2 | | f > > The column name "validvalid" should be "valid". Right, fixed. > How can we cause the record with server_name != NULL but user_name = NULL? Now this can happen when user_name is dropped, but the example was updated. Below contains a summary of changes. 0001: - Instead of adding new functions, postgres_fdw_get_connections() was extended. Some tricks were added to support old versions. I followed pg_stat_statements.c. Attributes were added after the `valid` to preserve ordering of the old version. - I found an inconsistency of name between source and doc, so I unified to postgres_fdw_can_verify_connection(). - Also, test patch (0002) was combined into this. 0002: - user_name was added after the `valid` to preserve ordering of the old version. - GetUserMappingFromOid() is allowed to miss a tuple. [1]: https://www.postgresql.org/message-id/TYAPR01MB58668728393648C2F7DC7C85F5399%40TYAPR01MB5866.jpnprd01.prod.outlook.com Best regards, Hayato Kuroda FUJITSU LIMITED
v41-0001-postgres_fdw-Allow-postgres_fdw_get_connections-.patch
Description: v41-0001-postgres_fdw-Allow-postgres_fdw_get_connections-.patch
v41-0002-Extend-postgres_fdw_get_connections-to-return-us.patch
Description: v41-0002-Extend-postgres_fdw_get_connections-to-return-us.patch
