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

On Wednesday, October 5, 2022 6:27 PM kuroda(dot)hayato(at)fujitsu(dot)com <kuroda(dot)hayato(at)fujitsu(dot)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