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

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Tue, 10 Jan 2023 16:26:00 +0000

Dear tom,

> I think that it's a really bad idea to require postgres_fdw.sql
> to have two expected-files: that will be a maintenance nightmare.
> Please put whatever it is that needs a variant expected-file
> into its own, hopefully very small and seldom-changed, test script.
> Or rethink whether you really need a test case that has
> platform-dependent output.

Thank you for giving the suggestion. I agreed your saying and modifed that.

I added new functions on the libpq and postgres-fdw layer that check whether the
checking works well or not. In the test, at first, the platform is checked and
the checking function is called only when it is supported.

An alternative approach is that PQCanConncheck() can be combined with PQConncheck().
This can reduce the libpq function, but we must define another returned value to
the function like -2. I was not sure which approach was better.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Ted Yu <[email-masked]>
Date: Tue, 10 Jan 2023 08:38:13 -0800

> the function like -2. I was not sure which approach was better.

Hi,

+       /* quick exit if connection cache has been not initialized yet. */

been not initialized -> not been initialized

+
(errcode(ERRCODE_CONNECTION_FAILURE),
+                                                errmsg("could not connect
to server \"%s\"",

Currently each server which is not connected would log a warning.
Is it better to concatenate names for such servers and log one line ? This
would be cleaner when there are multiple such servers.

Cheers

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 11 Jan 2023 07:37:00 +0000

Dear Ted,

Thank you for reviewing! PSA new version.

> been not initialized -> not been initialized

Fixed.

> +                                               (errcode(ERRCODE_CONNECTION_FAILURE),
> +                                                errmsg("could not connect to server \"%s\"",
>
> Is it better to concatenate names for such servers and log one line ? This would be cleaner when there are multiple such servers.

Sounds good, fixed as you said. The following shows the case that two disconnections
are detected by postgres_fdw_verify_connection_states_all().

```
postgres=*# select postgres_fdw_verify_connection_states_all ();
WARNING:  could not connect to server "my_external_server2", "my_external_server"
DETAIL:  Socket close is detected.
HINT:  Plsease check the health of server.
 postgres_fdw_verify_connection_states_all 
-------------------------------------------
 f
(1 row)
```

Currently, the name of servers is concatenated without doing unique checks. IIUC
a backend process cannot connect to the same foreign server by using different
user mapping, so there is no possibility that the same name appears twice.
If the user mapping is altered in the transaction, the cache entry is invalidated
and will not be checked.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 11 Jan 2023 10:04:17 +0000

Dear hackers,

I was not sure, but the cfbot could not be accepted the previous version.
I made the patch again from HEAD(5f6401) without any changes,
so I did not count up the version number.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Katsuragi Yuta <[email-masked]>
Date: Fri, 20 Jan 2023 19:41:41 +0900

Hi,

Thanks for the patch!
I read the patch v24. These are my comments. Please check.

## v24-0001-Add-PQConncheck-and-PQCanConncheck-to-libpq.patch
+    <varlistentry id="libpq-PQCanConncheck">
+     
<term><function>PQCanConncheck</function><indexterm><primary>PQCanConncheck</primary></indexterm></term>
+     <listitem>
+      <para>
+       Returns the status of the socket.

Is this description right? I think this description is for
PQConncheck. Something like "Checks whether PQConncheck is
available on this platform." seems better.

+/* Check whether the postgres server is still alive or not */
+extern int PQConncheck(PGconn *conn);
+extern int PQCanConncheck(void);

Should the names of these functions be in the form of PQconnCheck?
Not PQConncheck (c.f. The section of fe-misc.c in libpq-fe.h).

## v24-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch
+PG_FUNCTION_INFO_V1(postgres_fdw_verify_connection_states);
+PG_FUNCTION_INFO_V1(postgres_fdw_verify_connection_states_all);
+PG_FUNCTION_INFO_V1(postgres_fdw_can_verify_connection_states);

This patch adds new functions to postgres_fdw for PostgreSQL 16.
So, I think it is necessary to update the version of postgres_fdw (v1.1 
to v1.2).

+    <term><function>postgres_fdw_verify_connection_states_all() returns 
boolean</function></term>
+    <listitem>
+     <para>
+      This function checks the status of remote connections that are 
established
+      by <filename>postgres_fdw</filename> from the local session to 
the foreign
+      servers. This check is performed by polling the socket and allows

It seems better to add a description that states this function
checks all the connections. Like the description of
postgres_fdw_disconnect_all(). For example, "This function
checks the status of 'all the' remote connections..."?

regards,

-- 
Katsuragi Yuta
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Sat, 21 Jan 2023 12:03:07 +0000

Dear Katsuragi-san,

Thank you for reviewing! PSA new patch set.

> available on this platform." seems better.

It might be copy-and-paste error. Thanks for reporting.
According to other parts, the sentence should be started like "Returns ...".
So I followed the style and did cosmetic change.

> Not PQConncheck (c.f. The section of fe-misc.c in libpq-fe.h).

Agreed, fixed.

> So, I think it is necessary to update the version of postgres_fdw (v1.1

I checked postgres_fdw commit log, and it seemed that the version was
updated when SQL functions are added. Fixed.

> checks the status of 'all the' remote connections..."?

I checked the docs and fixed. Moreover, some inconsistent statements were
also fixed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Ted Yu <[email-masked]>
Date: Sat, 21 Jan 2023 04:33:09 -0800

> I checked the docs and fixed. Moreover, some inconsistent statements were
For v25-0001-Add-PQConnCheck-and-PQCanConnCheck-to-libpq.patch ,
`pqConnCheck_internal` only has one caller which is quite short.
Can pqConnCheck_internal and PQConnCheck be merged into one func ?

+int
+PQCanConnCheck(void)

It seems the return value should be of bool type.

Cheers

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 23 Jan 2023 04:57:24 +0000

> Thank you for reviewing! PSA new patch set.

Sorry, I missed the updated file in the patch. New version will be posted soon.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 23 Jan 2023 05:40:34 +0000

Dear Ted,

Thanks for reviewing! PSA new version.

> For v25-0001-Add-PQConnCheck-and-PQCanConnCheck-to-libpq.patch , `pqConnCheck_internal` only has one caller which is quite short.

I divided the function for feature expandability. Currently it works on linux platform,
but the limitation should be removed in future and internal function will be longer.
Therefore I want to keep this style.

> It seems the return value should be of bool type.

I slightly changed the returned value like true/false. But IIUC libpq functions
cannot define as "bool" datatype. E.g. PQconnectionNeedsPassword() returns true/false,
but the function is defined as int.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Katsuragi Yuta <[email-masked]>
Date: Wed, 25 Jan 2023 17:15:10 +0900

> but the function is defined as int.

Thank you for updating the patch!

+/* Check whether the postgres server is still alive or not */
+extern int PQConnCheck(PGconn *conn);
+extern int PQCanConnCheck(void);

Aren't these PQconnCheck and PQcanConnCheck? I think the first letter
following 'PQ' should be lower case.

regards.

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 25 Jan 2023 11:07:16 +0000

Dear Katsuragi-san,

Thank you for reading the patch! PSA new version.

> following 'PQ' should be lower case.

I cannot find any rules about it, but seems right. Fixed.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 27 Jan 2023 03:50:18 +0000

Dear hackers,

I have updated my patch for error handling and kqueue() support.
Actually I do not have BSD-like machine, but I developed by using github CICD.
I think at first we should focus on 0001-0003, and then work for 0004.

Followings are change notes and my analysis.

0001

* Fix missed replacements from PQConnCheck() to PQconnCheck().
* Error handling was improved. Now we can detect the failure of poll() and return -1 at that time.
* I thought we don't have to add select(2) in PQconnCheck(). According to man page [1],
  select(2) can be only used for watch whether the status is readable, writable, or exceptional condition.
  It means that select() does not have an event corresponding to POLLRDHUP.

0002, 0003
Not changed

0004

* Add kqueue(2) support() for BSD family.
* I did not add epoll() support, because it can be used only on linux and such systems have POLLRDHUP instead. 
 checked other codes in libpq, and they do not use epoll(). We can see that such an event does not specified in POSIX [2]
 and it can be used for linux only. I decided to use poll() as much as possible to keep the policy.
* While coding, I found that there are no good timing to close the kernel event queue.
 It means that the lifetime of kqueue becomes same as the client program and occupy the small memory forever.
 I'm not sure it can be accepted.

[1]: https://man7.org/linux/man-pages/man2/select.2.html
[2]: https://pubs.opengroup.org/onlinepubs/9699919799/functions/poll.html

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 27 Jan 2023 06:57:01 +0000

I found cfbot failure, PSA fixed version.
Sorry for noise.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Katsuragi Yuta <[email-masked]>
Date: Wed, 08 Feb 2023 16:10:25 +0900

> I found cfbot failure, PSA fixed version.

Hi Kuroda-san,

Thank you for updating the patch! Sorry for the late reply.

0001:
+       while (result < 0 && errno == EINTR);
+
+       if (result < 0)
+                       return -1;

this `return -1` is not indented properly.

0002:
+    <term><function>postgres_fdw_verify_connection_states(server_name 
text) returns boolean</function></term>
...
+      extension to the <symbol>poll</symbol> system call, including 
Linux. This
+      returns <literal>true</literal> if checked connections are still 
valid,
+      or the checking is not supported on this platform. 
<literal>false</literal>
+      is returned if the local session seems to be disconnected from 
other
+      servers. Example usage of the function:

Here, 'still valid' seems a little bit confusing because this 'valid' is 
not
the same as postgres_fdw_get_connections's 'valid' [1].

Should 'still valid' be 'existing connection is not closed by the remote 
peer'?
But this description does not cover all the cases where this function 
returns true...
I think one choice is to write all the cases like 'returns true if any 
of the
following condition is satisfied. 1) existing connection is not closed 
by the
remote peer 2) there is no connection for specified server yet 3) the 
checking
is not supported...'. If my understanding is not correct, please point 
it out.

BTW, is it reasonable to return true if ConnectionHash is not 
initialized or
there is no ConnCacheEntry for specified remote server? What do you 
think
about returning NULL in that case?

0003:
I think it is better that the test covers all the new functions.
How about adding a test for postgres_fdw_verify_connection_states_all?

+-- ===================================================================
+-- test for postgres_fdw_verify_foreign_servers function
+-- ===================================================================

Writing all the functions' name like 'test for 
postgres_fdw_verify_connection_states
and postgres_fdw_can_verify_connection_states' looks straightforward.
What do you think about this?

0004:
Sorry, I have not read 0004. I will read.

[1]: 
https://github.com/postgres/postgres/blob/master/doc/src/sgml/postgres-fdw.sgml#L764-L765

regards,

---

From: Kyotaro Horiguchi <[email-masked]>
Date: Thu, 09 Feb 2023 11:31:49 +0900 (JST)

At Fri, 27 Jan 2023 06:57:01 +0000, "Hayato Kuroda (Fujitsu)" <[email-masked]> wrote in 
> I found cfbot failure, PSA fixed version.

+       Unlike <xref linkend="libpq-PQstatus"/>, this function checks socket
+       health. This check is performed by polling the socket. This function is
+       currently available only on systems that support the non-standard
+       <symbol>POLLRDHUP</symbol> extension to the <symbol>poll</symbol> system

I find it quite confusing that we have pqSocketCheck and PQconnCheck,
that does almost the same thing.. Since pqSocketCheck is a static
function, we can modify the function as we like.

I still don't understand why we need pqconnCheck_internal separate
from pqSocketPoll(), and PQconnCheck from pqSocketCheck.

https://www.postgresql.org/message-id/flat/TYAPR01MB58665BF23D38EDF10028DE2AF5299%40TYAPR01MB5866.jpnprd01.prod.outlook.com#47d21431bf9fa94f763c824f6e81fa54
> 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.

Yeah, I didn't suggest to use the function as-is. Couldn't we extend
the fucntion by letting it accept end_time = 0 && !forRead &&
!forWrite, not causing side effects?

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Thu, 9 Feb 2023 14:39:34 +0000

Dear Katsuragi-san,

Thank you for reviewing! PSA new version patches.

> this `return -1` is not indented properly.

This part is no longer needed. Please see another discussion[1].

> is not supported...'. If my understanding is not correct, please point

Modified like you pointed out.

> about returning NULL in that case?

I'm not sure which one is better, but modified accordingly.

> +--
> ===================================================
> ================
> +--
> ===================================================
> ================
> 

Added.

> Sorry, I have not read 0004. I will read.

No problem:-).

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

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Katsuragi Yuta <[email-masked]>
Date: Fri, 17 Feb 2023 17:43:13 +0900

> Thank you for reviewing! PSA new version patches.

Thank you for updating the patch! These are my comments,
please check.

0001:
Extending pqSocketPoll seems to be a better way because we can
avoid having multiple similar functions. I also would like to hear
horiguchi-san's opinion whether this matches his expectation.
Improvements of pqSocketPoll/pqSocketCheck is discussed in this
thread[1]. I'm concerned with the discussion.

As for the function's name, what do you think about keeping
current name (pqSocketCheck)? pqSocketIsReadable... describes
the functionality very well though.

pqConnCheck seems to be a family of pqReadReady or pqWriteRedy,
so how about placing pqConnCheck below them?

+ * Moreover, when neither forRead nor forWrite is requested and timeout 
is
+ * disabled, try to check the health of socket.
Isn't it better to put the comment on how the arguments are
interpreted before the description of return value?

+#if defined(POLLRDHUP)
+			input_fd.events = POLLRDHUP | POLLHUP | POLLNVAL;
...
+	input_fd.events |= POLLERR;
To my understanding, POLLHUP, POLLNVAL and POLLERR are ignored
in event. Are they necessary?

0002:
As for the return value of postgres_fdw_verify_connection_states,
what do you think about returning NULL when connection-checking
is not performed? I think there are two cases 1) ConnectionHash
is not initialized or 2) connection is not found for specified
server name, That is, no entry passes the first if statement below
(case 2)).

```
          if (all || entry->serverid == serverid)
          {
              if (PQconnCheck(entry->conn))
              {
```

0004:
I'm wondering if we should add kqueue support in this version,
because adding kqueue support introduces additional things to
be considered. What do you think about focusing on the main
functionality using poll in this patch and going for kqueue
support after this patch?

[1]: 
https://www.postgresql.org/message-id/20230209.115009.2229702014236187289.horikyota.ntt%40gmail.com

regards,

---

From: Peter Smith <[email-masked]>
Date: Mon, 20 Feb 2023 12:23:32 +1100

Here is a code review only for patch v31-0001.

======
General Comment

1.
PQcanConnCheck seemed like a strange API name. Maybe it can have the
same prefix as the other?

e.g.

- PQconnCheck()
- PGconnCheckSupported()

or

- PQconnCheck()
- PGconnCheckable()

======
Commit Message

2.
PqconnCheck() function allows to check the status of socket by polling
the socket. This function is currently available only on systems that
support the non-standard POLLRDHUP extension to the poll system call,
including Linux.

PqcanConnCheck() checks whether above function is available or not.

~

2a.
"status of socket" --> "status of the connection"

~

2b.
"above function" --> "the above function"

======
doc/src/sgml/libpq.sgml

3. PQconnCheck

Returns the health of the socket.

int PQconnCheck(PGconn *conn);

Unlike PQstatus, this function checks socket health. This check is
performed by polling the socket. This function is currently available
only on systems that support the non-standard POLLRDHUP extension to
the poll system call, including Linux. PQconnCheck returns greater
than zero if the remote peer seems to be closed, returns 0 if the
socket is valid, and returns -1 if the connection has been already
invalid or an error is error occurred.

~

3a.
Should these descriptions be referring to the health of the
*connection* rather than the health of the socket?

~

3b.
"has been already invalid" ?? wording

~~~

4. PQcanConnCheck

Returns whether PQconnCheck is available on this platform.
PQcanConnCheck returns 1 if the function is supported, otherwise
returns 0.

~

I thought this should be worded using "true" and "false" same as other
boolean functions on this page.

SUGGESTION
Returns true (1) or false (0) to indicate if the PQconnCheck function
is supported on this platform.

======
src/interfaces/libpq/fe-misc.c

5.
-static int pqSocketCheck(PGconn *conn, int forRead, int forWrite,
-   time_t end_time);
+static int pqSocketIsReadableOrWritableOrValid(PGconn *conn, int forRead,
+ int forWrite, time_t end_time);

I was not 100% sure overloading this API is the right thing to do.
Doesn't this introduce a subtle side-effect on some of the existing
callers? e.g. Previously pqWaitTimed would ALWAYS return 0 if
forRead/forWrite were both false. But now other return values like
errors will be possible. Is that OK?

~~~

6. pqSocketPoll

/*
 * Check a file descriptor for read and/or write data, possibly waiting.
 * If neither forRead nor forWrite are set, immediately return a timeout
 * condition (without waiting).  Return >0 if condition is met, 0
 * if a timeout occurred, -1 if an error or interrupt occurred.
 *
 * Timeout is infinite if end_time is -1.  Timeout is immediate (no blocking)
 * if end_time is 0 (or indeed, any time before now).
 *
 * Moreover, when neither forRead nor forWrite is requested and timeout is
 * disabled, try to check the health of socket.
 */

The new comment "Moreover..." is contrary to the earlier part of the
same comment which already said, "If neither forRead nor forWrite are
set, immediately return a timeout condition (without waiting)."

There might be side-effects to previous/existing callers of this
function (e.g. pqWaitTimed via pqSocketCheck)

~~~

7.
  if (!forRead && !forWrite)
- return 0;
+ {
+ /* Try to check the health if requested */
+ if (end_time == 0)
+#if defined(POLLRDHUP)
+ input_fd.events = POLLRDHUP | POLLHUP | POLLNVAL;
+#else
+ return 0;
+#endif /* defined(POLLRDHUP) */
+ else
+ return 0;
+ }

FYI - I think the new code can be simpler without needing #else by
calling your other new function.

SUGGESTION
if (!forRead && !forWrite)
{
if (!PQcanConnCheck() || end_time != 0)
return 0;

/* Check the connection health when end_time is 0 */
Assert(PQcanConnCheck() && end_time == 0);
#if defined(POLLRDHUP)
input_fd.events = POLLRDHUP | POLLHUP | POLLNVAL;
#endif
}

~~~

8. PQconnCheck
+/*
+ * Check whether PQconnCheck() can work well on this platform.
+ *
+ * Returns 1 if this can use PQconnCheck(), otherwise 0.
+ */
+int
+PQcanConnCheck(void)
+{
+#if (defined(HAVE_POLL) && defined(POLLRDHUP))
+ return true;
+#else
+ return false;
+#endif
+}

~

8a.
"can work well" --> "works"

~

8b.
Maybe better to say "true (1)" and "otherwise false (0)"

------
Kind Regards,
Peter Smith.
Fujitsu Australia