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

Thank you for the new patch!

On 2021-12-15 15:40, kuroda.hayato@fujitsu.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.


-- 
Regards,

--
Shinya Kato
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION