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

From: Fujii Masao <[email-masked]>
Date: Wed, 19 Jan 2022 00:42:05 +0900

On 2021/12/15 15:40, [email-masked] wrote:
> 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,

What about starting the timeout in GetConnection(), instead?

> I attached which implements that.

v05_0004_add_tests.patch failed to be applied to the master. Could you rebase it?

-        This option is currently available only on systems that support the
-        non-standard <symbol>POLLRDHUP</symbol> extension to the
-        <symbol>poll</symbol> system call, including Linux.
+        This option relies on kernel events exposed by Linux, macOS, illumos
+        and the BSD family of operating systems, and is not currently available
+        on other systems.

The above change is included in both v5-0003-Use-WL_SOCKET_CLOSED-for-client_connection_check_.patch and v05_0002_add_doc.patch. If it should be in the former patch, it should be removed from your patch v05_0002_add_doc.patch.

There seems no user of UnregisterCheckingRemoteServersCallback(). So how about removing it?

Regards,

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

---

From: "[email-masked]" <[email-masked]>
Date: Thu, 20 Jan 2022 08:21:42 +0000

Dear Fujii-san,

Thank you for your interest! I'll post new version within several days.

> What about starting the timeout in GetConnection(), instead?

Did you said about a function in postgres_fdw/connection.c?
In my understanding that means that the timeout should be enabled or disabled
by each FDW extensions.
I did not find bad cases for that, so I'll change like that and make new APIs.

> v05_0004_add_tests.patch failed to be applied to the master. Could you rebase it?

It's caused because a testcase was added in postgres_fdw. Will rebase.

> The above change is included in both
> v5-0003-Use-WL_SOCKET_CLOSED-for-client_connection_check_.patch and
> v05_0002_add_doc.patch. If it should be in the former patch, it should be removed
> from your patch v05_0002_add_doc.patch.

I confused about doc-patch. Sorry for inconvenience.

> There seems no user of UnregisterCheckingRemoteServersCallback(). So how

Previously I kept the API for any other extensions, but I cannot find use cases.
Will remove.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "[email-masked]" <[email-masked]>
Date: Fri, 21 Jan 2022 06:08:03 +0000

Dear Fujii-san, Zhihong,

I attached the latest version.
The biggest change is that callbacks are no longer un-registered at the end of transactions.
FDW developer must enable or disable timeout instead, via new APIs.

The timer will be turned on when:
* new GUC is >= 0, and
* anyone calls TryEnableRemoteServerCheckingTimeout().

I think this version is reduced overhead, but it might not be developer friendly...

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Fujii Masao <[email-masked]>
Date: Sun, 23 Jan 2022 14:36:02 +0900

> I attached the latest version.

Thanks for updating the patch!

+int
+TryDisableRemoteServerCheckingTimeout(void)

When more than one FDWs are used, even if one FDW calls this function to disable the timeout, its remote-server-check-callback can still be called. Is this OK?
Please imagine the case where two FDWs are used and they registered their callbacks. Even when one FDW calls TryDisableRemoteServerCheckingTimeout(), if another FDW has not called that yet, the timeout is still being enabled. If the timeout is triggered during that period, even the callback registered by the FDW that has already called TryDisableRemoteServerCheckingTimeout() would be called.

+			if (remote_servers_connection_check_interval > 0)
+			{
+				CallCheckingRemoteServersCallbacks();
+				enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT,
+									 remote_servers_connection_check_interval);

LockErrorCleanup() needs to be called before reporting the error, doesn't it?

This can cause an error even while DoingCommandRead == true. Is that safe?

> The timer will be turned on when:

This can cause the timeout to be enabled even when no remote transaction is started?

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 1 Feb 2022 04:37:07 +0000

Dear Fujii-san,

Thank you for reviewing! I attached the latest version.

> When more than one FDWs are used, even if one FDW calls this function to
> disable the timeout, its remote-server-check-callback can still be called. Is this
> OK?
> Please imagine the case where two FDWs are used and they registered their
> callbacks. Even when one FDW calls TryDisableRemoteServerCheckingTimeout(),
> if another FDW has not called that yet, the timeout is still being enabled. If the
> timeout is triggered during that period, even the callback registered by the FDW
> that has already called TryDisableRemoteServerCheckingTimeout() would be
> called.

Indeed and it should be avoided. I added a counter to CheckingRemoteServersCallbackItem.
The register function returns the registered item, and it must be set as the argument for
enable and disable functions.
Callback functions will be called when item->counter is larger than zero.

> +
> 	enable_timeout_after(CHECKING_REMOTE_SERVERS_TIMEOUT,
> +
> remote_servers_connection_check_interval);
> 

You are right. I think this suggests that error-reporting should be done in the core layer.
For meaningful error reporting, I changed a callback specification
that it should return ForeignServer* which points to downed remote server.

> This can cause an error even while DoingCommandRead == true. Is that safe?

I read codes again and I think it is not safe. It is OK when whereToSendOutput is DestRemote,
but not good in InteractiveBackend().

