pg_dump possible fix, need testers. (was: Re: [HACKERS] pg_dump disaster) - Mailing list pgsql-hackers

* Tom Lane <tgl@sss.pgh.pa.us> [000121 14:35] wrote:
> Alfred Perlstein <bright@wintelcom.net> writes:
> > We both missed it, and yes it was my fault.  All connections are
> > behaving as if PQsetnonblocking(conn, TRUE) have been called on them.
> > The original non-blocking patches did something weird, they seemed
> > to _always_ stick the socket into non-blocking mode.  This would
> > activate my non-blocking stuff for all connections.
> 
> Yes, the present state of the code seems to activate nonblocking socket
> mode all the time; possibly we could band-aid our way back to a working
> psql by turning off nonblock mode by default.  But this doesn't address
> the fact that the API of these routines cannot support nonblock mode
> without being redesigned.

These patches revert the default setting of the non-block flag back
to the old way connections were done.  Since i'm unable to reproduce
this bug I'm hoping people can _please_ give me feedback.

This is just a first shot at fixing the issue, I'll supply changes
to the docs if this all goes well (that you must explicitly set the
blocking status after a connect/disconnect)

I'm a bit concerned about busy looping because the connection is
left in a non-blocking state after the connect, however most of
the code performs select() and checks for EWOULDBLOCK/EAGAIN so it
might not be an issue.

Thanks for holding off on backing out the changes.

Summary:
don't set the nonblock flag during connections
PQsetnonblocking doesn't fiddle with socket flags anymore as the libraryseems to be setup to deal with the socket being
innon-block mode atall times
 
turn off the nonblock flag if/when the connection is torn down to ensurethat a reconnect behaves like it used to.


Index: fe-connect.c
===================================================================
RCS file: /home/pgcvs/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.114
diff -u -c -I$Header: -r1.114 fe-connect.c
cvs diff: conflicting specifications of output style
*** fe-connect.c    2000/01/23 01:27:39    1.114
--- fe-connect.c    2000/01/23 08:56:17
***************
*** 391,397 ****     PGconn       *conn;     char       *tmp;    /* An error message from some service we call. */
bool       error = FALSE;    /* We encountered an error. */
 
-     int            i;      conn = makeEmptyPGconn();     if (conn == NULL)
--- 391,396 ----
***************
*** 586,591 ****
--- 585,614 ---- }  /* ----------
+  * connectMakeNonblocking -
+  * Make a connection non-blocking.
+  * Returns 1 if successful, 0 if not.
+  * ----------
+  */
+ static int
+ connectMakeNonblocking(PGconn *conn)
+ {
+ #ifndef WIN32
+     if (fcntl(conn->sock, F_SETFL, O_NONBLOCK) < 0)
+ #else
+     if (ioctlsocket(conn->sock, FIONBIO, &on) != 0)
+ #endif
+     {
+         printfPQExpBuffer(&conn->errorMessage,
+                           "connectMakeNonblocking -- fcntl() failed: errno=%d\n%s\n",
+                           errno, strerror(errno));
+         return 0;
+     }
+ 
+     return 1;
+ }
+ 
+ /* ----------  * connectNoDelay -  * Sets the TCP_NODELAY socket option.  * Returns 1 if successful, 0 if not.
***************
*** 755,761 ****      *   Ewan Mellor <eem21@cam.ac.uk>.      * ---------- */ #if (!defined(WIN32) ||
defined(WIN32_NON_BLOCKING_CONNECTIONS))&& !defined(USE_SSL)
 
!     if (PQsetnonblocking(conn, TRUE) != 0)         goto connect_errReturn; #endif     
--- 778,784 ----      *   Ewan Mellor <eem21@cam.ac.uk>.      * ---------- */ #if (!defined(WIN32) ||
defined(WIN32_NON_BLOCKING_CONNECTIONS))&& !defined(USE_SSL)
 
!     if (connectMakeNonblocking(conn) == 0)         goto connect_errReturn; #endif     
***************
*** 868,874 ****     /* This makes the connection non-blocking, for all those cases which forced us        not to do it
above.*/ #if (defined(WIN32) && !defined(WIN32_NON_BLOCKING_CONNECTIONS)) || defined(USE_SSL)
 
!     if (PQsetnonblocking(conn, TRUE) != 0)         goto connect_errReturn; #endif     
--- 891,897 ----     /* This makes the connection non-blocking, for all those cases which forced us        not to do it
above.*/ #if (defined(WIN32) && !defined(WIN32_NON_BLOCKING_CONNECTIONS)) || defined(USE_SSL)
 
!     if (connectMakeNonblocking(conn) == 0)         goto connect_errReturn; #endif     
***************
*** 1785,1790 ****
--- 1808,1820 ----         (void) pqPuts("X", conn);         (void) pqFlush(conn);     }
+ 
+     /* 
+      * must reset the blocking status so a possible reconnect will work
+      * don't call PQsetnonblocking() because it will fail if it's unable
+      * to flush the connection.
+      */
+     conn->nonblocking = FALSE;      /*      * Close the connection, reset all transient state, flush I/O buffers.
Index: fe-exec.c
===================================================================
RCS file: /home/pgcvs/pgsql/src/interfaces/libpq/fe-exec.c,v
retrieving revision 1.87
diff -u -c -I$Header: -r1.87 fe-exec.c
cvs diff: conflicting specifications of output style
*** fe-exec.c    2000/01/18 06:09:24    1.87
--- fe-exec.c    2000/01/23 08:56:29
***************
*** 2116,2122 **** int PQsetnonblocking(PGconn *conn, int arg) {
-     int    fcntlarg;      arg = (arg == TRUE) ? 1 : 0;     /* early out if the socket is already in the state
requested*/
 
--- 2116,2121 ----
***************
*** 2131,2174 ****      *  _from_ or _to_ blocking mode, either way we can block them.      */     /* if we are going
fromblocking to non-blocking flush here */
 
!     if (!pqIsnonblocking(conn) && pqFlush(conn))
!         return (-1);
! 
! 
! #ifdef USE_SSL
!     if (conn->ssl)
!     {
!         printfPQExpBuffer(&conn->errorMessage,
!             "PQsetnonblocking() -- not supported when using SSL\n");
!         return (-1);
!     }
! #endif /* USE_SSL */
! 
! #ifndef WIN32
!     fcntlarg = fcntl(conn->sock, F_GETFL, 0);
!     if (fcntlarg == -1)
!         return (-1);
! 
!     if ((arg == TRUE && 
!         fcntl(conn->sock, F_SETFL, fcntlarg | O_NONBLOCK) == -1) ||
!         (arg == FALSE &&
!         fcntl(conn->sock, F_SETFL, fcntlarg & ~O_NONBLOCK) == -1)) 
! #else
!     fcntlarg = arg;
!     if (ioctlsocket(conn->sock, FIONBIO, &fcntlarg) != 0)
! #endif
!     {
!         printfPQExpBuffer(&conn->errorMessage,
!             "PQsetblocking() -- unable to set nonblocking status to %s\n",
!             arg == TRUE ? "TRUE" : "FALSE");         return (-1);
-     }      conn->nonblocking = arg;
- 
-     /* if we are going from non-blocking to blocking flush here */
-     if (pqIsnonblocking(conn) && pqFlush(conn))
-         return (-1);      return (0); }
--- 2130,2139 ----      *  _from_ or _to_ blocking mode, either way we can block them.      */     /* if we are going
fromblocking to non-blocking flush here */
 
!     if (pqFlush(conn))         return (-1);      conn->nonblocking = arg;      return (0); }

--
-Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org]