Re: Rare SSL failures on eelpout

I wrote:
> ... I don't like pqHandleSendFailure all that much: it has strong
> constraints on what state libpq has to be in, as a consequence of which
> it's called from a bunch of ad-hoc places, and can't be called from
> some others.  It's kind of accidental that it will work here.
> I was toying with the idea of getting rid of that function in
> favor of a design like this:
> * Handle any socket-write failure at some low level of libpq by
> recording the fact that the error occurred (plus whatever data we
> need to produce a message about it), and then starting to just
> discard output data.
> * Eventually, we'll try to read.  Process any available input data
> as usual (and, if we get a read error, report that).  When no more
> input data is available, if a socket write failure has been recorded,
> report that much as if it were an incoming ERROR message, and then
> shut down the connection.
> This would essentially give us pqHandleSendFailure-like behavior
> across the board, which might be enough to fix this problem as well as
> bug #15598.  Or not ... as you say, we haven't thoroughly understood
> either issue, so it's possible this wouldn't do it.

Here's a draft patch along that line.  It's gratifyingly small, and
it does fix the SSL problem for me.  I have no way of investigating
whether it fixes bug #15598, but it might.

Aside from the SSL cert-verify issue, I've checked the behavior when
the backend is shut down ("pg_ctl stop") between queries, and when
the backend is kill 9'd mid-query.  The shutdown case reacts well with
or without SSL:

regression=# select 2+2;
FATAL:  terminating connection due to administrator command
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

The backend-crash case is fine without SSL:

regression=# select * from tenk1 t1, tenk1 t2, tenk1 t3;
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Succeeded.

but a bit less nice with it:

regression=# select * from tenk1 t1, tenk1 t2, tenk1 t3;
SSL SYSCALL error: EOF detected
The connection to the server was lost. Attempting reset: Succeeded.

That seems cosmetic --- we just haven't expended the same effort on
what to print for connection EOF with SSL as without it.  (Also,
you get that same result without this patch.)

Of course, this hardly counts as exhaustive testing, especially since
I only tried one OpenSSL version.

> One thing that isn't real clear to me is how much timing sensitivity
> there is in "when no more input data is available".  Can we assume that
> if we've gotten ECONNRESET or an allied error from a write, then any
> data the far end might've wished to send us is already available to
> read?  In principle, since TCP allows shutting down either transmission
> direction independently, the server could close the read side of its
> socket while continuing to transmit data.  In practice, PG servers
> don't do that; but could network timing glitches create the same effect?
> Even if it's possible, does it happen enough to worry about?
> The reason I'm concerned is that I don't think it'd be bright to ignore a
> send error until we see input EOF, which'd be the obvious way to solve a
> timing problem if there is one.  If our send error was some transient
> glitch and the connection is really still open, then we might not get EOF,
> but we won't get a server reply either because no message went to the
> server.  You could imagine waiting some small interval of time before
> deciding that it's time to report the write failure, but ugh.

As written, this patch does seem vulnerable to this concern, if it's real.
We could address it for post-connection failures by tweaking PQgetResult
so that it doesn't call pqWait if there's already a write error recorded;
but I'm far from sure that that would be a net improvement.  Also, making
a comparable change to the behavior of the PQconnectPoll state machine
might be tricky.

My current feeling is that this is OK to put in HEAD but I think the
risk-reward ratio isn't very good for the back branches.  Even with
an OpenSSL version where this makes a difference, the problematic
behavior is pretty hard to hit.  So I'm a bit inclined to do nothing
in the back branches.

            regards, tom lane

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index c96a52b..e3bf6a7 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -537,6 +537,10 @@ pqDropServerData(PGconn *conn)
     conn->last_sqlstate[0] = '\0';
     conn->auth_req_received = false;
     conn->password_needed = false;
+    conn->write_failed = false;
+    if (conn->write_err_msg)
+        free(conn->write_err_msg);
+    conn->write_err_msg = NULL;
     conn->be_pid = 0;
     conn->be_key = 0;
 }
@@ -3702,6 +3706,8 @@ freePGconn(PGconn *conn)
     /* Note that conn->Pfdebug is not ours to close or free */
     if (conn->last_query)
         free(conn->last_query);
