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

From: Fujii Masao <[email-masked]>
Date: Sun, 18 Sep 2022 12:39:14 +0900

On 2022/03/04 15:17, [email-masked] wrote:
> Hi Hackers,
> 
>> It's not happy, but I'm not sure about a good solution. I made a timer reschedule
>> if connection lost had detected. But if queries in the transaction are quite short,
>> catching SIGINT may be fail.
> 
> Attached uses another way: sets pending flags again if DoingCommandRead is true.
> If a remote server goes down while it is in idle_in_transaction,
> next query will fail because of ereport(ERROR).
> 
> How do you think?

Sounds ok to me.

Thanks for updating the patches!

These failed to be applied to the master branch cleanly. Could you update them?

+      this option relies on kernel events exposed by Linux, macOS,

s/this/This

+		GUC_check_errdetail("pgfdw_health_check_interval must be set to 0 on this platform");

The actual parameter name "postgres_fdw.health_check_interval"
should be used for the message instead of internal variable name.

+		/* Register a timeout for checking remote servers */
+		pgfdw_health_check_timeout = RegisterTimeout(USER_TIMEOUT, pgfdw_connection_check);

This registered signal handler does lots of things. But that's not acceptable
and they should be performed outside signal handler. No?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 21 Sep 2022 11:56:56 +0000

Dear Fujii-san,

Thanks for checking!

> These failed to be applied to the master branch cleanly. Could you update them?

PSA rebased patches. I reviewed my myself and they contain changes.
E.g., move GUC-related code to option.c.

> +      this option relies on kernel events exposed by Linux, macOS,

Fixed.

> should be used for the message instead of internal variable name.

Fixed.

> and they should be performed outside signal handler. No?

I modified like v09 or earlier versions, which has a mechanism for registering CheckingRemoteServersCallback.
It had been removed because we want to keep core simpler, but IIUC it is needed
if the signal handler just sets some flags.
The core-side does not consider the current status of transaction and running query for simpleness.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Andres Freund <[email-masked]>
Date: Sun, 2 Oct 2022 08:39:05 -0700

Hi,

> E.g., move GUC-related code to option.c.

This seems to reliably fail on windows. See
https://cirrus-ci.com/task/6454408568373248
https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest/39/3388

https://api.cirrus-ci.com/v1/artifact/task/6454408568373248/testrun/build/testrun/postgres_fdw/regress/regression.diffs

diff -w -U3 C:/cirrus/contrib/postgres_fdw/expected/postgres_fdw.out C:/cirrus/build/testrun/postgres_fdw/regress/results/postgres_fdw.out
--- C:/cirrus/contrib/postgres_fdw/expected/postgres_fdw.out	2022-10-02 14:47:24.486355800 +0000
+++ C:/cirrus/build/testrun/postgres_fdw/regress/results/postgres_fdw.out	2022-10-02 15:02:03.039752800 +0000
@@ -11478,6 +11478,8 @@
 ALTER SERVER loopback OPTIONS (SET application_name 'healthcheck');
 -- Set GUC for checking the health of remote servers
 SET postgres_fdw.health_check_interval TO '1s';
+ERROR:  invalid value for parameter "postgres_fdw.health_check_interval": 1000
+DETAIL:  postgres_fdw.health_check_interval must be set to 0 on this platform
 BEGIN;
 SELECT 1 FROM ft1 LIMIT 1;
  ?column?
@@ -11495,7 +11497,15 @@

 -- While sleeping the process down will be detected.
 SELECT pg_sleep(3);
-ERROR:  Foreign Server loopback might be down.
+ pg_sleep
+----------
+
+(1 row)
+
 COMMIT;
+ERROR:  server closed the connection unexpectedly
+	This probably means the server terminated abnormally
+	before or while processing the request.
+CONTEXT:  remote SQL command: COMMIT TRANSACTION
 -- Clean up
 RESET debug_discard_caches;

Greetings,

Andres Freund

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 3 Oct 2022 07:50:19 +0000

Dear Andres,

> This seems to reliably fail on windows. See

Thanks for reporting. Actually this feature cannot be used on Windows machine.
To check the status of each socket that connects to the foreign server,
the socket event WL_SOCKET_CLOSED is used.
The event is only enabled on some OSes, and Windows machine cannot.  

