[PATCH v2] [libpq] Try to avoid manually masking SIGPIPEs on every send()

If the connection isn't over SSL, there's no need to do the disable.

This effectively halves the number of syscalls performed by libpq when
SSL is not in use.

Signed-off-by: Jeremy Kerr <jk(at)ozlabs(dot)org>

---
src/interfaces/libpq/fe-secure.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index eb579cf..2101315 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -368,13 +368,13 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
{
ssize_t n;

- DISABLE_SIGPIPE(return -1);
-
#ifdef USE_SSL
if (conn->ssl)
{
int err;

+ DISABLE_SIGPIPE(return -1);
+
n = SSL_write(conn->ssl, ptr, len);
err = SSL_get_error(conn->ssl, n);
switch (err)
@@ -433,15 +433,14 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
n = -1;
break;
}
+ RESTORE_SIGPIPE();
}
else
#endif
{
n = send(conn->sock, ptr, len, 0);
- REMEMBER_EPIPE(n < 0 && SOCK_ERRNO == EPIPE);
}

- RESTORE_SIGPIPE();

return n;
}

Currently, I'm seeing the psecure_{red,write} functions being invoked
when connecting to postgres via a unix domain socket. psecure_write
seems to alter the signal mask of the process to disable sigpipe
reporting. psecure_read only does this when the connection is using SSL.

When using a multithreaded client application on Linux, this can result
in poor scalability. Each change to the signal mask requires an
current->sighand->siglock, which becomes highly contended between
the client threads. It also means we do 3 syscalls per write: mask
sigpipe, write, unmask sigpipe.

The following patch changes psecure_write to be more like psecure_read -
it only alters the signal mask if the connection is over SSL. It's only
an RFC, as I'm not entirely sure about the reasoning behind blocking
SIGPIPE for the non-SSL case - there may be other considerations here.

With this change I see the following performance improvement
during a sysbench OLTP run:

http://ozlabs.org/~jk/projects/db/data/sigpipe-perf.png

load: sysbench --test=oltp --oltp-read-only=on, connecting locally,
machine: POWER6, 64-way, 4.2GHz

Comments most welcome,

Jeremy

---
Jeremy Kerr (1):
Only disable sigpipe during SSL write

Jeremy Kerr <jk(at)ozlabs(dot)org> writes:
> The following patch changes psecure_write to be more like psecure_read -
> it only alters the signal mask if the connection is over SSL. It's only
> an RFC, as I'm not entirely sure about the reasoning behind blocking
> SIGPIPE for the non-SSL case - there may be other considerations here.

