MIME-Version: 1.0
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>
In-Reply-To: 
 <TYAPR01MB5866D73C56A4363039175E56F5669@TYAPR01MB5866.jpnprd01.prod.outlook.com>
From: Zhihong Yu <zyu@yugabyte.com>
Date: Mon, 29 Nov 2021 04:36:10 -0800
Message-ID: 
 <CALNJ-vSDLaDnbJpDuKTKwugX+dUnoLD+eEXHdR-aBRXG1Dh3hA@mail.gmail.com>
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
To: "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>
Cc: Shinya Kato <Shinya11.Kato@oss.nttdata.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: multipart/alternative; boundary="00000000000093f0d505d1eca982"
Archived-At: 
 <https://www.postgresql.org/message-id/CALNJ-vSDLaDnbJpDuKTKwugX%2BdUnoLD%2BeEXHdR-aBRXG1Dh3hA%40mail.gmail.com>
Precedence: bulk

--00000000000093f0d505d1eca982
Content-Type: text/plain; charset="UTF-8"

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

--00000000000093f0d505d1eca982
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr"><div dir=3D"ltr"><br></div><br><div class=3D"gmail_quote">=
<div dir=3D"ltr" class=3D"gmail_attr">On Mon, Nov 29, 2021 at 12:51 AM <a h=
ref=3D"mailto:kuroda.hayato@fujitsu.com">kuroda.hayato@fujitsu.com</a> &lt;=
<a href=3D"mailto:kuroda.hayato@fujitsu.com">kuroda.hayato@fujitsu.com</a>&=
gt; wrote:<br></div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0=
px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Dear =
Zhihong,<br>
<br>
Thank you for giving comments! I&#39;ll post new patches later.<br>
<br>
&gt; +#define HOLD_CHECKING_REMOTE_SERVERS_INTERRUPTS()=C2=A0 (CheckingRemo=
teServersHoldoffCount++)<br>
&gt; <br>
&gt; The macro contains only one operation. Can the macro be removed (with =
`CheckingRemoteServersHoldoffCount++` inlined) ?<br>
<br>
Hmm, these HOLD/RESUME macros are followed HOLD_INTERRUPUST() and HOLD_CANC=
EL_INTERRUPTS():<br>
<br>
```<br>
#define HOLD_INTERRUPTS()=C2=A0 (InterruptHoldoffCount++)<br>
<br>
#define RESUME_INTERRUPTS() \<br>
do { \<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 Assert(InterruptHoldoffCount &gt; 0); \<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 InterruptHoldoffCount--; \<br>
} while(0)<br>
<br>
#define HOLD_CANCEL_INTERRUPTS()=C2=A0 (QueryCancelHoldoffCount++)<br>
<br>
#define RESUME_CANCEL_INTERRUPTS() \<br>
do { \<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 Assert(QueryCancelHoldoffCount &gt; 0); \<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 QueryCancelHoldoffCount--; \<br>
} while(0)<br>
<br>
#define START_CRIT_SECTION()=C2=A0 (CritSectionCount++)<br>
<br>
#define END_CRIT_SECTION() \<br>
do { \<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 Assert(CritSectionCount &gt; 0); \<br>
=C2=A0 =C2=A0 =C2=A0 =C2=A0 CritSectionCount--; \<br>
} while(0)<br>
```<br>
<br>
So I want to keep the current style. Could you tell me if you have any othe=
r reasons?<br>
<br>
&gt; +=C2=A0 =C2=A0if (CheckingRemoteServersTimeoutPending &amp;&amp; Check=
ingRemoteServersHoldoffCount !=3D 0)<br>
&gt; +=C2=A0 =C2=A0{<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0/*<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 * Skip checking foreign servers while rea=
ding messages.<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0 */<br>
&gt; +=C2=A0 =C2=A0 =C2=A0 =C2=A0InterruptPending =3D true;<br>
&gt; +=C2=A0 =C2=A0}<br>
&gt; +=C2=A0 =C2=A0else if (CheckingRemoteServersTimeoutPending)<br>
&gt; <br>
&gt; Would the code be more readable if the above two if blocks be moved in=
side one enclosing if block (factoring the common condition)?<br>
&gt; <br>
&gt; +=C2=A0 =C2=A0if (CheckingRemoteServersTimeoutPending)<br>
<br>
+1. Will fix.<br>
<br>
Best Regards,<br>
Hayato Kuroda<br>
FUJITSU LIMITED<br></blockquote><div><br></div><div>Hi,</div><div><br></div=
><div>It is Okay to keep the macros.</div><div><br></div><div>Thanks</div><=
/div></div>

--00000000000093f0d505d1eca982--