[PATCH v2] [libpq] Try to avoid manually masking SIGPIPEs on every send() - Mailing list pgsql-hackers

Currently, libpq will wrap each send() call on the connection with
two system calls to mask SIGPIPEs. This results in 3 syscalls instead
of one, and (on Linux) can lead to high contention on the signal
mask locks in threaded apps.

We have a couple of other methods to avoid SIGPIPEs:
sockopt(SO_NOSIGPIPE) and the MSG_NOSIGNAL flag to send().

This change attempts to use these if they're available at compile-
and run-time. If not, we drop back to manipulating the signal mask as
before.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>

---
v2: initialise optval

---src/interfaces/libpq/fe-connect.c |   40 ++++++++++++++++++src/interfaces/libpq/fe-secure.c  |   83
+++++++++++++++++++++++++++++---------src/interfaces/libpq/libpq-int.h |    2 3 files changed, 107 insertions(+), 18
deletions(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 7f4ae4c..92ab4e0 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1085,6 +1085,7 @@ keep_going:                        /* We will come back to here until there is
while(conn->addr_cur != NULL)                {                    struct addrinfo *addr_cur = conn->addr_cur;
 
+                    int optval;                    /* Remember current address for possible error msg */
    memcpy(&conn->raddr.addr, addr_cur->ai_addr,
 
@@ -1149,6 +1150,45 @@ keep_going:                        /* We will come back to here until there is
}#endif   /* F_SETFD */
 
+                    /* We have three methods of blocking sigpipe during
+                     * send() calls to this socket:
+                     *
+                     *  - setsockopt(sock, SO_NOSIGPIPE)
+                     *  - send(sock, ..., MSG_NOSIGNAL)
+                     *  - setting the signal mask to SIG_IGN during send()
+                     *
+                     * The first two reduce the number of syscalls (for the
+                     * third, we require three syscalls to implement a send()),
+                     * so use them if they're available. Their availability is
+                     * flagged in the following members of PGconn:
+                     *
+                     * conn->sigpipe_so        - we have set up SO_NOSIGPIPE
+                     * conn->sigpipe_flag    - we're specifying MSG_NOSIGNAL
+                     *
+                     * If we can use SO_NOSIGPIPE, then set sigpipe_so here and
+                     * we don't need to care about anything else. Otherwise,
+                     * try MSG_NOSIGNAL by setting sigpipe_flag. If we get an
+                     * error with MSG_NOSIGNAL, we clear the flag and revert
+                     * to manual masking.
+                     */
+                    conn->sigpipe_so = false;
+#ifdef MSG_NOSIGNAL
+                    conn->sigpipe_flag = true;
+#else /* !MSG_NOSIGNAL */
+                    conn->sigpipe_flag = false;
+#endif /* MSG_NOSIGNAL */
+
+#ifdef SO_NOSIGPIPE
+                    optval = 1;
+                    if (!setsockopt(conn->sock, SOL_SOCKET, SO_NOSIGPIPE,
+                            (char *)&optval, sizeof(optval)))
+                    {
+                        conn->sigpipe_so = true;
+                        conn->sigpipe_flag = false;
+                    }
+#endif /* SO_NOSIGPIPE */
+
+                    /*                     * Start/make connection.  This should not block, since we
 * are in nonblock mode.  If it does, well, too bad.
 
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index 13c97ac..949cd0f 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -122,6 +122,18 @@ static long win32_ssl_create_mutex = 0; */#ifndef WIN32
+
+static inline int sigpipe_masked(PGconn *conn)
+{
+    /* If we're on an SSL connection, we can only use SO_NOSIGPIPE masking.
+     * Otherwise, we can handle SO_NOSIGPIPE or the MSG_NOSIGNAL flag */
+#ifdef USE_SSL
+    if (conn->ssl)
+        return conn->sigpipe_so;
+#endif
+    return conn->sigpipe_so || conn->sigpipe_flag;
+}
+#ifdef ENABLE_THREAD_SAFETYstruct sigpipe_info {
@@ -130,8 +142,11 @@ struct sigpipe_info {    bool        got_epipe;};
-static inline int disable_sigpipe(struct sigpipe_info *info)
+static inline int disable_sigpipe(PGconn *conn, struct sigpipe_info *info){
+    if (sigpipe_masked(conn))
+        return 0;
+    info->got_epipe = false;    return pq_block_sigpipe(&info->oldsigmask, &info->sigpipe_pending) < 0;}
@@ -142,8 +157,11 @@ static inline void remember_epipe(struct sigpipe_info *info, bool cond)        info->got_epipe =
true;}
-static inline void restore_sigpipe(struct sigpipe_info *info)
+static inline void restore_sigpipe(PGconn *conn, struct sigpipe_info *info){
+    if (sigpipe_masked(conn))
+        return;
+    pq_reset_sigpipe(&info->oldsigmask, info->sigpipe_pending, info->got_epipe);}
@@ -153,9 +171,10 @@ struct sigpipe_info {    pqsigfunc    oldhandler;};
-static inline int disable_sigpipe(struct sigpipe_info *info)
+static inline int disable_sigpipe(PGconn *conn, struct sigpipe_info *info){
-    info->oldhandler = pqsignal(SIGPIPE, SIG_IGN);
+    if (!sigpipe_masked(conn))
+        info->oldhandler = pqsignal(SIGPIPE, SIG_IGN);    return 0;}
@@ -163,18 +182,22 @@ static inline void remember_epipe(struct sigpipe_info *info, bool cond){}
-static inline void restore_sigpipe(struct sigpipe_info *info)
+static inline void restore_sigpipe(PGconn *conn, struct sigpipe_info *info){
-    pqsignal(SIGPIPE, info->oldhandler);
+    if (!sigpipe_masked(conn))
+        pqsignal(SIGPIPE, info->oldhandler);}#endif    /* ENABLE_THREAD_SAFETY */#else    /* WIN32 */struct
sigpipe_info{ };
 
-static inline int disable_sigpipe(struct sigpipe_info *info) { return 0; }
+static inline int disable_sigpipe(PGConn *conn, struct sigpipe_info *info)
+{
+    return 0;
+}static inline void remember_epipe(struct sigpipe_info *info, bool cond) { }
-static inline void restore_sigpipe(struct sigpipe_info *info) { }
+static inline void restore_sigpipe(PGConn *conn, struct sigpipe_info *info) { }#endif    /* WIN32 */
@@ -306,7 +329,7 @@ pqsecure_read(PGconn *conn, void *ptr, size_t len)        struct sigpipe_info info;        /*
SSL_readcan write to the socket, so we need to disable SIGPIPE */
 
-        if (disable_sigpipe(&info))
+        if (disable_sigpipe(conn, &info))            return -1;rloop:
@@ -370,7 +393,7 @@ rloop:                break;        }
-        restore_sigpipe(&info);
+        restore_sigpipe(conn, &info);    }    else#endif
@@ -388,14 +411,14 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)    ssize_t        n;    struct
sigpipe_infoinfo;
 
