On Wednesday, October 5, 2022 6:27 PM kuroda.hayato@fujitsu.com <kuroda.hayato@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
