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
v33-0001-Add-PQconnCheck-and-PQconnCheckable-to-libpq.patch
Description: v33-0001-Add-PQconnCheck-and-PQconnCheckable-to-libpq.patch
v33-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch
Description: v33-0002-postgres_fdw-add-postgres_fdw_verify_connection_.patch
v33-0003-add-test.patch
Description: v33-0003-add-test.patch