The part must be skipped if the system cannot be used the event, but I was not sure how to do that...

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Önder Kalacı <[email-masked]>
Date: Mon, 3 Oct 2022 10:59:01 +0200

Hi Hayato Kuroda,

Thanks for the patch. I think there are some non-fdw extensions out there
which could benefit from this logic. That's why I want to first learn some
more about high-level design/goals of the patch a little more.

+/*
> + * Call callbacks for checking remote servers.
> + */
> +void
> +CallCheckingRemoteServersCallbacks(void)

Why do we run this periodically, but not when a specific connection is
going to be used? Wouldn't running it periodically prevent detecting some
already-closed sockets at the time of the connection used (e.g., we checked
10 seconds ago, the server died 5 seconds ago)?

In other words, what is the trade-off for calling
pgfdw_connection_check_internal() inside GetConnection() when we are about
to use a "cached" connection? I think that might simplify the patch as well.

+/*
> + * Helper function for pgfdw_connection_check
> + */
> +static bool
> +pgfdw_connection_check_internal(PGconn *conn)
> +{

Can we have this function/logic on Postgres core, so that other extensions
can also use?

 + AddWaitEventToSet(eventset, WL_SOCKET_CLOSED, PQsocket(conn), NULL,
> NULL);

What if PQsocket(conn) returns -1? Maybe we move certain connection state
checks into pgfdw_connection_check_internal() such that it is more generic?
I can think of checks like: conn!=NULL, PQsocket(conn) != PGINVALID_SOCKET,
PQstatus == CONNECTION_OK

+ eventset = CreateWaitEventSet(CurrentMemoryContext, 1);
> + AddWaitEventToSet(eventset, WL_SOCKET_CLOSED, PQsocket(conn), NULL,
> NULL);
> +
> + WaitEventSetWait(eventset, 0, &events, 1, 0);
> +
> + if (events.events & WL_SOCKET_CLOSED)
> + {
> + FreeWaitEventSet(eventset);
> + return false;
> + }
> + FreeWaitEventSet(eventset);

Do you see any performance implications of creating/freeing waitEventSets
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?

Thanks,
Onder KALACI
Developing the Citus extension @Microsoft

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 3 Oct 2022 09:58:50 +0000

Dear Önder,

Thank you for being interest to my patch! Your suggestions will be included to newer version.

> to use a "cached" connection? I think that might simplify the patch as well

If the checking function is called not periodically but GetConnection(),
it means that the health of foreign servers will be check only when remote connections are used.
So following workload described in [1] cannot handle the issue.

BEGIN --- remote operations--- local operations --- COMMIT

But, yes, I perfectly agreed that it could simplify the code
because we can reduce the timer part. This is second plan of this patch,
I may move on this approach if it is still useful.

> Can we have this function/logic on Postgres core, so that other extensions

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?

> PQstatus == CONNECTION_OK

ereport(ERROR) will be thrown if PQsocket(conn) returns -1.
All of you said should be handled here. I will modify it.

> 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?

[1]: https://www.postgresql.org/message-id/TYAPR01MB58662809E678253B90E82CE5F5889%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Önder Kalacı <[email-masked]>
Date: Mon, 3 Oct 2022 16:19:10 +0200

Hi Hayato Kuroda,

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

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

> but you mean to say that we can add a hash table that associates  PGconn
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 ·
Pull Request #6259 · citusdata/citus (github.com)
<https://github.com/citusdata/citus/pull/6259>
[2]: Add a single connection retry in MULTI_CONNECTION_LOST state by
marcocitus · Pull Request #6283 · citusdata/citus (github.com)
<https://github.com/citusdata/citus/pull/6283>

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 4 Oct 2022 02:26:49 +0000

Dear Önder,

> to access to the remote server.

Sounds reasonable. Do you mean that we can add additional GUC like "postgres_fdw.initial_check",
wait WL_SOCKET_CLOSED if the conneciton is found in the hash table, and do reconnection if it might be closed, right?

> can benefit from such a function.

OK, I agreed it may be useful, but where should this function be?
This cannot be used from application, so it should not in interface/libpq dir. So backend/libpq/pqcomm.c?

> else use what is passed to the function? Not sure, maybe you can come up

Thank you for describing more detail. I can imagine you said.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 5 Oct 2022 09:27:07 +0000

Dear Önder, Fujii-san,

Thanks for giving many comments! Based on off and on discussions, I modified my patch.
Note that this patch-set did not contain all of fixes you said.

[Modified]

1.
QueryCancelPending was cleaned up when the query cancel was skipped.
In the previous version the QueryCancelPending was set to true again even if the ereport(ERROR) was skipped.
It was because I thought the transaction that accessed to foreign servers but one of them crashed should be aborted immediately.
But currenlty postgres does not have such "delayed" query cancel mechanism, so I followed the manner.

Another approach is documenting like "If QueryCancelPending is already filled, the callback function must send SIGINT signal".
This seems simpler for us, but it may be troublesome for FDW authors. How do you think?

2.
The new filed servername was added to the hash entry. It was needed
because the server name must be logged in the pgfdw_connection_check(), but according to [1],
the catalog should not be read in ProcessInterrupts() to avoid the race condition.
The string is pstrdup()'d when the entry has been created, and replaced when ALTER SERER RENAME has been executed.

3.
The manner to implement the health check function was documented.

4.
raise() was replaced to kill(), because it was not used in core code. 

5.
APIs to handle QueryCancelMessage were added to avoid overriding the message.

6.
Some checks for socket was added in the checking function.

[1]: https://www.postgresql.org/message-id/CA%2BTgmoYW_rSOW4JMQ9_0Df9PKQ%3DsQDOKUGA4Gc9D8w4wui8fSA%40mail.gmail.com

[Unmodified]

a.
The checking function is still in the postgres_fdw.
This is because I could not find any good file to move this function.
IIUC this function needs PGconn as an argument,
but it seems that it is declared in libpq-fe.h and it should not be included from core. 

b.
In the checking function, the eventset is still created, added, and freed.
Firstly I tried to declare eventset as inner-function static variable and reuse that, but I could not do. This is because:

* The size of eventset cannot be determined when the function is called for the first time.
  The size must be set in CreateWaitEventSet(), and it must be more than the number of remote connection.
* To reuse the eventset the added event must be removed every time, but it cannot.
  The API ModifyWaitEvent() can be only used for "modifying" the event, not "removing".
  If the function is called as events=0, we will get assertion failure in WaitEventAdjustEpoll() or something.

To avoid creating/freeing the event set, hash table must be modified but it has not done yet.

c.
Currently the status of socket is not checked in the GetConnection().
I agreed it may be useful, but I thought it might be out of scope. It could not meet the requirements.
I think it can be added separately.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Önder Kalacı <[email-masked]>
Date: Wed, 12 Oct 2022 15:30:34 +0200

Hi,

Sounds reasonable. Do you mean that we can add additional GUC like
> "postgres_fdw.initial_check",
> wait WL_SOCKET_CLOSED if the conneciton is found in the hash table, and do
> reconnection if it might be closed, right?
>
>
Alright, it took me sometime to realize that postgres_fdw already has a
retry mechanism if the first command fails: postgres_fdw: reestablish new
connection if cached one is detected as… · postgres/postgres@32a9c0b
(github.com)
<https://github.com/postgres/postgres/commit/32a9c0bdf493cf5fc029ab44a22384d547290eff>

Still, the reestablish mechanism can be further simplified with
WL_SOCKET_CLOSED event such as the following (where we should probably
rename pgfdw_connection_check_internal):

 /*
> * If the connection needs to be remade due to invalidation, disconnect as
> * soon as we're out of all transactions.
> */
>
* | +bool remoteSocketIsClosed =  entry->conn != NULL : *
pgfdw_connection_check_internal*(entry->conn) : false;*

> if (entry->conn != NULL && (entry->invalidated || remoteSocketIsClosed) &&
> entry->xact_depth == 0)

{
> elog(DEBUG3, "closing connection %p for option changes to take effect",
> entry->conn);
> disconnect_pg_server(entry);
> }

* | +else if (remoteSocketIsClosed && && entry->xact_depth > 0)*
* | + error ("Remote Server is down ...")*

In other words, a variation of pgfdw_connection_check_internal()
could potentially go into interfaces/libpq/libpq-fe.h
(backend/libpq/pqcomm.c or src/interfaces/libpq/fe-connect.c).
Then, GetConnection() in postgres_fdw, it can force to reconnect as it is
already done for some cases or error properly:

>
>  Based on off and on discussions, I modified my patch.
>
>
I still think that it is probably too much work/code to detect the
mentioned use-case you described on [1]. Each backend constantly
calling CallCheckingRemoteServersCallbacks() for this purpose doesn't sound
the optimal way to approach the "check whether server down" problem. You
typically try to decide whether a server is down by establishing a
connection (or ping etc), not going over all the existing connections.

As far as I can think of, it should probably be a single background task
checking whether the server is down. If so, sending an invalidation message
to all the backends such that related backends could act on the
invalidation and throw an error. This is to cover the use-case you
described on [1].

Also, maybe we could have a new catalog table like pg_foreign_server_health
or such, where we can keep the last time the check succeeded (and/or
failed), and how many times the check  succeeded (and/or failed).

This is of course how I would approach this problem. I think some other
perspectives on this would be very useful to hear.

Thanks,
Onder KALACI

  [1]
https://www.postgresql.org/message-id/TYAPR01MB58662809E678253B90E82CE5F5889%40TYAPR01MB5866.jpnprd01.prod.outlook.com

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 17 Oct 2022 01:45:31 +0000

Hi,

> Thanks for giving many comments! Based on off and on discussions, I modified
Thank you for your patch set !

While reviewing and testing the new v16, I've met a possible issue
by slightly adjusting the scenario written in [1].

I mainly followed the steps there and
replaced the command "SELECT" for the remote table at 6-9 with "INSERT" command.
Then, after waiting for few seconds, the "COMMIT" succeeded like below output,
even after the server stop of the worker side.

After the transaction itself, any reference to the remote table fails.
Note that the local table has some data initially in my test.

postgres=# begin;
BEGIN
postgres=*# insert into remote values (-1000);
INSERT 0 1
postgres=*# select * from local;
 number 
--------
    101
    102
    103
    104
    105
(5 rows)

postgres=*# commit;
COMMIT
postgres=# select * from remote;
ERROR:  could not connect to server "my_external_server"
DETAIL:  connection to server on socket "/tmp/.s.PGSQL.9999" failed: No such file or directory
        Is the server running locally and accepting connections on that socket?

Additionally, the last reference "SELECT" which failed above can succeed,
if I restart the worker server before the "SELECT" command to the remote table.
This means the transaction looks successful but the data isn't there ?
Could you please have a look at this issue ?

[1] - https://www.postgresql.org/message-id/TYAPR01MB58662CD4FD98AA475B3D10F9F59B9%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
	Takamichi Osumi

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 17 Oct 2022 03:02:05 +0000

> Thanks for giving many comments! Based on off and on discussions, I modified
Here are my other quick review comments on v16.

(1) v16-0001 : definition of a new structure

CheckingRemoteServersCallbackItem can be added to typedefs.list.

(2) v16-0001 : UnRegisterCheckingRemoteServersCallback's header comment

+void
+UnRegisterCheckingRemoteServersCallback(CheckingRemoteServersCallback callback,
+                                                                               void *arg)
+{

Looks require a header comment for this function,
because in this file, all other functions have each one.

(3) v16-0002 : better place to declare new variables

New variables 'old' and 'server' in GetConnection() can be moved to
a scope of 'if' statement where those are used.

@@ -138,6 +149,8 @@ GetConnection(UserMapping *user, bool will_prep_stmt, PgFdwConnState **state)
        ConnCacheEntry *entry;
        ConnCacheKey key;
        MemoryContext ccxt = CurrentMemoryContext;
+       MemoryContext old;
+       ForeignServer *server = GetForeignServer(user->serverid);

(4) v16-0002 : typo in pgfdw_connection_check_internal()

+       /* return false is if the socket seems to be closed. */

It should be "return false if the socket seems to be closed" ?

(5) v16-0002 : pgfdw_connection_check's message

When I tested the new feature, I got a below message.

"ERROR:  Foreign Server my_external_server might be down."

I think we can wrap the server name with double quotes
so that the message is more aligned with existing error message
like connect_pg_server.

(6) v16-003 : typo

+      Authors needs to register the callback function via following API.

Should be "Authors need to register the ...".

(7) v16-0004 : unnecessary file

I think we can remove a new file which looks like a log file.

.../postgres_fdw/expected/postgres_fdw_1.out

Best Regards,
	Takamichi Osumi

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 17 Oct 2022 07:27:21 +0000

Dear Önder,

Thanks for giving suggestions!

> rename pgfdw_connection_check_internal):

Sounds reasonable.
I think it may be included in this patch. I will try to next (or later) version.

> (backend/libpq/pqcomm.c or src/interfaces/libpq/fe-connect.c).

Hmm, IIUC libpq related function and data structures cannot be accessed from core source,
so we cannot move to pqcomm.c.
(This is a motivation for introducing libpqwalreceiver library. It is used to avoid to link libpq directly)
And functions in libpq/fe-connect.c will be included libpq.so,
but latch related functions like WaitEventSetWait() should not be called from client application.
So it is also not appropriate.
In short, there are no good position to place the function because this requires both of libpq and core functions.

> connection (or ping etc), not going over all the existing connections.

Yes, the approach that establishes a new connection is very simple, but I think it has some holes.
For example, if the DNS server or some routing software may is stopped,
we will fail to connect to foreign servers.
In your approach, we regard the case as "failed" and try to invalidate the server
even if the existing connection can be used.
Another case is that if a server goes down and failover has occurred, we will succeed to connect to foreign servers.
We may regard the case as a "success", but we cannot COMMIT the transaction.

> invalidation and throw an error. This is to cover the use-case you

Indeed your approach covers the use case I said, but I'm not sure whether it is really good. 
In your approach, once the background worker process will manage all foreign servers.
It may be OK if there are a few servers, but if there are hundreds of servers,
the time interval during checks will be longer.

Currently, each FDW can decide whether we do health checks or not per the backend.
For example, we can skip health checks if the foreign server is not used now.
The background worker cannot control such a way.

Moreover, methods to connect to foreign servers and check health are different per FDW.
In terms of mysql_fdw [1], we must do mysql_init() and mysql_real_connect().
About file_fdw, we do not have to connect, but developers may want to calculate checksum and compare.
Therefore, we must provide callback functions anyway.

Based on the above, I do not agree that we introduce a new background worker and make it to do a health check.

> perspectives on this would be very useful to hear.

Yes, we can hear other opinions :-).

[1]: https://github.com/EnterpriseDB/mysql_fdw

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Mon, 17 Oct 2022 17:26:42 +0900 (JST)

At Mon, 17 Oct 2022 07:27:21 +0000, "[email-masked]" <[email-masked]> wrote in 
> In short, there are no good position to place the function because this requires both of libpq and core functions.

Might be on slight different direction, but it looks to me a bit too
much to use WaitEventSet to check only if a socket is live or not.

A quick search in the tree told me that we could use pqSocketCheck()
instead, and I think it would be the something that "could potentially
go into libpq-fe.h" as Önder mentioned, if I understand what he said
correctly.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 17 Oct 2022 12:04:07 +0000

Dear Horiguchi-san,

> go into libpq-fe.h" as Önder mentioned, if I understand what he said

Based on your suggestion, I tried to add like following function to fe-misc.c:

```
int
PQconncheck(PGconn *conn)
{
	/* Raise an ERROR if invalid socket has come */
	if (conn == NULL ||
		PQsocket(conn) == PGINVALID_SOCKET)
		return -1;

	return pqSocketCheck(conn, 1, 1, -1);
}
```

... and replace pgfdw_connection_check_internal() to PQconncheck(),
but it did not work well.
To be exact, pqSocketCheck() said the socket was "readable" and "writable"
even if the connection has been killed.

