Re: Expansion of our checks for connection-loss errors

Поиск
Список
Период
Сортировка
От Tom Lane
Тема Re: Expansion of our checks for connection-loss errors
Дата
Msg-id 2768918.1602260057@sss.pgh.pa.us
обсуждение исходный текст
Ответ на Re: Expansion of our checks for connection-loss errors  (Tom Lane <tgl@sss.pgh.pa.us>)
Список pgsql-hackers
I wrote:
> Hmm, excellent point.  While our code response to all these errors
> should be the same, you are right that that doesn't extend to emitting
> identical error texts.  For EHOSTUNREACH/ENETDOWN/ENETUNREACH, we
> should say something like "connection to server lost", without claiming
> that the server crashed.  It is less clear what to do with ECONNABORTED,
> but I'm inclined to put it in the network-problem bucket not the
> server-crash bucket, despite lorikeet's behavior.  Thoughts?

> This also destroys the patch's idea that switch statements should be
> able to handle these all alike.  If we group things as "ECONNRESET means
> server crash and the others are all network failures", then I'd be
> inclined to leave the ECONNRESET cases alone and just introduce
> new infrastructure to recognize all the network-failure errnos.

Actually, treating it that way seems like a good thing because it nets
out as (nearly) no change to our error message behavior.  The connection
failure errnos fall through to the default case, which produces a
perfectly reasonable report that includes strerror().  The only big thing
we're changing is the set of errnos that errcode_for_socket_access will
map to ERRCODE_CONNECTION_FAILURE, so this is spiritually closer to your
original patch.

Some other changes in the attached v2:

* I incorporated Kyotaro-san's suggested improvements.

* I went ahead and included ENETRESET and EHOSTDOWN, figuring that
if they exist we definitely want to class them as network failures.
We can worry about ifdef'ing them when and if we find a platform
that hasn't got them.  (I don't see any non-ugly way to make the
ALL_NETWORK_FAILURE_ERRNOS macro vary for missing symbols, so I'd
rather not deal with that unless it's proven necessary.)