+    if (conn->write_err_msg)
+        free(conn->write_err_msg);
     if (conn->inBuffer)
         free(conn->inBuffer);
     if (conn->outBuffer)
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index ac969e7..93ff428 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -791,6 +791,32 @@ pqSaveErrorResult(PGconn *conn)
 }

 /*
+ * As above, and append conn->write_err_msg to whatever other error we have.
+ * This is used when we've detected a write failure and have exhausted our
+ * chances of reporting something else instead.
+ */
+static void
+pqSaveWriteError(PGconn *conn)
+{
+    /*
+     * Ensure conn->result is an error result, and add anything in
+     * conn->errorMessage to it.
+     */
+    pqSaveErrorResult(conn);
+
+    /*
+     * Now append write_err_msg to that.  If it's null because of previous
+     * strdup failure, do what we can.  (It's likely our machinations here are
+     * all getting OOM failures as well, but ...)
+     */
+    if (conn->write_err_msg)
+        pqCatenateResultError(conn->result, conn->write_err_msg);
+    else
+        pqCatenateResultError(conn->result,
+                              libpq_gettext("write to server failed\n"));
+}
+
+/*
  * This subroutine prepares an async result object for return to the caller.
  * If there is not already an async result object, build an error object
  * using whatever is in conn->errorMessage.  In any case, clear the async
@@ -1224,7 +1250,7 @@ PQsendQuery(PGconn *conn, const char *query)
         pqPuts(query, conn) < 0 ||
         pqPutMsgEnd(conn) < 0)
     {
-        pqHandleSendFailure(conn);
+        /* error message should be set up already */
         return 0;
     }

@@ -1243,7 +1269,7 @@ PQsendQuery(PGconn *conn, const char *query)
      */
     if (pqFlush(conn) < 0)
     {
-        pqHandleSendFailure(conn);
+        /* error message should be set up already */
         return 0;
     }

@@ -1389,7 +1415,7 @@ PQsendPrepare(PGconn *conn,
     return 1;

 sendFailed:
-    pqHandleSendFailure(conn);
+    /* error message should be set up already */
     return 0;
 }

@@ -1641,40 +1667,11 @@ PQsendQueryGuts(PGconn *conn,
     return 1;

 sendFailed:
-    pqHandleSendFailure(conn);
+    /* error message should be set up already */
     return 0;
 }

 /*
- * pqHandleSendFailure: try to clean up after failure to send command.
- *
- * Primarily, what we want to accomplish here is to process any ERROR or
- * NOTICE messages that the backend might have sent just before it died.
- * Since we're in IDLE state, all such messages will get sent to the notice
- * processor.
- *
- * NOTE: this routine should only be called in PGASYNC_IDLE state.
- */
-void
-pqHandleSendFailure(PGconn *conn)
-{
-    /*
-     * Accept and parse any available input data, ignoring I/O errors.  Note
-     * that if pqReadData decides the backend has closed the channel, it will
-     * close our side of the socket --- that's just what we want here.
-     */
-    while (pqReadData(conn) > 0)
-        parseInput(conn);
-
-    /*
-     * Be sure to parse available input messages even if we read no data.
-     * (Note: calling parseInput within the above loop isn't really necessary,
-     * but it prevents buffer bloat if there's a lot of data available.)
-     */
-    parseInput(conn);
-}
-
-/*
  * Select row-by-row processing mode
  */
 int
@@ -1763,8 +1760,11 @@ PQisBusy(PGconn *conn)
     /* Parse any available data, if our state permits. */
     parseInput(conn);

-    /* PQgetResult will return immediately in all states except BUSY. */
-    return conn->asyncStatus == PGASYNC_BUSY;
+    /*
+     * PQgetResult will return immediately in all states except BUSY, or if we
+     * had a write failure.
+     */
+    return conn->asyncStatus == PGASYNC_BUSY || conn->write_failed;
 }


@@ -1804,7 +1804,13 @@ PQgetResult(PGconn *conn)
             }
         }

-        /* Wait for some more data, and load it. */
+        /*
+         * Wait for some more data, and load it.  (Note: if the connection has
+         * been lost, pqWait should return immediately because the socket
+         * should be read-ready, either with the last server data or with an
+         * EOF indication.  We expect therefore that this won't result in any
+         * undue delay in reporting a previous write failure.)
+         */
         if (flushResult ||
             pqWait(true, false, conn) ||
             pqReadData(conn) < 0)
