MIME-Version: 1.0
References: <20220217.170832.627054018291482509.horikyota.ntt@gmail.com>
 <TYAPR01MB58664E79C4728A6FFEA46F94F53B9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <379473f3-ed14-a19f-92ed-91dc73d430d3@oss.nttdata.com>
 <TYAPR01MB5866FF28C802916242F1B492F53B9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <TYAPR01MB5866C6278B6386672B35F624F53B9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <4968741d-f32c-a251-efc9-2d3db83e6891@oss.nttdata.com>
 <TYAPR01MB5866FC683843ED8BD09505FEF53D9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <TYAPR01MB5866982FC21ABE5DF3C1E3A2F5059@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <2160c6db-079a-be7c-df96-6c5cb21c03cf@oss.nttdata.com>
 <TYAPR01MB58661CD72B69848FBE0CD466F54F9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <20221002153905.kaadndwwqemd3eu2@awork3.anarazel.de>
 <TYAPR01MB586690F8E0FC4AAC0C1A32A6F55B9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
 <CACawEhW_R=6mKsB24QW3WpCZTQgtxAPH7J0q8yedKkCQY2xT0w@mail.gmail.com>
 <TYAPR01MB5866F419C4261177578AC1CCF55B9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <TYAPR01MB5866F419C4261177578AC1CCF55B9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
From: =?UTF-8?B?w5ZuZGVyIEthbGFjxLE=?= <onderkalaci@gmail.com>
Date: Mon, 3 Oct 2022 16:19:10 +0200
Message-ID: 
 <CACawEhXAy4gjJ4G3-59Xv_yb8Xbtn1VMYP8SOj-0C9dG2zbAcQ@mail.gmail.com>
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
To: "kuroda.hayato@fujitsu.com" <kuroda.hayato@fujitsu.com>
Cc: Andres Freund <andres@anarazel.de>,
 Fujii Masao <masao.fujii@oss.nttdata.com>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
	"Shinya11.Kato@oss.nttdata.com" <Shinya11.Kato@oss.nttdata.com>,
 "zyu@yugabyte.com" <zyu@yugabyte.com>,
	Kyotaro Horiguchi <horikyota.ntt@gmail.com>
Content-Type: multipart/alternative; boundary="000000000000a72df605ea22072b"
Archived-At: 
 <https://www.postgresql.org/message-id/CACawEhXAy4gjJ4G3-59Xv_yb8Xbtn1VMYP8SOj-0C9dG2zbAcQ%40mail.gmail.com>
Precedence: bulk

--000000000000a72df605ea22072b
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Hayato Kuroda,


> If the checking function is called not periodically but GetConnection(),
> it means that the health of foreign servers will be check only when remot=
e
> connections are used.
> So following workload described in [1] cannot handle the issue.
>
> BEGIN --- remote operations--- local operations --- COMMIT
>
>
As far as I can see this patch is mostly useful for detecting the failures
on the initial remote command. This is especially common when the remote
server does a failover/switchover and postgres_fdw uses a cached connection
to access to the remote server.

The remote server failures within a transaction block sounds like a much
less common problem. Still, you can give a good error message before COMMIT
is sent to the remote server.

I agree that this doesn't solve the issue you described, but I'm not sure
if it is worthwhile to fix such a problem.


> > Can we have this function/logic on Postgres core, so that other
> extensions
> > can also use?
>
> I was not sure about any use-case, but I think it can because it does
> quite general things.
> Is there any good motivation to do that?
>

I think any extension that deals with multiple Postgres nodes can benefit
from such logic. In fact, the reason I realized this patch is that on the
Citus extension, we are trying to solve a similar problem [1], [2].

Thinking even more, I think any extension that uses libpq and WaitEventSets
can benefit from such a function.


> > Do you see any performance implications of creating/freeing waitEventSe=
ts
> > per check? I wonder if we can somehow re-use the same waitEventSet by
> > modifyWaitEvent? I guess no, but still, if this check causes a
> performance
> > implication, can we somehow cache 1 waitEventSet per connection?
>
> I have not tested yet, but I agreed this will be caused performance
> decrease.
> In next version first I will re-use the event set anyway, and it must be
> considered later.
> Actually I'm not sure your suggestion,
> but you mean to say that we can add a hash table that associates  PGconn
> and WaitEventSet,  right?
>
>
I think it also depends on where you decide to put
pgfdw_connection_check_internal(). If you prefer the postgres_fdw side,
could we maybe use ConnCacheEntry in contrib/postgres_fdw/connection.c?

But if you decide to put it into the Postgres side, the API
for pgfdw_connection_check_internal() -- or equivalent function -- could be
discussed. Do we pass a WaitEventSet and if it is NULL create a new one,
else use what is passed to the function? Not sure, maybe you can come up
with a better API.

