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

On Tue, Jan 4, 2022 at 10:21 PM Shinya Kato <Shinya11(dot)Kato(at)oss(dot)nttdata(dot)com>
wrote:

> Thank you for the new patch!
>
> On 2021-12-15 15:40, kuroda(dot)hayato(at)fujitsu(dot)com wrote:
> > Dear Kato-san,
> >
> > Thank you for giving comments! And sorry for late reply.
> > I rebased my patches.
> >
> >> Even for local-only transaction, I thought it useless to execute
> >> CallCheckingRemoteServersCallbacks() and enable_timeout_after(). Can I
> >> make it so that it determines outside whether it contains SQL to the
> >> remote or not?
> >
> > Yeah, remote-checking timeout will be enable even ifa local
> > transaction is opened.
> > In my understanding, postgres cannot distinguish whether opening
> > transactions
> > are using only local object or not.
> > My first idea was that turning on the timeout when GetFdwRoutineXXX
> > functions
> > were called, but in some cases FDWs may not use remote connection even
> > if
> > they call such a handler function. e.g. postgresExplainForeignScan().
> > Hence I tried another approach that unregister all checking callback
> > the end of each transactions. Only FDWs can notice that transactions
> > are remote or local,
> > so they must register callback when they really connect to other
> > database.
> > This implementation will avoid calling remote checking in the case of
> > local transaction,
> > but multiple registering and unregistering may lead another overhead.
> > I attached which implements that.
> >
> It certainly incurs another overhead, but I think it's better than the
> previous one.
> So far, I haven't encountered any problems, but I'd like other people's
> opinions.
>
> >> The following points are minor.
> >> 1. In foreign.c, some of the lines are too long and should be broken.
> >> 2. In pqcomm.c, the newline have been removed, what is the intention
> >> of
> >> this?
> >> 3. In postgres.c,
> >> 3-1. how about inserting a comment between lines 2713 and 2714,
> >> similar
> >> to line 2707?
> >> 3-2. the line breaks in line 3242 and line 3243 should be aligned.
> >> 3-3. you should change
> >> /*
> >> * Skip checking foreign servers while reading
> >> messages.
> >> */
> >> to
> >> /*
> >> * Skip checking foreign servers while reading
> >> messages.
> >> */
> >> 4. In connection.c, There is a typo in line 1684, so "fucntion" should
> >> be changed to "function".
> >
> > Maybe all of them were fixed. Thanks!
> Thank you, and it looks good to me.
>
>
> Hi,
+ UnregisterAllCheckingRemoteServersCallback();

UnregisterAllCheckingRemoteServersCallback
-> UnregisterAllCheckingRemoteServersCallbacks

+ CheckingRemoteServersCallbackItem *item;
+ item = fdw_callbacks;

The above two lines can be combined.

+UnregisterCheckingRemoteServersCallback(CheckingRemoteServersCallback
callback,
+ void *arg)

Is the above func called anywhere ?

+ if (item->callback == callback && item->arg == arg)

Is comparing argument pointers stable ? What if the same argument is cloned
?

+ CallCheckingRemoteServersCallbacks();
+
+ if (remote_servers_connection_check_interval > 0)
+ enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT,
+
remote_servers_connection_check_interval);

Should the call to CallCheckingRemoteServersCallbacks() be placed under the
if block checking remote_servers_connection_check_interval ?
If remote_servers_connection_check_interval is 0, it seems the callbacks
shouldn't be called.

Cheers