IIUC, pqSocketCheck () calls pqSocketPoll(),
and in the pqSocketPoll() we poll()'d the POLLIN or POLLOUT event.
But according to [1], we must wait POLLRDHUP event,
so we cannot reuse it straightforward.

If we really want to move the checking function anyway,
we must follow AddWaitEventToSet() and some WaitEventAdjust functions.
I'm not sure whether it is really good.

[1]: https://man7.org/linux/man-pages/man2/poll.2.html

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 17 Oct 2022 12:25:21 +0000

Dear Osumi-san,

> Could you please have a look at this issue ?

Good catch. This was occurred because we disconnected the crashed server.
 
Previously the failed server had been disconnected in the pgfdw_connection_check().
It was OK if the transaction(or statement) was surely cacneled.
But currently the statement might be not canceled because QueryCancelPending might be cleaned up[1].

If we failed to cancel statements and reached pgfdw_xact_callback(),
the connection would not be used because entry->conn is NULL.
That's why you sucseeded to do "COMMIT".
However, the backend did not send "COMMIT" command to the srever,
so inserted data was vanished on remote server.

I understood that we should not call disconnect_pg_server(entry->conn) even if we detect the disconnection.
If should be controlled in Xact callback. This will be modified in next version.

Moreover, I noticed that we should enable timer even if the QueryCancelMessage was not NULL.
If we do not turn on here, the checking will be never enabled again.