Thanks,
Onder KALACI

[1]: Check WL_SOCKET_CLOSED before using any connection by onderkalaci =C2=
=B7
Pull Request #6259 =C2=B7 citusdata/citus (github.com)
<https://github.com/citusdata/citus/pull/6259>
[2]: Add a single connection retry in MULTI_CONNECTION_LOST state by
marcocitus =C2=B7 Pull Request #6283 =C2=B7 citusdata/citus (github.com)
<https://github.com/citusdata/citus/pull/6283>

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

<div dir=3D"ltr"><div dir=3D"ltr">Hi=C2=A0Hayato Kuroda,<br></div><div clas=
s=3D"gmail_quote"><div><br></div><div>=C2=A0</div><blockquote class=3D"gmai=
l_quote" style=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,20=
4,204);padding-left:1ex">
If the checking function is called not periodically but GetConnection(),<br=
>
it means that the health of foreign servers will be check only when remote =
connections are used.<br>
So following workload described in [1] cannot handle the issue.<br>
<br>
BEGIN --- remote operations--- local operations --- COMMIT<br>
<br></blockquote><div><br></div><div>As far as I can see this patch is most=
ly useful for detecting the failures on the initial remote command. This is=
 especially common when the remote server does a failover/switchover and po=
stgres_fdw uses=C2=A0a cached=C2=A0connection to access to the remote=C2=A0=
server.</div><div><br></div><div>The remote=C2=A0server=C2=A0failures withi=
n a transaction block sounds like a much less common=C2=A0problem. Still, y=
ou can give a good error message before COMMIT is sent to the remote server=
.</div><div>=C2=A0<br></div><div>I agree that this=C2=A0doesn&#39;t solve t=
he issue you described, but I&#39;m not sure if it is worthwhile=C2=A0to fi=
x such a problem.</div><div><br></div><blockquote class=3D"gmail_quote" sty=
le=3D"margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);paddi=
ng-left:1ex">
<br>
&gt; Can we have this function/logic on Postgres core, so that other extens=
ions<br>
&gt; can also use?<br>
<br>
I was not sure about any use-case, but I think it can because it does quite=
 general things.<br>
Is there any good motivation to do that?<br></blockquote><div><br></div><di=
v>I think any extension that deals with multiple Postgres nodes can benefit=
 from such logic. In fact, the reason I realized this patch is that on the =
Citus extension, we are trying to solve a similar problem [1], [2].</div><d=
iv><br></div><div>Thinking even more, I think any extension that uses libpq=
 and WaitEventSets can benefit from such a function.=C2=A0</div><div><br></=
div><blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;bor=
der-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
&gt; Do you see any performance implications of creating/freeing waitEventS=
ets<br>
&gt; per check? I wonder if we can somehow re-use the same waitEventSet by<=
br>
&gt; modifyWaitEvent? I guess no, but still, if this check causes a perform=
ance<br>
&gt; implication, can we somehow cache 1 waitEventSet per connection?<br>
<br>
I have not tested yet, but I agreed this will be caused performance decreas=
e.<br>
In next version first I will re-use the event set anyway, and it must be co=
nsidered later.<br>
Actually I&#39;m not sure your suggestion,<br>
but you mean to say that we can add a hash table that associates=C2=A0 PGco=
nn and WaitEventSet,=C2=A0 right?<br><br></blockquote><div><br></div><div>I=
 think it also depends on where you decide to put pgfdw_connection_check_in=
ternal(). If you prefer the postgres_fdw side, could we maybe use ConnCache=
Entry in=C2=A0contrib/postgres_fdw/connection.c?=C2=A0</div><div><br></div>=
<div>But if you decide to put it into the Postgres side, the API for=C2=A0p=
gfdw_connection_check_internal() -- or equivalent function -- could be disc=
ussed. Do we pass a WaitEventSet and if it is NULL create=C2=A0a new one, e=
lse use what is passed to the function? Not sure, maybe you can come up wit=
h a better API.</div><div>=C2=A0</div><div>Thanks,</div><div>Onder KALACI</=
div><div><br></div><div>[1]:=C2=A0<a href=3D"https://github.com/citusdata/c=
itus/pull/6259">Check WL_SOCKET_CLOSED before using any connection by onder=
kalaci =C2=B7 Pull Request #6259 =C2=B7 citusdata/citus (github.com)</a></d=
iv><div>[2]:=C2=A0<a href=3D"https://github.com/citusdata/citus/pull/6283">=
Add a single connection retry in MULTI_CONNECTION_LOST state by marcocitus =
=C2=B7 Pull Request #6283 =C2=B7 citusdata/citus (github.com)</a>=C2=A0</di=
v></div></div>

--000000000000a72df605ea22072b--