public inbox for [email protected]
help / color / mirror / Atom feedFrom: Peter Smith <[email protected]>
To: Hayato Kuroda (Fujitsu) <[email protected]>
Cc: Katsuragi Yuta <[email protected]>
Cc: Ted Yu <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: vignesh C <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Önder Kalacı <[email protected]>
Cc: Fujii Masao <[email protected]>
Cc: [email protected] <[email protected]>
Cc: Kyotaro Horiguchi <[email protected]>
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
Date: Mon, 20 Feb 2023 12:23:32 +1100
Message-ID: <CAHut+Puqtm1tYJNtggNKANDy7foJD2zuJog4Vb6u9YBE3vLgKA@mail.gmail.com> (raw)
In-Reply-To: <TYAPR01MB58669EAAC02493BFF9F39B06F5D99@TYAPR01MB5866.jpnprd01.prod.outlook.com>
References: <CACawEhW_R=6mKsB24QW3WpCZTQgtxAPH7J0q8yedKkCQY2xT0w@mail.gmail.com>
<TYAPR01MB5866F419C4261177578AC1CCF55B9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<CACawEhXAy4gjJ4G3-59Xv_yb8Xbtn1VMYP8SOj-0C9dG2zbAcQ@mail.gmail.com>
<TYAPR01MB5866ACB4D3B845B4B2ED121BF55A9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<CACawEhW19nPfbFpvfke9eidFDxAy+ic36wmY0s936T=xzxgHog@mail.gmail.com>
<TYAPR01MB5866CFD6BAE6DDF01A27CBF1F5299@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<CACawEhUzpqYJ8mQmSjYgX0ePtPpvb2u9Onjf6pCjUGkoZ=-xSg@mail.gmail.com>
<TYAPR01MB58668728393648C2F7DC7C85F5399@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<CACawEhW56PHQ83Q59x4U5zpi0rVJ6=0Vn-FeYSLZ13Y0yasebQ@mail.gmail.com>
<TYAPR01MB5866AAA4EB0632B4001AA2C5F50E9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<[email protected]>
<TYAPR01MB586635DB5F1C5CE4660175FEF51A9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<TYAPR01MB58660C8F72746C2A8C6381F2F5EA9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<TYAPR01MB586613F662443D5A3005CD17F5FF9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<[email protected]>
<TYAPR01MB586612A29BAF468042F04B20F5FF9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<CALte62zppS-YxEhes5jSsHppQsRrgHBw-krViS1HJJPSsV9onw@mail.gmail.com>
<TYAPR01MB5866413814861A5016839506F5FC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<TYAPR01MB5866C20861167C9CBEDF0A36F5FC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<[email protected]>
<TYAPR01MB5866E17F6C47AB41DCB8ADBEF5CA9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<CALte62y83j9p8xOoOS=ftG8PZTwUCUBHAp=WiKM7i6NUon3cNg@mail.gmail.com>
<TYAPR01MB58668DD17F19CAFC97A3134EF5C89@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<[email protected]>
<TYAPR01MB5866969DBAEEEC5AE07591BFF5CE9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<TYAPR01MB5866B55A252EF7005AA81D5BF5CC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<TYAPR01MB586675882721E646693E13FBF5CC9@TYAPR01MB5866.jpnprd01.prod.outlook.com>
<[email protected]>
<TYAPR01MB58669EAAC02493BFF9F39B06F5D99@TYAPR01MB5866.jpnprd01.prod.outlook.com>
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
view thread (19+ messages)
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: [Proposal] Add foreign-server health checks infrastructure
In-Reply-To: <CAHut+Puqtm1tYJNtggNKANDy7foJD2zuJog4Vb6u9YBE3vLgKA@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox