RE: [Proposal] Add foreign-server health checks infrastructure

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