BUG #1467: fe_connect doesn't handle EINTR right

AgentM <agentm@themactionfaction.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@themactionfaction.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@candle.pha.pa.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@themactionfaction.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)
  						{
  							/*