Re: pg_dump possible fix, need testers. (was: Re: [HACKERS] pg_dump disaster)

Patch appled.  And thanks very much for the patch.  I am sorry you were
given a bad time about your previous patch.  We will try to not let it
happen again.



> 
> 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 library
>  seems to be setup to deal with the socket being in non-block mode at
>  all times
> turn off the nonblock flag if/when the connection is torn down to ensure
>  that 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 from blocking 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 from blocking to non-blocking flush here */
> !     if (pqFlush(conn))
>           return (-1);
>   
>       conn->nonblocking = arg;
>   
>       return (0);
>   }
> 
> --
> -Alfred Perlstein - [bright@wintelcom.net|alfred@freebsd.org]
> 


--  Bruce Momjian                        |  http://www.op.net/~candle pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026