MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII;
 format=flowed
Content-Transfer-Encoding: 7bit
Date: Mon, 06 Dec 2021 20:06:49 +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: 
 <CALNJ-vSDLaDnbJpDuKTKwugX+dUnoLD+eEXHdR-aBRXG1Dh3hA@mail.gmail.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>
User-Agent: Roundcube Webmail/1.4.11
Message-ID: <b9076e887ec768463428ecccf4e63a26@oss.nttdata.com>
Archived-At: 
 <https://www.postgresql.org/message-id/b9076e887ec768463428ecccf4e63a26%40oss.nttdata.com>
Precedence: bulk

On 2021-11-29 21:36, Zhihong Yu wrote:
> On Mon, Nov 29, 2021 at 12:51 AM kuroda.hayato@fujitsu.com
> <kuroda.hayato@fujitsu.com> wrote:
> 
>> Dear Zhihong,
>> 
>> Thank you for giving comments! I'll post new patches later.
>> 
>>> +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS()
>> (CheckingRemoteServersHoldoffCount++)
>>> 
>>> The macro contains only one operation. Can the macro be removed
>> (with `CheckingRemoteServersHoldoffCount++` inlined) ?
>> 
>> Hmm, these HOLD/RESUME macros are followed HOLD_INTERRUPUST() and
>> HOLD_CANCEL_INTERRUPTS():
>> 
>> ```
>> #define HOLD_INTERRUPTS()  (InterruptHoldoffCount++)
>> 
>> #define RESUME_INTERRUPTS() \
>> do { \
>> Assert(InterruptHoldoffCount > 0); \
>> InterruptHoldoffCount--; \
>> } while(0)
>> 
>> #define HOLD_CANCEL_INTERRUPTS()  (QueryCancelHoldoffCount++)
>> 
>> #define RESUME_CANCEL_INTERRUPTS() \
>> do { \
>> Assert(QueryCancelHoldoffCount > 0); \
>> QueryCancelHoldoffCount--; \
>> } while(0)
>> 
>> #define START_CRIT_SECTION()  (CritSectionCount++)
>> 
>> #define END_CRIT_SECTION() \
>> do { \
>> Assert(CritSectionCount > 0); \
>> CritSectionCount--; \
>> } while(0)
>> ```
>> 
>> So I want to keep the current style. Could you tell me if you have
>> any other reasons?
>> 
>>> +   if (CheckingRemoteServersTimeoutPending &&
>> CheckingRemoteServersHoldoffCount != 0)
>>> +   {
>>> +       /*
>>> +        * Skip checking foreign servers while reading messages.
>>> +        */
>>> +       InterruptPending = true;
>>> +   }
>>> +   else if (CheckingRemoteServersTimeoutPending)
>>> 
>>> Would the code be more readable if the above two if blocks be
>> moved inside one enclosing if block (factoring the common
>> condition)?
>>> 
>>> +   if (CheckingRemoteServersTimeoutPending)
>> 
>> +1. Will fix.
>> 
>> Best Regards,
>> Hayato Kuroda
>> FUJITSU LIMITED
> 
> Hi,
> 
> It is Okay to keep the macros.
> 
> Thanks

Hi, Kuroda-san. Sorry for late reply.

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?

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".


-- 
Regards,

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