* I noticed that we were not terribly consistent about whether
EPIPE is regarded as indicating a server failure like ECONNRESET
does.  So this patch also makes sure that EPIPE is treated like
ECONNRESET everywhere.  (Hence, pqsecure_raw_read's error reporting
does change, since it'll now report EPIPE as server failure.)

I lack a way to test this on Windows, but otherwise it feels
like it's about ready.

            regards, tom lane

diff --git a/src/backend/port/win32/socket.c b/src/backend/port/win32/socket.c
index 6fbd1ed6fb..b4ef9fbd8f 100644
--- a/src/backend/port/win32/socket.c
+++ b/src/backend/port/win32/socket.c
@@ -121,12 +121,20 @@ TranslateSocketError(void)
             errno = EADDRNOTAVAIL;
             break;
         case WSAEHOSTUNREACH:
-        case WSAEHOSTDOWN:
         case WSAHOST_NOT_FOUND:
+            errno = EHOSTUNREACH;
+            break;
+        case WSAEHOSTDOWN:
+            errno = EHOSTDOWN;
+            break;
         case WSAENETDOWN:
+            errno = ENETDOWN;
+            break;
         case WSAENETUNREACH:
+            errno = ENETUNREACH;
+            break;
         case WSAENETRESET:
-            errno = EHOSTUNREACH;
+            errno = ENETRESET;
             break;
         case WSAENOTCONN:
         case WSAESHUTDOWN:
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index d0b368530e..637768d776 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -712,9 +712,8 @@ errcode_for_socket_access(void)
     {
             /* Loss of connection */
         case EPIPE:
-#ifdef ECONNRESET
         case ECONNRESET:
-#endif
+        case ALL_NETWORK_FAILURE_ERRNOS:
             edata->sqlerrcode = ERRCODE_CONNECTION_FAILURE;
             break;

diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index f0587f41e4..fa798431ea 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -1825,10 +1825,15 @@ piperead(int s, char *buf, int len)
 {
     int            ret = recv(s, buf, len, 0);

-    if (ret < 0 && WSAGetLastError() == WSAECONNRESET)
+    if (ret < 0)
     {
-        /* EOF on the pipe! */
-        ret = 0;
+        int            werrno = TranslateSocketError(WSAGetLastError());
+
+        if (errno_is_connection_loss(werrno))
+        {
+            /* EOF on the pipe! */
+            ret = 0;
+        }
     }
     return ret;
 }
diff --git a/src/include/port.h b/src/include/port.h
index 84bf2c363f..766bb3dd6d 100644
--- a/src/include/port.h
+++ b/src/include/port.h
@@ -99,6 +99,36 @@ extern void pgfnames_cleanup(char **filenames);
 )
 #endif

+/*
+ * This macro provides a centralized list of all errnos that identify
+ * network-level failure of a previously-established TCP connection.  This
+ * excludes ECONNRESET, which we treat as reporting a probable server crash.
+ * (EPIPE is also excluded, since it's likewise a server-crash indication,
+ * and not TCP either.)  The macro is intended to be used in a switch
+ * statement, in the form "case ALL_NETWORK_FAILURE_ERRNOS:".
+ */
+#define ALL_NETWORK_FAILURE_ERRNOS \
+    ECONNABORTED: \
+    case EHOSTDOWN: \
+    case EHOSTUNREACH: \
+    case ENETDOWN: \
+    case ENETRESET: \
+    case ENETUNREACH
+
+/* Test for all connection-loss errnos, whether server or network failure */
+static inline bool
+errno_is_connection_loss(int e)
+{
+    switch (e)
+    {
+        case EPIPE:
+        case ECONNRESET:
+        case ALL_NETWORK_FAILURE_ERRNOS:
+            return true;
+    }
+    return false;
+}
+
 /* Portable locale initialization (in exec.c) */
 extern void set_pglocale_pgservice(const char *argv0, const char *app);

diff --git a/src/include/port/win32_port.h b/src/include/port/win32_port.h
index 8b6576b23d..e07baa555d 100644
--- a/src/include/port/win32_port.h
+++ b/src/include/port/win32_port.h
@@ -349,8 +349,16 @@ extern int    pgwin32_safestat(const char *path, struct stat *buf);
 #define EADDRINUSE WSAEADDRINUSE
 #undef EADDRNOTAVAIL
 #define EADDRNOTAVAIL WSAEADDRNOTAVAIL
+#undef EHOSTDOWN
+#define EHOSTDOWN WSAEHOSTDOWN
 #undef EHOSTUNREACH
 #define EHOSTUNREACH WSAEHOSTUNREACH
+#undef ENETDOWN
+#define ENETDOWN WSAENETDOWN
+#undef ENETRESET
+#define ENETRESET WSAENETRESET
+#undef ENETUNREACH
+#define ENETUNREACH WSAENETUNREACH
 #undef ENOTCONN
 #define ENOTCONN WSAENOTCONN

diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index ff840b7730..9bcbda7eb7 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -679,11 +679,9 @@ retry3:
         if (SOCK_ERRNO == EWOULDBLOCK)
             return someread;
 #endif
-        /* We might get ECONNRESET here if using TCP and backend died */
-#ifdef ECONNRESET
-        if (SOCK_ERRNO == ECONNRESET)
+        /* We might get ECONNRESET etc here if connection failed */
+        if (errno_is_connection_loss(SOCK_ERRNO))
             goto definitelyFailed;
-#endif
         /* pqsecure_read set the error message for us */
         return -1;
     }
@@ -769,11 +767,9 @@ retry4:
         if (SOCK_ERRNO == EWOULDBLOCK)
             return 0;
 #endif
-        /* We might get ECONNRESET here if using TCP and backend died */
-#ifdef ECONNRESET
-        if (SOCK_ERRNO == ECONNRESET)
+        /* We might get ECONNRESET etc here if connection failed */
+        if (errno_is_connection_loss(SOCK_ERRNO))
             goto definitelyFailed;