@@ -1820,6 +1826,17 @@ PQgetResult(PGconn *conn)

         /* Parse it. */
         parseInput(conn);
+
+        /*
+         * If we had a write error, but nothing above obtained a query result
+         * or detected a read error, report the write error.
+         */
+        if (conn->write_failed && conn->asyncStatus == PGASYNC_BUSY)
+        {
+            pqSaveWriteError(conn);
+            conn->asyncStatus = PGASYNC_IDLE;
+            return pqPrepareAsyncResult(conn);
+        }
     }

     /* Return the appropriate thing. */
@@ -2252,7 +2269,7 @@ PQsendDescribe(PGconn *conn, char desc_type, const char *desc_target)
     return 1;

 sendFailed:
-    pqHandleSendFailure(conn);
+    /* error message should be set up already */
     return 0;
 }

diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index e5ef8d4..718da1c 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -824,6 +824,13 @@ definitelyFailed:
  *
  * Return 0 on success, -1 on failure and 1 when not all data could be sent
  * because the socket would block and the connection is non-blocking.
+ *
+ * Upon write failure, conn->write_failed is set and the error message is
+ * saved in conn->write_err_msg, but we clear the output buffer and return
+ * zero anyway; this is because callers should soldier on until it's possible
+ * to read from the server and check for an error message.  write_err_msg
+ * should be reported only when we are unable to obtain a server error first.
+ * (Thus, a -1 result is returned only for an internal *read* failure.)
  */
 static int
 pqSendSome(PGconn *conn, int len)
@@ -832,13 +839,32 @@ pqSendSome(PGconn *conn, int len)
     int            remaining = conn->outCount;
     int            result = 0;

+    /*
+     * If we already had a write failure, we will never again try to send data
+     * on that connection.  Even if the kernel would let us, we've probably
+     * lost message boundary sync with the server.  conn->write_failed
+     * therefore persists until the connection is reset, and we just discard
+     * all data presented to be written.
+     */
+    if (conn->write_failed)
+    {
+        /* conn->write_err_msg should be set up already */
+        conn->outCount = 0;
+        return 0;
+    }
+
     if (conn->sock == PGINVALID_SOCKET)
     {
         printfPQExpBuffer(&conn->errorMessage,
                           libpq_gettext("connection not open\n"));
+        conn->write_failed = true;
+        /* Transfer error message to conn->write_err_msg, if possible */
+        /* (strdup failure is OK, we'll cope later) */
+        conn->write_err_msg = strdup(conn->errorMessage.data);
+        resetPQExpBuffer(&conn->errorMessage);
         /* Discard queued data; no chance it'll ever be sent */
         conn->outCount = 0;
-        return -1;
+        return 0;
     }

     /* while there's still data to send */
@@ -876,17 +902,18 @@ pqSendSome(PGconn *conn, int len)

                 default:
                     /* pqsecure_write set the error message for us */
+                    conn->write_failed = true;

                     /*
-                     * We used to close the socket here, but that's a bad idea
-                     * since there might be unread data waiting (typically, a
-                     * NOTICE message from the backend telling us it's
-                     * committing hara-kiri...).  Leave the socket open until
-                     * pqReadData finds no more data can be read.  But abandon
-                     * attempt to send data.
+                     * Transfer error message to conn->write_err_msg, if
+                     * possible (strdup failure is OK, we'll cope later)
                      */
+                    conn->write_err_msg = strdup(conn->errorMessage.data);
+                    resetPQExpBuffer(&conn->errorMessage);
+
+                    /* Discard queued data; no chance it'll ever be sent */
                     conn->outCount = 0;
-                    return -1;
+                    return 0;
             }
         }
         else
@@ -921,6 +948,9 @@ pqSendSome(PGconn *conn, int len)
              * can do, and works pretty well in practice.  (The documentation
              * used to say that you only need to wait for write-ready, so
              * there are still plenty of applications like that out there.)