-    if (disable_sigpipe(&info))
-        return -1;
-#ifdef USE_SSL    if (conn->ssl)    {        int            err;
+        if (disable_sigpipe(conn, &info))
+            return -1;
+        n = SSL_write(conn->ssl, ptr, len);        err = SSL_get_error(conn->ssl, n);        switch (err)
@@ -458,11 +481,35 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)    else#endif    {
-        n = send(conn->sock, ptr, len, 0);
-        remember_epipe(&info, n < 0 && SOCK_ERRNO == EPIPE);
+        int flags = 0;
+
+#ifdef MSG_NOSIGNAL
+        if (!conn->sigpipe_so && conn->sigpipe_flag)
+            flags |= MSG_NOSIGNAL;
+#endif /* MSG_NOSIGNAL */
+
+retry_masked:
+        if (disable_sigpipe(conn, &info))
+            return -1;
+
+        n = send(conn->sock, ptr, len, flags);
+
+        if (n < 0) {
+            /* if we see an EINVAL, it may be because MSG_NOSIGNAL isn't
+             * available on this machine. So, clear sigpipe_flag so we don't
+             * try this flag again, and retry the send().
+             */
+            if (flags != 0 && SOCK_ERRNO == EINVAL) {
+                conn->sigpipe_flag = false;
+                flags = 0;
+                goto retry_masked;
+            }
+
+            remember_epipe(&info, SOCK_ERRNO == EPIPE);
+        }    }
-    restore_sigpipe(&info);
+    restore_sigpipe(conn, &info);    return n;}
@@ -1219,14 +1266,14 @@ close_SSL(PGconn *conn)    if (conn->ssl)    {        struct sigpipe_info info;
-        disable_sigpipe(&info);
+        disable_sigpipe(conn, &info);        SSL_shutdown(conn->ssl);        SSL_free(conn->ssl);        conn->ssl =
NULL;       pqsecure_destroy();        /* We have to assume we got EPIPE */        remember_epipe(&info, true);
 
-        restore_sigpipe(&info);
+        restore_sigpipe(conn, &info);    }    if (conn->peer)
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 2340347..71dfefc 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -336,6 +336,8 @@ struct pg_conn    ProtocolVersion pversion;    /* FE/BE protocol version in use */    int
sversion;        /* server version, e.g. 70401 for 7.4.1 */    bool        password_needed;    /* true if server
demandeda password */
 
+    bool        sigpipe_so;        /* have we masked sigpipes via SO_NOSIGPIPE? */
+    bool        sigpipe_flag;    /* can we mask sigpipes via MSG_NOSIGNAL? */    /* Transient state needed while
establishingconnection */    struct addrinfo *addrlist;    /* list of possible backend addresses */