-#endif
         /* pqsecure_read set the error message for us */
         return -1;
     }
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 3311fd7a5b..97c3805303 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -261,14 +261,13 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t len)
                 /* no error message, caller is expected to retry */
                 break;

-#ifdef ECONNRESET
+            case EPIPE:
             case ECONNRESET:
                 printfPQExpBuffer(&conn->errorMessage,
                                   libpq_gettext("server closed the connection unexpectedly\n"
                                                 "\tThis probably means the server terminated abnormally\n"
                                                 "\tbefore or while processing the request.\n"));
                 break;
-#endif

             default:
                 printfPQExpBuffer(&conn->errorMessage,
@@ -374,11 +373,9 @@ retry_masked:
                 /* Set flag for EPIPE */
                 REMEMBER_EPIPE(spinfo, true);

-#ifdef ECONNRESET
                 /* FALL THRU */

             case ECONNRESET:
-#endif
                 printfPQExpBuffer(&conn->errorMessage,
                                   libpq_gettext("server closed the connection unexpectedly\n"
                                                 "\tThis probably means the server terminated abnormally\n"
diff --git a/src/interfaces/libpq/win32.h b/src/interfaces/libpq/win32.h
index c42d7abfe3..fcce1e0544 100644
--- a/src/interfaces/libpq/win32.h
+++ b/src/interfaces/libpq/win32.h
@@ -14,17 +14,6 @@
 #define write(a,b,c) _write(a,b,c)

 #undef EAGAIN                    /* doesn't apply on sockets */
-#undef EINTR
-#define EINTR WSAEINTR
-#ifndef EWOULDBLOCK
-#define EWOULDBLOCK WSAEWOULDBLOCK
-#endif
-#ifndef ECONNRESET
-#define ECONNRESET WSAECONNRESET
-#endif
-#ifndef EINPROGRESS
-#define EINPROGRESS WSAEINPROGRESS
-#endif

 /*
  * support for handling Windows Socket errors
diff --git a/src/port/strerror.c b/src/port/strerror.c
index 375edb0f5a..43a9761d90 100644
--- a/src/port/strerror.c
+++ b/src/port/strerror.c
@@ -146,16 +146,12 @@ get_errno_symbol(int errnum)
             return "EBUSY";
         case ECHILD:
             return "ECHILD";
-#ifdef ECONNABORTED
         case ECONNABORTED:
             return "ECONNABORTED";
-#endif
         case ECONNREFUSED:
             return "ECONNREFUSED";
-#ifdef ECONNRESET
         case ECONNRESET:
             return "ECONNRESET";
-#endif
         case EDEADLK:
             return "EDEADLK";
         case EDOM:
@@ -166,10 +162,10 @@ get_errno_symbol(int errnum)
             return "EFAULT";
         case EFBIG:
             return "EFBIG";
-#ifdef EHOSTUNREACH
+        case EHOSTDOWN:
+            return "EHOSTDOWN";
         case EHOSTUNREACH:
             return "EHOSTUNREACH";
-#endif
         case EIDRM:
             return "EIDRM";
         case EINPROGRESS:
@@ -198,6 +194,12 @@ get_errno_symbol(int errnum)
             return "EMSGSIZE";
         case ENAMETOOLONG:
             return "ENAMETOOLONG";
+        case ENETDOWN:
+            return "ENETDOWN";
+        case ENETRESET:
+            return "ENETRESET";
+        case ENETUNREACH:
+            return "ENETUNREACH";
         case ENFILE:
             return "ENFILE";
         case ENOBUFS:

В списке pgsql-hackers по дате отправления:

Предыдущее
От: Magnus Hagander
Дата:
Сообщение: Re: [PATCH] ecpg: fix progname memory leak
Следующее
От: Tom Lane
Дата:
Сообщение: Re: [PATCH] ecpg: fix progname memory leak