[1]: https://www.postgresql.org/message-id/TYAPR01MB5866CE34430424A588F60FC2F55D9%40TYAPR01MB5866.jpnprd01.prod.outlook.com

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "[email-masked]" <[email-masked]>
Date: Mon, 17 Oct 2022 13:01:37 +0000

Dear Osumi-san,

Thanks for reviewing! PSA v17 patchset.

> CheckingRemoteServersCallbackItem can be added to typedefs.list.

Added.

> void *arg)

Added. Additionally, I renamed this function to Unregister...,
this follows other unregister functions.

Fixed.

Fixed.

> so that the message is more aligned with existing error message

Fixed.
...I'm not sure whether this message is still need, because
the disconnection was delegated to XactCallback, and the function did following output:

```
WARNING:  FATAL:  terminating connection due to administrator command
server closed the connection unexpectedly
       This probably means the server terminated abnormally
       before or while processing the request.
```

Fixed.

> .../postgres_fdw/expected/postgres_fdw_1.out

This is needed to pass the test on the windows platform. See:
https://www.postgresql.org/docs/devel/regress-variant.html

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 18 Oct 2022 05:06:34 +0000

On Monday, October 17, 2022 9:25 PM Kuroda, Hayato/黒田 隼人 <[email-masked]> wrote:
> > Then, after waiting for few seconds, the "COMMIT" succeeded like below
> > output, even after the server stop of the worker side.
Hi,

