BUG #1467: fe_connect doesn't handle EINTR right

Attached is a patch which corrects the behavior. I verified that the
patch does not interfere with normal operation (using psql) but
unfortunately the code path is virtually impossible to test without a
really slow connection to a postgresql server [which I thankfully
don't have]. To test the patch, you would need to send an interrupt
at the exact time that the kernel is connect()ing in blocking mode-
good luck.

Also, I recommend removing a (sarcastic?) comment left by a previous
developer- I wrote a note about in my patch.

The patch is against 8.0.3 [because HEAD requires access to a
specific version of bison] but I imagine that the code hasn't changed
in fe-connect.c since then.

Patch against src/interfaces/libpq/fe-connect.c (v 8.0.3)

AgentM <agentm(at)themactionfaction(dot)com> writes:
> Attached is a patch which corrects the behavior. I verified that the
> patch does not interfere with normal operation (using psql) but
> unfortunately the code path is virtually impossible to test without a
> really slow connection to a postgresql server [which I thankfully
> don't have].

I'm still looking for some demonstration (not an unsupported assertion)
that there's an issue here. A patch you cannot test doesn't impress
me at all --- what are the odds that it makes things worse not better?

regards, tom lane

AgentM said:
> Attached is a patch which corrects the behavior. I verified that the
> patch does not interfere with normal operation (using psql) but
> unfortunately the code path is virtually impossible to test without a
> really slow connection to a postgresql server [which I thankfully
> don't have]. To test the patch, you would need to send an interrupt
> at the exact time that the kernel is connect()ing in blocking mode-
> good luck.
>
> Also, I recommend removing a (sarcastic?) comment left by a previous
> developer- I wrote a note about in my patch.
>
> The patch is against 8.0.3 [because HEAD requires access to a
> specific version of bison] but I imagine that the code hasn't changed
> in fe-connect.c since then.
>
> Patch against src/interfaces/libpq/fe-connect.c (v 8.0.3)

It's not even legal C89 C - please don't use // style comments.

cheers

andrew

Tom Lane wrote:
> AgentM <agentm(at)themactionfaction(dot)com> writes:
> > Attached is a patch which corrects the behavior. I verified that the
> > patch does not interfere with normal operation (using psql) but
> > unfortunately the code path is virtually impossible to test without a
> > really slow connection to a postgresql server [which I thankfully
> > don't have].
>
> I'm still looking for some demonstration (not an unsupported assertion)
> that there's an issue here. A patch you cannot test doesn't impress
> me at all --- what are the odds that it makes things worse not better?

Well, we have a documented case that we are not following the API. In
that case, I don't consider it necessary for someone to provide a
reproducable failure (it might be quite rare and therefore hard to
demostrate). It is enough we are not following the API and need to fix
our code.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

AgentM <agentm(at)themactionfaction(dot)com> writes:
> Attached is a patch which corrects the behavior. I verified that the
> patch does not interfere with normal operation (using psql) but
> unfortunately the code path is virtually impossible to test without a
> really slow connection to a postgresql server [which I thankfully
> don't have]. To test the patch, you would need to send an interrupt
> at the exact time that the kernel is connect()ing in blocking mode-
> good luck.

I've applied a simplified version of this patch --- there is no need to
duplicate the functionality that the calling level must have anyway.

regards, tom lane

Index: fe-connect.c
===================================================================
RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.316
retrieving revision 1.317
diff -c -r1.316 -r1.317
*** fe-connect.c 9 Aug 2005 05:14:26 -0000 1.316
--- fe-connect.c 11 Aug 2005 22:53:41 -0000 1.317
***************
*** 1082,1096 ****
* since we are in nonblock mode. If it does, well,
* too bad.
*/
- retry_connect:
if (connect(conn->sock, addr_cur->ai_addr,
addr_cur->ai_addrlen) < 0)
{
- if (SOCK_ERRNO == EINTR)
- /* Interrupted system call - just try again */
- goto retry_connect;
if (SOCK_ERRNO == EINPROGRESS ||
SOCK_ERRNO == EWOULDBLOCK ||
SOCK_ERRNO == 0)
{
/*
--- 1082,1093 ----
* since we are in nonblock mode. If it does, well,
* too bad.
*/
if (connect(conn->sock, addr_cur->ai_addr,
addr_cur->ai_addrlen) < 0)
{
if (SOCK_ERRNO == EINPROGRESS ||
SOCK_ERRNO == EWOULDBLOCK ||
+ SOCK_ERRNO == EINTR ||
SOCK_ERRNO == 0)
{
/*