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

Dear Peter,

Thank you for reviewing! PSA new version.

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

Sorry, fixed.

> ====
> 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"

Fixed.

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

Right, fixed.

> 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

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

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

I choose 2.

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

Descriptions were added.

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

This comments were reworded.

Best Regards,
Hayato Kuroda
FUJITSU LIMITED