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(at)cam(dot)ac(dot)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(at)cam(dot)ac(dot)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(at)wintelcom(dot)net|alfred(at)freebsd(dot)org]
>

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