MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Wed, 05 Jan 2022 15:21:47 +0900
From: Shinya Kato <Shinya11.Kato@oss.nttdata.com>
To: kuroda.hayato@fujitsu.com
Cc: pgsql-hackers@lists.postgresql.org, zyu@yugabyte.com
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
In-Reply-To: 
 <TYAPR01MB5866BC188389FA07C0E6CFE1F5769@TYAPR01MB5866.jpnprd01.prod.outlook.com>
References: 
 <TYAPR01MB58662809E678253B90E82CE5F5889@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <54449d7bea79263e937266ad6ebc65b3@oss.nttdata.com>
 <TYAPR01MB58662CD4FD98AA475B3D10F9F59B9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <4e64bb8a0a88f5b6ac5a8901377c81af@oss.nttdata.com>
 <TYAPR01MB58661992162BA7708191EC79F5619@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CALNJ-vQWNK6AzpL=5iXLfpr4oRMYdJ72tDYhxLwd=iX5H5XUdQ@mail.gmail.com>
 <TYAPR01MB5866D73C56A4363039175E56F5669@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CALNJ-vSDLaDnbJpDuKTKwugX+dUnoLD+eEXHdR-aBRXG1Dh3hA@mail.gmail.com>
 <b9076e887ec768463428ecccf4e63a26@oss.nttdata.com>
 <TYAPR01MB5866BC188389FA07C0E6CFE1F5769@TYAPR01MB5866.jpnprd01.prod.outlook.com>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <4a2059d419eec5671a6d167cb83229db@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/4a2059d419eec5671a6d167cb83229db%40oss.nttdata.com>
Precedence: bulk

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