On Wednesday, October 5, 2022 6:27 PM kuroda.hay...@fujitsu.com <kuroda.hay...@fujitsu.com> wrote: > Thanks for giving many comments! Based on off and on discussions, I modified > my patch. Here are my other quick review comments on v16.
(1) v16-0001 : definition of a new structure CheckingRemoteServersCallbackItem can be added to typedefs.list. (2) v16-0001 : UnRegisterCheckingRemoteServersCallback's header comment +void +UnRegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback, + void *arg) +{ Looks require a header comment for this function, because in this file, all other functions have each one. (3) v16-0002 : better place to declare new variables New variables 'old' and 'server' in GetConnection() can be moved to a scope of 'if' statement where those are used. @@ -138,6 +149,8 @@ GetConnection(UserMapping *user, bool will_prep_stmt, PgFdwConnState **state) ConnCacheEntry *entry; ConnCacheKey key; MemoryContext ccxt = CurrentMemoryContext; + MemoryContext old; + ForeignServer *server = GetForeignServer(user->serverid); (4) v16-0002 : typo in pgfdw_connection_check_internal() + /* return false is if the socket seems to be closed. */ It should be "return false if the socket seems to be closed" ? (5) v16-0002 : pgfdw_connection_check's message When I tested the new feature, I got a below message. "ERROR: Foreign Server my_external_server might be down." I think we can wrap the server name with double quotes so that the message is more aligned with existing error message like connect_pg_server. (6) v16-003 : typo + Authors needs to register the callback function via following API. Should be "Authors need to register the ...". (7) v16-0004 : unnecessary file I think we can remove a new file which looks like a log file. .../postgres_fdw/expected/postgres_fdw_1.out Best Regards, Takamichi Osumi