+             *
+             * Note that errors here don't result in write_failed becoming
+             * set.
              */
             if (pqReadData(conn) < 0)
             {
@@ -956,6 +986,7 @@ pqSendSome(PGconn *conn, int len)
  *
  * Return 0 on success, -1 on failure and 1 when not all data could be sent
  * because the socket would block and the connection is non-blocking.
+ * (See pqSendSome comments about how failure should be handled.)
  */
 int
 pqFlush(PGconn *conn)
diff --git a/src/interfaces/libpq/fe-protocol2.c b/src/interfaces/libpq/fe-protocol2.c
index 1ab1421..0e36974 100644
--- a/src/interfaces/libpq/fe-protocol2.c
+++ b/src/interfaces/libpq/fe-protocol2.c
@@ -1450,42 +1450,30 @@ pqFunctionCall2(PGconn *conn, Oid fnid,
         pqPutInt(fnid, 4, conn) != 0 || /* function id */
         pqPutInt(nargs, 4, conn) != 0)    /* # of args */
     {
-        pqHandleSendFailure(conn);
+        /* error message should be set up already */
         return NULL;
     }

     for (i = 0; i < nargs; ++i)
     {                            /* len.int4 + contents       */
         if (pqPutInt(args[i].len, 4, conn))
-        {
-            pqHandleSendFailure(conn);
             return NULL;
-        }

         if (args[i].isint)
         {
             if (pqPutInt(args[i].u.integer, 4, conn))
-            {
-                pqHandleSendFailure(conn);
                 return NULL;
-            }
         }
         else
         {
             if (pqPutnchar((char *) args[i].u.ptr, args[i].len, conn))
-            {
-                pqHandleSendFailure(conn);
                 return NULL;
-            }
         }
     }

     if (pqPutMsgEnd(conn) < 0 ||
         pqFlush(conn))
-    {
-        pqHandleSendFailure(conn);
         return NULL;
-    }

     for (;;)
     {
diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c
index 47dbc31..ec51fee 100644
--- a/src/interfaces/libpq/fe-protocol3.c
+++ b/src/interfaces/libpq/fe-protocol3.c
@@ -1926,50 +1926,35 @@ pqFunctionCall3(PGconn *conn, Oid fnid,
         pqPutInt(1, 2, conn) < 0 || /* format code: BINARY */
         pqPutInt(nargs, 2, conn) < 0)    /* # of args */
     {
-        pqHandleSendFailure(conn);
+        /* error message should be set up already */
         return NULL;
     }

     for (i = 0; i < nargs; ++i)
     {                            /* len.int4 + contents       */
         if (pqPutInt(args[i].len, 4, conn))
-        {
-            pqHandleSendFailure(conn);
             return NULL;
-        }
         if (args[i].len == -1)
             continue;            /* it's NULL */

         if (args[i].isint)
         {
             if (pqPutInt(args[i].u.integer, args[i].len, conn))
-            {
-                pqHandleSendFailure(conn);
                 return NULL;
-            }
         }
         else
         {
             if (pqPutnchar((char *) args[i].u.ptr, args[i].len, conn))
-            {
-                pqHandleSendFailure(conn);
                 return NULL;
-            }
         }
     }

     if (pqPutInt(1, 2, conn) < 0)    /* result format code: BINARY */
-    {
-        pqHandleSendFailure(conn);
         return NULL;
-    }

     if (pqPutMsgEnd(conn) < 0 ||
         pqFlush(conn))
-    {
-        pqHandleSendFailure(conn);
         return NULL;
-    }

     for (;;)
     {
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 4a93d8e..dbe0f7e 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -410,6 +410,8 @@ struct pg_conn
     bool        password_needed;    /* true if server demanded a password */
     bool        sigpipe_so;        /* have we masked SIGPIPE via SO_NOSIGPIPE? */
     bool        sigpipe_flag;    /* can we mask SIGPIPE via MSG_NOSIGNAL? */
+    bool        write_failed;    /* have we had a write failure on sock? */
+    char       *write_err_msg;    /* write error message, or NULL if OOM */

     /* Transient state needed while establishing connection */
     bool        try_next_addr;    /* time to advance to next address/host? */
@@ -585,7 +587,6 @@ extern void pqSaveMessageField(PGresult *res, char code,
 extern void pqSaveParameterStatus(PGconn *conn, const char *name,
                       const char *value);
 extern int    pqRowProcessor(PGconn *conn, const char **errmsgp);
-extern void pqHandleSendFailure(PGconn *conn);

 /* === in fe-protocol2.c === */