I changed that if-statement for CheckingRemoteServersTimeoutPending is moved just after
ClientConnectionLost, because the that may throw a FATAL error and disconnect from client.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Fujii Masao <[email-masked]>
Date: Tue, 1 Feb 2022 23:51:54 +0900

> Thank you for reviewing! I attached the latest version.

Thanks!

> Callback functions will be called when item->counter is larger than zero.

This logic sounds complicated to me. I'm afraid that FDW developers may a bit easily misunderstand the logic and make the bug in their FDW.

Isn't it simpler to just disable the timeout in core whenever the transaction ends whether committed or aborted, like statement_timeout is disabled after each command? For example, call something like DisableForeignCheckTimeout() in CommitTransaction() etc.

> that it should return ForeignServer* which points to downed remote server.

This approach seems to assume that FDW must manage all the ForeignServer information so that the callback can return it. Is this assumption valid for all the FDW?

How about making FDW trigger a query cancel interrupt by signaling SIGINT to the backend, instead?

Regards,

---

From: "[email-masked]" <[email-masked]>
Date: Wed, 2 Feb 2022 12:37:42 +0000

Dear Fujii-san,

Thank you for good suggestions.

> This logic sounds complicated to me. I'm afraid that FDW developers may a bit
> easily misunderstand the logic and make the bug in their FDW.
> Isn't it simpler to just disable the timeout in core whenever the transaction ends
> whether committed or aborted, like statement_timeout is disabled after each
> command? For example, call something like DisableForeignCheckTimeout() in
> CommitTransaction() etc.

Your idea is that stop the timer at the end of each transactions, right?
I had not adopted that because I thought some developers might want not to stop the timer
even if transactions ends. It caused complexed situation and not have good usecase, however,
so your logic was implemented.

> This approach seems to assume that FDW must manage all the ForeignServer
> information so that the callback can return it. Is this assumption valid for all the
> FDW?

Not sure, the assumption might be too optimistic. 
mysql_fdw can easily return ForeignServer* because it caches serverid,
but dblink and maybe oracle_fdw cannot.

> How about making FDW trigger a query cancel interrupt by signaling SIGINT to

I understood that the error should be caught by QueryCancelPending.
Could you check 0003? Does it follow that?

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "[email-masked]" <[email-masked]>
Date: Tue, 15 Feb 2022 12:05:40 +0000

Dear Hackers,

I found patches we depend have been committed, so rebased.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=50e570a59e7f86bb41f029a66b781fc79b8d50f1

In this version there is a little bit change in part of postgres_fdw.
A system checking by WaitEventSetCanReportClosed() is added
because some OSes cannot wait WL_SOCKET_CLOSED event.
Note that test cannot be added in the regression test
because cfbot may be not happy.
In my environment a test that contained in previous patches works well.

0001, 0002 is not changed from previous version. 

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Wed, 16 Feb 2022 11:50:58 +0900 (JST)

At Tue, 1 Feb 2022 23:51:54 +0900, Fujii Masao <[email-masked]> wrote in 
> This logic sounds complicated to me. I'm afraid that FDW developers
> may a bit easily misunderstand the logic and make the bug in their
> FDW.

> Isn't it simpler to just disable the timeout in core whenever the
> transaction ends whether committed or aborted, like statement_timeout
> is disabled after each command? For example, call something like
> DisableForeignCheckTimeout() in CommitTransaction() etc.

> This approach seems to assume that FDW must manage all the
> ForeignServer information so that the callback can return it. Is this
> assumption valid for all the FDW?

FWIW, I'm not sure this feature necessarily requires core support
dedicated to FDWs.  The core have USER_TIMEOUT feature already and
FDWs are not necessarily connection based.  It seems better if FDWs
can implement health check feature without core support and it seems
possible.  Or at least the core feature should be more generic and
simpler. Why don't we just expose InTransactionHealthCheckCallbacks or
something and operating functions on it?

> SIGINT to the backend, instead?

Mmm. AFAICS the running command will stop with "canceling statement
due to user request", which is a hoax.  We need a more decent message
there.

I understand that the motive of this patch is "to avoid wasted long
local work when fdw-connection dies".  In regard to the workload in
your first mail, it is easily avoided by ending the transaction as soon
as remote access ends. This feature doesn't work for the case "begin;
<long local query>; <fdw access>". But the same measure also works in
that case.  So the only case where this feature is useful is "begin;
<fdw-access>; <some long work>; <fdw-access>; end;".  But in the first
place how frequently do you expecting remote-connection close happens?
If that happens so frequently, you might need to recheck the system
health before implementing this feature.  Since it is correctly
detected when something really went wrong, I feel that it is a bit too
complex for the usefulness especially for the core part.

In conclusion, as my humble opinion I would like to propose to reduce
this feature to:

- Just periodically check health (in any aspect) of all live
  connections regardless of the session state.

- If an existing connection is found to be dead, just try canceling
  the query (or sending query cancel).

One issue with it is how to show the decent message for the query
cancel, but maybe we can have a global variable that suggests the
reason for the cancel.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center