FYI, I quickly rechecked the patch's behavior on that scenario with v17.
Now, the last "COMMIT" above failed expectedly, which looks reasonable behavior.
Thank you for having modified it.

Best Regards,
	Takamichi Osumi

---

From: Önder Kalacı <[email-masked]>
Date: Wed, 19 Oct 2022 17:18:14 +0200

Hi,

> the time interval during checks will be longer.

I expect users typically will have a lot more backends than the servers. We
can have a threshold for spinning a new bg worker (e.g., every 10 servers
gets a new bg worker etc.). Still, I think that'd be an optimization that
is probably not necessary for the majority of the users?

> worker and make it to do a health check.

Again, the definition of "health check" is probably different for me. I'd
expect the health check to happen continuously, ideally keeping track of
how many consecutive times it succeeded and/or last time it
failed/succeeded etc.

A transaction failing with a bad error message (or holding some resources
locally until the transaction is committed) doesn't sound essential to me.
Is there any specific workload are you referring for optimizing to rollback
a transaction earlier if a remote server dies?  What kind of workload would
benefit from that? Maybe there is, but not clear to me and haven't seen
discussed on the thread (sorry if I missed).

I'm trying to understand if we are trying to solve a problem that does not
really exists. I'm bringing this up, because I often deal with
architectures where there is a local node and remote transaction on
different Postgres servers. And, I have not encountered many (or any)
pattern that'd benefit from this change much. In fact, I think, on the
contrary, this might add significant overhead for OLTP type of high query
throughput systems.

> Therefore, we must provide callback functions anyway.
I think providing callback functions is useful for any case. Each fdw (or
in general extension) should be able to provide its own "health check"
function.

Thanks,
Onder KALACI

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 2 Nov 2022 02:43:04 +0000

Dear Önder, all,

Thank you for responding and sorry for late response.

> discussed on the thread (sorry if I missed).

I (and my company) worried about overnight batch processing that
contains some accesses to foreign servers. If the transaction is opened overnight and
one of foreign servers is crashed during it, the transaction must be rollbacked.
But there is a possibility that DBAs do not recognize the crash and
they waste a time until the morning. This problem may affect customer's business.
(It may not be sufficient to check the status from another different server.
DBAs must check the network between the databases, and they may be oversight.)
This is a motivation we thought.

> contrary, this might add significant overhead for OLTP type of high query

As I said above, I did not considered about OLTP system. But I agreed that the current
callback mechanism may have significant overhead.