The consideration is that the application fails completely on server
disconnect (because it gets SIGPIPE'd). This was long ago deemed
unacceptable, and we aren't likely to change our opinion on that.

What disturbs me about your report is the suggestion that there are
paths through that code that fail to protect against SIGPIPE. If so,
we need to fix that.

regards, tom lane

Tom,

> The consideration is that the application fails completely on server
> disconnect (because it gets SIGPIPE'd). This was long ago deemed
> unacceptable, and we aren't likely to change our opinion on that.

OK, understood. I'm guessing MSG_NOSIGNAL on the send() isn't portable
enough here?

> What disturbs me about your report is the suggestion that there are
> paths through that code that fail to protect against SIGPIPE. If so,
> we need to fix that.

I just missed the comment that pqsecure_read may end up writing to the
socket in the SSL case, so looks like all is fine here. We shouldn't see
a SIGPIPE from the recv() alone.

Cheers,

Jeremy

On 6/2/09, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jeremy Kerr <jk(at)ozlabs(dot)org> writes:
> > The following patch changes psecure_write to be more like psecure_read -
> > it only alters the signal mask if the connection is over SSL. It's only
> > an RFC, as I'm not entirely sure about the reasoning behind blocking
> > SIGPIPE for the non-SSL case - there may be other considerations here.
>
>
> The consideration is that the application fails completely on server
> disconnect (because it gets SIGPIPE'd). This was long ago deemed
> unacceptable, and we aren't likely to change our opinion on that.
>
> What disturbs me about your report is the suggestion that there are
> paths through that code that fail to protect against SIGPIPE. If so,
> we need to fix that.

Slightly OT, but why are we not using MSG_NOSIGNAL / SO_NOSIGPIPE
on OS'es that support them? I guess significant portion of userbase
has at least one of them available...

Thus avoiding 2 syscalls per operation plus potential locking issues.

--
marko

Jeremy Kerr <jk(at)ozlabs(dot)org> writes:
>> The consideration is that the application fails completely on server
>> disconnect (because it gets SIGPIPE'd). This was long ago deemed
>> unacceptable, and we aren't likely to change our opinion on that.

> OK, understood. I'm guessing MSG_NOSIGNAL on the send() isn't portable
> enough here?

Well, it's certainly not 100% portable, but I wouldn't object to a patch
that tests for it and uses it where it works.

One question that might be a bit hard to answer is whether mere
existence of the #define is sufficient evidence that the feature works.
We've had problems before with userland headers not being in sync
with what the kernel knows.

regards, tom lane

On 6/2/09, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jeremy Kerr <jk(at)ozlabs(dot)org> writes:
>
> >> The consideration is that the application fails completely on server
> >> disconnect (because it gets SIGPIPE'd). This was long ago deemed
> >> unacceptable, and we aren't likely to change our opinion on that.
>
> > OK, understood. I'm guessing MSG_NOSIGNAL on the send() isn't portable
> > enough here?
>
>
> Well, it's certainly not 100% portable, but I wouldn't object to a patch
> that tests for it and uses it where it works.
>
> One question that might be a bit hard to answer is whether mere
> existence of the #define is sufficient evidence that the feature works.
> We've had problems before with userland headers not being in sync
> with what the kernel knows.

Well, we could just test in configure perhaps? Runtime test is also
possible (if kernel gives error on unknown flag). Safest would
be enable on known-good OSes, maybe with version check?

--
marko

Marko Kreen <markokr(at)gmail(dot)com> writes:
> On 6/2/09, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> We've had problems before with userland headers not being in sync
>> with what the kernel knows.

> Well, we could just test in configure perhaps?

The single most common way to get into that kind of trouble is to
compile on machine A then install the executables on machine B with
a different kernel. So a configure test wouldn't give me any warm
feeling at all.

A feature that is exercised via setsockopt is probably fairly safe,
since you can check for failure of the setsockopt call and then do
it the old way. MSG_NOSIGNAL is a recv() flag, no? The question
is whether you could expect that the recv() would fail if it had
any unrecognized flags. Not sure if I trust that. SO_NOSIGPIPE
seems safer.

regards, tom lane

Tom,

> A feature that is exercised via setsockopt is probably fairly safe,
> since you can check for failure of the setsockopt call and then do
> it the old way. MSG_NOSIGNAL is a recv() flag, no?

It's a flag to send().

> The question is whether you could expect that the recv() would fail if
> it had any unrecognized flags. Not sure if I trust that. SO_NOSIGPIPE
> seems safer.

Yep, a once-off test would be better. However, I don't seem to have a
NOSIGPIPE sockopt here :(

Cheers,

Jeremy

On 6/2/09, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Marko Kreen <markokr(at)gmail(dot)com> writes:
> > On 6/2/09, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>
> >> We've had problems before with userland headers not being in sync
> >> with what the kernel knows.
>
> > Well, we could just test in configure perhaps?
>
>
> The single most common way to get into that kind of trouble is to
> compile on machine A then install the executables on machine B with
> a different kernel. So a configure test wouldn't give me any warm
> feeling at all.

Agreed. Another problem would be cross-compilation.

> A feature that is exercised via setsockopt is probably fairly safe,
> since you can check for failure of the setsockopt call and then do
> it the old way. MSG_NOSIGNAL is a recv() flag, no? The question
> is whether you could expect that the recv() would fail if it had
> any unrecognized flags. Not sure if I trust that. SO_NOSIGPIPE
> seems safer.

send(). The question is if the kernel would give error (good)
or simply ignore it (bad). I guess with MSG_NOSIGNAL only safe
way is to hardcode working OSes.

Are there any OS-es that have MSG_NOSIGNAL but not SO_NOSIGPIPE?

*grep* Eh, seems like Linux is such OS... But I also see it existing
as of Linux 2.2.0 in working state, so should be safe to use on linux
despite the kernel version.

--
marko

Jeremy Kerr <jk(at)ozlabs(dot)org> writes:
>> MSG_NOSIGNAL is a recv() flag, no?

> It's a flag to send().

Doh, need more caffeine.

>> The question is whether you could expect that the recv() would fail if
>> it had any unrecognized flags. Not sure if I trust that. SO_NOSIGPIPE
>> seems safer.

> Yep, a once-off test would be better. However, I don't seem to have a
> NOSIGPIPE sockopt here :(

On OS X I see SO_NOSIGPIPE but not MSG_NOSIGNAL. Seems like we might
have to support both if we want this to work as widely as possible.

The SUS man page for send() does explicitly specify an error code for
unrecognized flags bits, so maybe it's safe to assume that we'll get
an error if we set MSG_NOSIGNAL but the kernel doesn't recognize it.

regards, tom lane

A new approach to avioding manipulating the signal mask during for every
send - this time round, use SO_NOSIGPIPE and MSG_NOSIGNAL if available.

The patches have been tested on Linux and OSX, and I've confirmed that
'struct foo { };' will compile with a MSVC compiler. I'd still like a
little more testing though, is there a machine that allows both
SO_NOSIGPIPE and MSG_NOSIGNAL?

Again, comments most welcome,

Jeremy

---
Jeremy Kerr (2):
[libpq] rework sigpipe-handling macros
[libpq] Try to avoid manually masking SIGPIPEs on every send()

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(at)ozlabs(dot)org>

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

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 7f4ae4c..8265268 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,44 @@ 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
+ 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_SAFETY

struct 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_read can 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_info info;

- 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 demanded a 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 establishing connection */
struct addrinfo *addrlist; /* list of possible backend addresses */

Currently, the sigpipe-masking code in libpq is implemented as
a set of macros, which depend on declaring local variables.

This change adds a (private) struct sigpipe_info to contain the
compile-dependent data required for sigpipe masking and restoring.
The caller can then declare a struct sigpipe info explicitly, and
pass this to the subsequent sigpipe-masking code.

This allows us to separate the variable declarations from the code,
and gives the caller more flexibility for controlling the scope of
these variables.

Also, since we don't need to declare variables in the macros, we
can change the code to be implemented as static inlines.

Signed-off-by: Jeremy Kerr <jk(at)ozlabs(dot)org>

---
src/interfaces/libpq/fe-secure.c | 88 ++++++++++++++++++++++++---------------
1 file changed, 55 insertions(+), 33 deletions(-)

diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index ee0a91e..13c97ac 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -119,45 +119,62 @@ static long win32_ssl_create_mutex = 0;

/*
* Macros to handle disabling and then restoring the state of SIGPIPE handling.
- * Note that DISABLE_SIGPIPE() must appear at the start of a block.
*/

#ifndef WIN32
#ifdef ENABLE_THREAD_SAFETY

-#define DISABLE_SIGPIPE(failaction) \
- sigset_t osigmask; \
- bool sigpipe_pending; \
- bool got_epipe = false; \
-\
- if (pq_block_sigpipe(&osigmask, &sigpipe_pending) < 0) \
- failaction
+struct sigpipe_info {
+ sigset_t oldsigmask;
+ bool sigpipe_pending;
+ bool got_epipe;
+};

-#define REMEMBER_EPIPE(cond) \
- do { \
- if (cond) \
- got_epipe = true; \
- } while (0)
+static inline int disable_sigpipe(struct sigpipe_info *info)
+{
+ info->got_epipe = false;
+ return pq_block_sigpipe(&info->oldsigmask, &info->sigpipe_pending) < 0;
+}

-#define RESTORE_SIGPIPE() \
- pq_reset_sigpipe(&osigmask, sigpipe_pending, got_epipe)
+static inline void remember_epipe(struct sigpipe_info *info, bool cond)
+{
+ if (cond)
+ info->got_epipe = true;
+}
+
+static inline void restore_sigpipe(struct sigpipe_info *info)
+{
+ pq_reset_sigpipe(&info->oldsigmask, info->sigpipe_pending, info->got_epipe);
+}

#else /* !ENABLE_THREAD_SAFETY */

-#define DISABLE_SIGPIPE(failaction) \
- pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN)
+struct sigpipe_info {
+ pqsigfunc oldhandler;
+};

-#define REMEMBER_EPIPE(cond)
+static inline int disable_sigpipe(struct sigpipe_info *info)
+{
+ info->oldhandler = pqsignal(SIGPIPE, SIG_IGN);
+ return 0;
+}
+
+static inline void remember_epipe(struct sigpipe_info *info, bool cond)
+{
+}

-#define RESTORE_SIGPIPE() \
- pqsignal(SIGPIPE, oldsighandler)
+static inline void restore_sigpipe(struct sigpipe_info *info)
+{
+ pqsignal(SIGPIPE, info->oldhandler);
+}

#endif /* ENABLE_THREAD_SAFETY */
#else /* WIN32 */

-#define DISABLE_SIGPIPE(failaction)
-#define REMEMBER_EPIPE(cond)
-#define RESTORE_SIGPIPE()
+struct sigpipe_info { };
+static inline int disable_sigpipe(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) { }

#endif /* WIN32 */

@@ -286,9 +303,11 @@ pqsecure_read(PGconn *conn, void *ptr, size_t len)
if (conn->ssl)
{
int err;
+ struct sigpipe_info info;

/* SSL_read can write to the socket, so we need to disable SIGPIPE */
- DISABLE_SIGPIPE(return -1);
+ if (disable_sigpipe(&info))
+ return -1;

rloop:
n = SSL_read(conn->ssl, ptr, len);
@@ -315,7 +334,7 @@ rloop:

if (n == -1)
{
- REMEMBER_EPIPE(SOCK_ERRNO == EPIPE);
+ remember_epipe(&info, SOCK_ERRNO == EPIPE);
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSL SYSCALL error: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
@@ -351,7 +370,7 @@ rloop:
break;
}

- RESTORE_SIGPIPE();
+ restore_sigpipe(&info);
}
else
#endif
@@ -367,8 +386,10 @@ ssize_t
pqsecure_write(PGconn *conn, const void *ptr, size_t len)
{
ssize_t n;
+ struct sigpipe_info info;

- DISABLE_SIGPIPE(return -1);
+ if (disable_sigpipe(&info))
+ return -1;

#ifdef USE_SSL
if (conn->ssl)
@@ -399,7 +420,7 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)

if (n == -1)
{
- REMEMBER_EPIPE(SOCK_ERRNO == EPIPE);
+ remember_epipe(&info, SOCK_ERRNO == EPIPE);
printfPQExpBuffer(&conn->errorMessage,
libpq_gettext("SSL SYSCALL error: %s\n"),
SOCK_STRERROR(SOCK_ERRNO, sebuf, sizeof(sebuf)));
@@ -438,10 +459,10 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
#endif
{
n = send(conn->sock, ptr, len, 0);
- REMEMBER_EPIPE(n < 0 && SOCK_ERRNO == EPIPE);
+ remember_epipe(&info, n < 0 && SOCK_ERRNO == EPIPE);
}

- RESTORE_SIGPIPE();
+ restore_sigpipe(&info);

return n;
}
@@ -1197,14 +1218,15 @@ close_SSL(PGconn *conn)
{
if (conn->ssl)
{
- DISABLE_SIGPIPE((void) 0);
+ struct sigpipe_info info;
+ disable_sigpipe(&info);
SSL_shutdown(conn->ssl);
SSL_free(conn->ssl);
conn->ssl = NULL;
pqsecure_destroy();
/* We have to assume we got EPIPE */
- REMEMBER_EPIPE(true);
- RESTORE_SIGPIPE();
+ remember_epipe(&info, true);
+ restore_sigpipe(&info);
}

if (conn->peer)

On 6/10/09, Jeremy Kerr <jk(at)ozlabs(dot)org> wrote:
> + int optval;

> + if (!setsockopt(conn->sock, SOL_SOCKET, SO_NOSIGPIPE,
> + (char *)&optval, sizeof(optval)))

optval seems used without initialization.

But generally, I like it. +1

--
marko

Marko,

> optval seems used without initialization.

Dang, I was checking for the SO_NOSIGPIPE flag, and didn't check for
optval. New patch coming...

Cheers,

Jeremy

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(at)ozlabs(dot)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_SAFETY

struct 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_read can 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_info info;

- 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 demanded a 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 establishing connection */
struct addrinfo *addrlist; /* list of possible backend addresses */