Hi Thomas! Thanks for working on this patch.
I have attached a new version with some typo corrections of doc entry,
removing of redundant `include` entries and trailing whitespaces. Also I
added in doc the case when single query transaction with disconnected
client might be eventually commited upon completion in autocommit mode
if it doesn't return any rows (doesn't communicate with user) before
sending final commit message. This behavior might be unexpected for
clients and hence IMO it's worth noticing.
> diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
> index 4c7b1e7bfd..8cf95d09a4 100644
> --- a/src/backend/libpq/pqcomm.c
> +++ b/src/backend/libpq/pqcomm.c
>
> @@ -919,13 +923,13 @@ socket_set_nonblocking(bool nonblocking)
> }
>
> /* --------------------------------
> - * pq_recvbuf - load some bytes into the input buffer
> + * pq_recvbuf_ext - load some bytes into the input buffer
> *
> * returns 0 if OK, EOF if trouble
> * --------------------------------
> */
> static int
> -pq_recvbuf(void)
> +pq_recvbuf_ext(bool nonblocking)
> {
> if (PqRecvPointer > 0)
> {
> @@ -941,8 +945,7 @@ pq_recvbuf(void)
> PqRecvLength = PqRecvPointer = 0;
> }
>
> - /* Ensure that we're in blocking mode */
> - socket_set_nonblocking(false);
> + socket_set_nonblocking(nonblocking);
>
> /* Can fill buffer from PqRecvLength and upwards */
> for (;;)
> @@ -954,6 +957,9 @@ pq_recvbuf(void)
>
> if (r < 0)
> {
> + if (nonblocking && (errno == EAGAIN || errno ==
EWOULDBLOCK))
> + return 0;
> +
> if (errno == EINTR)
> continue; /* Ok if interrupted */
>
> @@ -981,6 +987,13 @@ pq_recvbuf(void)
> }
> }
>
> +static int
> +pq_recvbuf(void)
> +{
> + return pq_recvbuf_ext(false);
> +}
> +
> +
AFAICS, the above fragment is not related with primary fix directly.
AFAICS, there are the following open items that don't allow to treat the
current patch completed:
* Absence of tap tests emulating some scenarios of client disconnection.
IIRC, you wanted to rewrite the test case posted by Sergey.
* Concerns about portability of `pq_check_connection()`A implementation.
BTW, on windows postgres with this patch have not been built [1].
* Absence of benchmark results to show lack of noticeable performance
regression after applying non-zero timeout for checking client liveness.
1.
https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.131820
--
Regards,
Maksim Milyutin
