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

From: Katsuragi Yuta <[email-masked]>
Date: Wed, 01 Mar 2023 11:34:06 +0900

Hi Kuroda-san,

Thank you for updating the patch!

>> I rethought the pqSocketPoll part. Current interpretation of
>> arguments seems a little bit confusing because a specific pattern
>> of arguments has a different meaning. What do you think about
>> introducing a new argument like `int forConnCheck`? This seems
>> straightforward and readable.
> 
> I think it may be better, so fixed.
> But now we must consider another thing - will we support combination
> of conncheck
> and {read|write} or timeout? Especially about timeout, if someone
> calls pqSocketPoll()
> with forConnCheck = 1 and end_time = -1, the process may be stuck
> because it waits
> till socket peer closed connection.
> Currently the forConnCheck can be specified with other request, but
> timeout must be zero.

Yes, we need to consider these.
I'm wondering whether we need a special care for the combination
of event and timeout. Surely, if forConnCheck is set and end_time = -1,
pqSocketPoll blocks until the connection close. However, the
behavior matches the meaning of the arguments and does not seem
confusing (also not an error state). Do we need to restrict this
kind of usage in the pqSocketPoll side? I think like the following
might be fine.

```
  	if (!forRead && !forWrite)
	{
		if (!(forConnCheck && PQconnCheckable()))
			return 0;
	}
```

> While making the patch, I come up with idea that
> postgres_fdw_verify_connection_states*
> returns NULL if PQconnCheck() cannot work on this platform. This can be 
> done by
> adding PQconnCheckable(). It may be reasonable because the checking is
> not really
> done on this environment. How do you think?

I agree with you.

Followings are comments for v33. Please check.
0001:
1. the comment of PQconnCheck
+/*
+ * Check whether the socket peer closed connection or not.
+ *

Check whether the socket peer closed 'the' connection or not?

2. the comment of pqSocketCheck
- * or both.  Returns >0 if one or more conditions are met, 0 if it 
timed
- * out, -1 if an error occurred.
+ * or both. Moreover, this function can check the health of socket on 
some
+ * limited platforms if end_time is 0.

the health of socket -> the health of the connection?
if end_time is 0 -> if forConnCehck is specified?

3. the comment of pqSocketPoll
- * 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.
+ * Moreover, this function can check the health of socket on some 
limited
+ * platforms if end_time is 0.

the health of socket -> the health of the connection?
if end_time is 0 -> if forConnCehck is specified?

4. the code of pqSocketPoll
+#if defined(POLLRDHUP)
+	if (forConnCheck)
+		input_fd.events |= POLLRDHUP;
+#endif

I think it is better to use PQconnCheckable() to remove the macro.

5. the code of pqSocketCheck and the definition of pqSocketPoll
-		result = pqSocketPoll(conn->sock, forRead, forWrite, end_time);
+		result = pqSocketPoll(conn->sock, forRead, forWrite, forConnCheck, 
end_time);

-pqSocketPoll(int sock, int forRead, int forWrite, time_t end_time)
+pqSocketPoll(int sock, int forRead, int forWrite, int forConnCheck, 
time_t end_time)

Should these be divided into two lines?

6. the comment of verify_cached_connections
+ * This function emits warnings if a disconnection is found, and this 
returns
+ * false only when the lastly verified server seems to be disconnected.

It seems better to write the case where this function returns
true.

7. the comment of postgres_fdw_verify_connection_states
+ * This function emits a warning if a disconnection is found. This 
returns
+ * false only when the verified server seems to be disconnected, and 
reutrns
+ * NULL if the connection check had not been done.

It seems better to write the case where this function returns
true.

8. the code of
+				(errcode(ERRCODE_CONNECTION_FAILURE),
+				 errmsg("%s", str.data),
+				 errdetail("Socket close is detected."),
+				 errhint("Plsease check the health of server.")));

Is it better to use "Connection close is detected" rather than
"Socket close is detected"?

9. the document of postgres_fdw
The document of postgres_fdw_verify_connection_states_* is a little
bit old. Could you update it?

regards,

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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Fri, 3 Mar 2023 12:24:52 +0000

Hi Katsuragi-san,

Thank you for reviewing! PSA new version.

> 		if (!(forConnCheck && PQconnCheckable()))

Seems right, I was too pessimistic. Fixed.

> > done on this environment. How do you think?

Changed.

> Check whether the socket peer closed 'the' connection or not?

Changed.

> if end_time is 0 -> if forConnCehck is specified?

Fixed.

> if end_time is 0 -> if forConnCehck is specified?

Fixed.

> I think it is better to use PQconnCheckable() to remove the macro.

IIUC the macro is needed. In FreeBSD, macOS and other platforms do not have the
macro POLLRDHUP so they cannot compile. I checked by my CI and got following error.

```
...
FAILED: src/interfaces/libpq/libpq.a.p/fe-misc.c.o 
...
../src/interfaces/libpq/fe-misc.c:1149:22: error: use of undeclared identifier 'POLLRDHUP'
                input_fd.events |= POLLRDHUP;
````

It must be invisible from them.

> Should these be divided into two lines?

pgindent did not say anything about them, but I divided. 

> + * false only when the lastly verified server seems to be disconnected.

Updated. I think comments were not correct, so these were also fixed.

> "Socket close is detected"?

Changed.

> bit old. Could you update it?

Updated. IIUC postgres_fdw_verify_connection_states returns

* true, if the connection is verified.
* false, if the connection seems to be disconnected.
* NULL, if this is not the supported platform or connection has not been established.

And postgres_fdw_verify_connection_states_all returns

* true if all the connections are verified.
* false, if one of connections seems to be disconnected.
* NULL, if this is not the supported platform or this backend has never established connections

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Katsuragi Yuta <[email-masked]>
Date: Mon, 06 Mar 2023 19:13:58 +0900

Hi Kuroda-san,

> It must be invisible from them.

Sorry, my mistake...

> never established connections

I think 'backend has never established connections' is a little bit 
strong.
I think the following cases also return NULL. The case where a
connection was established, however the connection is now closed
by the postgres_fdw_disconnect() or something. NULL is also returned if
the connection is invalidated. So, I think 'NULL, if no valid
connection is established or the function is not supported on
the platform.' is better. What do you think?

Followings are my comments for v34. Please check.

0001:
1. the document of pqConnCheck
+       <literal>0</literal> if the socket is valid, and returns 
<literal>-1</literal>
+       if the connection has already been closed or an error has 
occurred.

1.1 if the socket is valid -> returns 0 if the 'connection is not 
closed'?

1.2 returns -1 if the connection has already been closed  <- Let me ask 
a question.
Isn't this a situation where we would like to check using this
function? Is 'error has occurred' insufficient?

2. the comment of pqSocketCheck
+ * means that the socket has not matched POLLRDHUP event and the socket 
has
+ * still survived.

socket has still survived -> connection is not closed by the socket 
peer?

3. the comment of pqSocketPoll
+ * returned and forConnCheck is requested, it means that the socket has 
not
+ * matched POLLRDHUP event and the socket has still survived.

socket has still survived -> connection is not closed by the socket 
peer?

0002:
4. the comment of verify_cached_connections
+ * This function emits warnings if a disconnection is found. This 
return true
+ * if disconnections cannot be found, otherwise return false.

return ture -> return's' true
return false -> return's' false

5. the comment of postgres_fdw_verify_connection_states
+ * if the local session seems to be disconnected from other servers. 
NULL is
+ * returned if a connection to the specified foreign server has not 
been
+ * established yet, or this function is not available on this platform.

Considering the above discussion, 'NULL is returned if a valid
connection to the specified foreign server is not established or
this function...' seems better. What do you think?

6. the document of postgres_fdw_verify_connection_states
  <literal>NULL</literal>
+      is returned if a connection to the specified foreign server has 
not been
+      established yet, or this function is not available on this 
platform

The same as comment no.5.

7. the document of postgres_fdw_verify_connection_states_all
<literal>NULL</literal>
+      is returned if the local session does not have connection caches, 
or this
+      function is not available on this platform.

I think there is a case where a connection cache exists but valid
connections do not exist and NULL is returned (disconnection case).
Almost the same document as the postgres_fdw_verify_connection_states
case (comment no.5) seems better.

8. the document of postgres_fdw_can_verify_connection_states
+      This function checks whether 
<function>postgres_fdw_verify_connection_states</function>
+      and <function>postgres_fdw_verify_connection_states</function> 
work well

Should the latter (or former) postgres_fdw_verify_connection_states be
postgres_fdw_verify_connection_states_all?

regards,