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

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Mon, 20 Feb 2023 06:42:32 +0000

Dear Katsuragi-san,

Thank you for reviewing! PSA new version.

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

I checked the thread and seems correct. I can post +1 to the thread.
And the modification will be automatically reflected to the feature
if we use the same function, I thought.

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

No objection, I can keep the shorter name.

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

Moved.

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

I read man poll(3) again, and I found that these event is ignored when
it sets to the events attribute. So removed.

> 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))
>               {
> ```

I think in that case we can follow postgres_fdw_disconnect().
About postgres_fdw_disconnect(), if the given server_name does not exist,
an error is reported. This is a current behavior so I want to keep it.
Besides, I added the description to document.

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

I think it is better because it can keep patches smaller. So I stopped attaching 0004.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: Peter Smith <[email-masked]>
Date: Tue, 21 Feb 2023 18:11:06 +1100

Here are some review comments for v32-0001.

======
Commit message

1.
PQconnCheck() function allows to check the status of the 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.

~

(missed fix from previous review)

"status of the socket" --> "status of the connection"

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

2. PQconnCheck
+      <para>
+       This function check the health of the connection. Unlike <xref
linkend="libpq-PQstatus"/>,
+       this check is performed by polling the corresponding socket. This
+       function is currently available only on systems that support the
+       non-standard <symbol>POLLRDHUP</symbol> extension to the
<symbol>poll</symbol>
+       system call, including Linux. <xref linkend="libpq-PQconnCheck"/>
+       returns greater than zero if the remote peer seems to be closed, returns
+       <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.
+      </para>

"check the health" --> "checks the health"

~~~

3. PQcanConnCheck

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

Should the reference to PQconnCheck be a link as it previously was?

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

4. PQconnCheck

+/*
+ * Check whether the socket peer closed connection or not.
+ *
+ * Returns >0 if remote peer seems to be closed, 0 if it is valid,
+ * -1 if the input connection is bad or an error occurred.
+ */
+int
+PQconnCheck(PGconn *conn)
+{
+ return pqSocketCheck(conn, 0, 0, (time_t) 0);
+}

I'm confused. This comment says =0 means connection is valid. But the
pqSocketCheck comment says =0 means it timed out.

So those two function comments don't seem compatible

~~~

5. PQconnCheckable

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

Why say "works well"? IMO it either works or doesn't work – there is no "well".

SUGGESTION1
Check whether PQconnCheck() works on this platform.

SUGGESTION2
Check whether PQconnCheck() can work on this platform.

~~~

6. pqSocketCheck

 /*
  * Checks a socket, using poll or select, for data to be read, written,
- * or both.  Returns >0 if one or more conditions are met, 0 if it timed
+ * or both. Moreover, when neither forRead nor forWrite is requested and
+ * timeout is disabled, try to check the health of socket.
+ *
+ * Returns >0 if one or more conditions are met, 0 if it timed
  * out, -1 if an error occurred.
  *
  * If SSL is in use, the SSL buffer is checked prior to checking the socket

~

See review comment #4. (e.g. This says =0 if it timed out).

~~~

7. pqSocketPoll

+ * When neither forRead nor forWrite are set and timeout is disabled,
+ *
+ * - If the timeout is disabled, try to check the health of the socket
+ * - Otherwise this immediately returns 0
+ *
+ * Return >0 if condition is met, 0 if a timeout occurred, -1 if an error
+ * or interrupt occurred.

Don't say "and timeout is disabled," because it clashes with the 1st
bullet which also says "- If the timeout is disabled,".

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

---

From: Katsuragi Yuta <[email-masked]>
Date: Tue, 21 Feb 2023 16:59:44 +0900

Hi Kuroda-san,

Thank you for updating the patch!

> Thank you for reviewing! PSA new version.

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.

> an error is reported.

Yes, I think this error is fine.
I think there are cases where the given server name does exist,
however the connection check is not performed (does not pass the
first if statement above). 1) a case where the server name exist,
however an open connection to the specified server is not found.
2) case where connection for specified server is invalidated.
3) case where there is not ConnectionHash entry for the specified
server. Current implementation returns true in that case, however
the check is not performed. Suppose the checking mechanism is
supported on the platform, it does not seem reasonable to return
true or false when the check is not performed. What do you think?

regards,

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

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 22 Feb 2023 12:33:09 +0000

Dear Peter,

> "status of the socket" --> "status of the connection"

Sorry, fixed.

> "check the health" --> "checks the health"

Fixed.

> Should the reference to PQconnCheck be a link as it previously was?

Right, fixed.

> So those two function comments don't seem compatible

Added further descriptions atop pqSocketCheck() and pqSocketPoll().
Is it helpful to understand?

> Check whether PQconnCheck() can work on this platform.

I choose 2.

> See review comment #4. (e.g. This says =0 if it timed out).

Descriptions were added.

> bullet which also says "- If the timeout is disabled,".

This comments were reworded.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED

---

From: "Hayato Kuroda (Fujitsu)" <[email-masked]>
Date: Wed, 22 Feb 2023 12:34:24 +0000

Dear Katsuragi-san,

Thank you for reviewing! New patch set can be available on [1].

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

> true or false when the check is not performed. What do you think?

Thank you for detailed explanation. I agreed your opinion and modified like that.

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?

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

Best Regards,
Hayato Kuroda
FUJITSU LIMITED