Actually, we may not decide the correct way to detect the failure now.
Your argument, which operations should be done by BGworker and we record stats about checking,
seems to be efficient and may be smarter but this may be not match my motivation now.
My approach may have large overhead and may be not able to use for OLTP system.

So how about implementing a check function as an SQL function once and update incrementally?
This still satisfy our motivation and it can avoid overhead because we can reduce the number of calling it.
If we decide that we establish a new connection in the checking function, we can refactor the it.
And if we decide that we introduce health-check BGworker, then we can add a process that calls implemented function periodically.

PSA patchset that implemented as an SQL function. I moved the checking function to libpq layer, fe-misc.c.
Note that poll() is used here, it means that currently this function can be used on some limited platforms.

I have added a parameter check_all that controls the scope of to-be-checked servers,
But this is not related with my motivation so we can remove if not needed.

(I have not implemented another version that uses epoll() or kqueue(),
because they seem to be not called on the libpq layer. Do you know any reasons?)

How do you think?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Önder Kalacı <[email-masked]>
Date: Fri, 11 Nov 2022 11:30:33 +0100

Hi Hayota Kuroda,

> This is a motivation we thought.

Thanks for the clarification, I agree that this might be a valid concern
for systems.

> So how about implementing a check function as an SQL function once and

I think a SQL function makes sense.

> reduce the number of calling it.

Yes, it makes sense. Also, it allows other extensions to utilize the new
libpq function, which is neat.

> And if we decide that we introduce health-check BGworker, then we can add
> a process that calls implemented function periodically.
>

Right, your approach doesn't conflict with a more sophisticated approach,
in fact is a useful building block.

> be used on some limited platforms.
I think more experienced hackers could guide us here. I don't see a problem
with that, but checking other functions like pqSocketPoll(), I see that
Postgres uses the select system call. I wonder if we can have something
similar with select? Seems no, but wanted to bring up in case you know more
about that?

> But this is not related with my motivation so we can remove if not needed.

I guess it kind of makes sense to have the flexibility to check all
connections vs only in tx connections.

Though, maybe we should follow a similar approach
to postgres_fdw_disconnect(servername) / postgres_fdw_disconnect_all()

    postgres_fdw_verify_connection_states(servername) /
postgres_fdw_verify_connection_states_all()

That seems like a more natural API considering the other UDFs. You can also
use in combination with postgres_fdw_get_connections()

> because they seem to be not called on the libpq layer. Do you know any
Hmm, I don't know the reason. Is that maybe epoll is available on a smaller
number of platforms and libpq can be used on more platforms as being a
client library?

Now, some comments regarding the v18:

>
> +static int
> +pqConncheck_internal(int sock)
> +{
> +#if (defined(HAVE_POLL) && defined(POLLRDHUP))
> + struct pollfd input_fd;
> +
> + input_fd.fd = sock;
> + input_fd.events = POLLRDHUP;
> + input_fd.revents = 0;
> +
> + poll(&input_fd, 1, 0);
> +
> + return input_fd.revents & POLLRDHUP;

WaitEventSetWaitBlock's* defined(WAIT_USE_POLL)* branch uses the following
check to find WL_SOCKET_CLOSED

#ifdef POLLRDHUP
if ((cur_event->events & WL_SOCKET_CLOSED) &&
(cur_pollfd->revents & (POLLRDHUP | errflags)))
{
/* remote peer closed, or error */
occurred_events->events |= WL_SOCKET_CLOSED;
}
#endif

Where *errflags = POLLHUP | POLLERR | POLLNVAL;*

So, should we also be using all these flags like: input_fd.events =
POLLRDHUP | *errflags*;  ?

+ ereport(ERROR,
> + (errcode(ERRCODE_CONNECTION_FAILURE),
> + errmsg("could not connect to server \"%s\"",
> + server->servername),
> + errdetail("Socket close is detected."),
> + errhint("Please check the health of the server.")));

Is erroring out always necessary? Maybe we should just return true/false,
and let the caller decide what to do? For example, you might want to
rollback to savepoint and retry?  If the caller wants to rollback the whole
transaction, that is possible anyway.

Maybe similar to postgres_fdw_disconnect(), we can warn if the connection
is involved in a tx (e.g., depth > 0)?

new file mode 100644
> index 0000000000..3ce169c837
> --- /dev/null
> +++ b/contrib/postgres_fdw/expected/postgres_fdw_1.out
> @@ -0,0 +1,11615 @@
> +-- ===================================================================
> +-- create FDW objects
> +-- ===================================================================
> +CREATE EXTENSION postgres_fdw;
> +CREATE SERVER testserver1 FOREIGN DATA WRAPPER postgres_fdw;
> +DO $d$
> +    BEGIN

Do we really need this new file or just an oversight in your patch?

Thanks,
Onder KALACI

Software Engineer at Microsoft &
Developing the Citus database extension for PostgreSQL

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 25 Nov 2022 11:41:45 +0000

Dear Önder,

Sorry for my late reply. I understood that we got agreement the basic design of first version. Thanks!
I attached new version patches.

> similar with select? Seems no, but wanted to bring up in case you know more

Here I have been still checking...

> use in combination with postgres_fdw_get_connections()

I think you are right, fixed.

> number of platforms and libpq can be used on more platforms as being a

Here I have been still checking...

Fixed.

> transaction, that is possible anyway.

I changed as you suggested once, but I was not sure we can really do such a thing.
Please see the following case.

```
BEGIN;

SAVEPOINT s;
SELECT 1 FROM ft1 LIMIT 1; -- connect to foreign server

SELECT pg_terminate_backend(pid, 180000) FROM pg_stat_activity
    WHERE application_name = 'fdw_retry_check'; -- kill the remote connection

SELECT 1 FROM ft1 LIMIT 1; -- will fail
-> ERROR:  FATAL:  terminating connection due to administrator command

ROLLBACK TO SAVEPOINT s; -- OK, go back to savepoint...
COMMIT;
-> ERROR:  connection to server "loopback" was lost
```

The test means that even if we go back to the savepoint, 
we could not commit the transaction because postgres_fdw tries to connect to the remove
in pgfdw_xact_callback().

Do we really have a flexibility when the remote connection seems to be closed?
If we do not have, I will revert the change.

> Do we really need this new file or just an oversight in your patch?

Previously there were three file because sometimes the ordering of
output  was changed, but it was no more needed.
Currently this patch contains two comparison files, one is for linux,
and another one is for unsupported OSes like Windows.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Andres Freund <[email-masked]>
Date: Tue, 6 Dec 2022 10:26:38 -0800

Hi,

> I attached new version patches.

This is failing on cfbot / CI, as have prior versions.

https://cirrus-ci.com/github/postgresql-cfbot/postgresql/commitfest%2F40%2F3388

https://api.cirrus-ci.com/v1/artifact/task/6655254493134848/testrun/build/testrun/postgres_fdw/regress/regression.diffs

diff -U3 /tmp/cirrus-ci-build/contrib/postgres_fdw/expected/postgres_fdw.out /tmp/cirrus-ci-build/build/testrun/postgres_fdw/regress/results/postgres_fdw.out
--- /tmp/cirrus-ci-build/contrib/postgres_fdw/expected/postgres_fdw.out	2022-12-06 05:21:33.906116000 +0000
+++ /tmp/cirrus-ci-build/build/testrun/postgres_fdw/regress/results/postgres_fdw.out	2022-12-06 05:27:24.929694000 +0000
@@ -11732,10 +11732,9 @@
 -- execute check function. This should return false on supported platforms,
 -- otherwise return true.
 SELECT postgres_fdw_verify_connection_states('loopback');
-WARNING:  08006
  postgres_fdw_verify_connection_states 
 ---------------------------------------
- f
+ t
 (1 row)
 
 ABORT;

The failure happens everywhere except on linux, so presumably there's some
portability issue in the patch.

I've set the patch as waiting on author for now.

Note that you can test CI in your own repository, as explained in src/tools/ci/README

Greetings,

Andres Freund

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 7 Dec 2022 05:59:14 +0000

Dear Andres,

> This is failing on cfbot / CI, as have prior versions.

Thank you for notifying to me. I attached newer versions.

> portability issue in the patch.

Yes, you are right. This feature can be used only for linux, and
previous patches seemed to have bad files.

> Note that you can test CI in your own repository, as explained in

Thank you for your information. Actually I was in trouble because
I don't have such machines for developing postgres.
I have confirmed that newer patches could pass the test CI.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 20 Dec 2022 01:52:17 +0000

Dear hackers,

PSA rebased patches.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED