Re: SIGPIPE handling

Hi,

attached is an update of my automatic sigaction patch: I've moved the
actual sigaction calls into pqsignal.c and added a helper function
(pgsignalinquire(signo)). I couldn't remove the include <signal.h> from
fe-connect.c: it's required for the SIGPIPE definition.
Additionally I've added a -a flag for pgbench that sets the signal
handler before calling PQconnectdb.

Tested on Fedora Core 1 (Redhat Linux) with pgbench.

--
Manfred

Better. However, I am confused over when we do sigaction. I thought we
were going to do it only if they had a signal handler defined, meaning

if (pipehandler != SIG_DFL &&
pipehandler != SIG_IGN &&
pipehandler != SIG_ERR)
conn->do_sigaction = true;
else
conn->do_sigaction = false;

By doing this, we don't do sigaction in the default case where no
handler was defined. I thought we would just set the entire application
to SIGPIPE <= SIG_IGN. This gives us good performance in all cases
except when a signal handler is defined. Is running the rest of the
application with SIGPIPE <= SIG_IGN a problem?

However, the code patch is:

if (pipehandler == SIG_DFL || pipehandler == SIG_ERR)
conn->do_sigaction = true;
else
conn->do_sigaction = false;

This gives us good performance only if SIGPIPE <= SIG_IGN has been set
by the application or a sigaction function has been defined.

---------------------------------------------------------------------------

Manfred Spraul wrote:
> Hi,
>
> attached is an update of my automatic sigaction patch: I've moved the
> actual sigaction calls into pqsignal.c and added a helper function
> (pgsignalinquire(signo)). I couldn't remove the include <signal.h> from
> fe-connect.c: it's required for the SIGPIPE definition.
> Additionally I've added a -a flag for pgbench that sets the signal
> handler before calling PQconnectdb.
>
> Tested on Fedora Core 1 (Redhat Linux) with pgbench.
>
> --
> Manfred

> Index: src/interfaces/libpq/fe-connect.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
> retrieving revision 1.263
> diff -c -r1.263 fe-connect.c
> *** src/interfaces/libpq/fe-connect.c 18 Oct 2003 05:02:06 -0000 1.263
> --- src/interfaces/libpq/fe-connect.c 16 Nov 2003 11:44:47 -0000
> ***************
> *** 41,46 ****
> --- 41,48 ----
> #include <netinet/tcp.h>
> #endif
> #include <arpa/inet.h>
> + #include <signal.h>
> + #include "pqsignal.h"
> #endif
>
> #include "libpq/ip.h"
> ***************
> *** 881,886 ****
> --- 883,891 ----
> struct addrinfo hint;
> const char *node = NULL;
> int ret;
> + #ifndef WIN32
> + pqsigfunc pipehandler;
> + #endif
>
> if (!conn)
> return 0;
> ***************
> *** 950,955 ****
> --- 955,976 ----
> conn->allow_ssl_try = false;
> else if (conn->sslmode[0] == 'a') /* "allow" */
> conn->wait_ssl_try = true;
> + #endif
> + #ifndef WIN32
> + /*
> + * Autodetect SIGPIPE signal handling:
> + * The default action per Unix spec is kill current process and
> + * that's not acceptable. If the current setting is not the default,
> + * then assume that the caller knows what he's doing and leave the
> + * signal handler unchanged. Otherwise set the signal handler to
> + * SIG_IGN around each send() syscall. Unfortunately this is both
> + * unreliable and slow for multithreaded apps.
> + */
> + pipehandler = pqsignalinquire(SIGPIPE);
> + if (pipehandler == SIG_DFL || pipehandler == SIG_ERR)
> + conn->do_sigaction = true;
> + else
> + conn->do_sigaction = false;
> #endif
>
> /*
> Index: src/interfaces/libpq/fe-secure.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-secure.c,v
> retrieving revision 1.32
> diff -c -r1.32 fe-secure.c
> *** src/interfaces/libpq/fe-secure.c 29 Sep 2003 16:38:04 -0000 1.32
> --- src/interfaces/libpq/fe-secure.c 16 Nov 2003 11:44:47 -0000
> ***************
> *** 348,354 ****
> ssize_t n;
>
> #ifndef WIN32
> ! pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
> #endif
>
> #ifdef USE_SSL
> --- 348,357 ----
> ssize_t n;
>
> #ifndef WIN32
> ! pqsigfunc oldsighandler = NULL;
> !
> ! if (conn->do_sigaction)
> ! oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
> #endif
>
> #ifdef USE_SSL
> ***************
> *** 408,414 ****
> n = send(conn->sock, ptr, len, 0);
>
> #ifndef WIN32
> ! pqsignal(SIGPIPE, oldsighandler);
> #endif
>
> return n;
> --- 411,418 ----
> n = send(conn->sock, ptr, len, 0);
>
> #ifndef WIN32
> ! if (conn->do_sigaction)
> ! pqsignal(SIGPIPE, oldsighandler);
> #endif
>
> return n;
> Index: src/interfaces/libpq/libpq-int.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
> retrieving revision 1.82
> diff -c -r1.82 libpq-int.h
> *** src/interfaces/libpq/libpq-int.h 5 Sep 2003 02:08:36 -0000 1.82
> --- src/interfaces/libpq/libpq-int.h 16 Nov 2003 11:44:48 -0000
> ***************
> *** 329,334 ****
> --- 329,337 ----
> char peer_dn[256 + 1]; /* peer distinguished name */
> char peer_cn[SM_USER + 1]; /* peer common name */
> #endif
> + #ifndef WIN32
> + bool do_sigaction; /* set SIGPIPE to SIG_IGN around every send() call */
> + #endif
>
> /* Buffer for current error message */
> PQExpBufferData errorMessage; /* expansible string */
> Index: src/interfaces/libpq/pqsignal.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/pqsignal.c,v
> retrieving revision 1.17
> diff -c -r1.17 pqsignal.c
> *** src/interfaces/libpq/pqsignal.c 4 Aug 2003 02:40:20 -0000 1.17
> --- src/interfaces/libpq/pqsignal.c 16 Nov 2003 11:44:48 -0000
> ***************
> *** 40,42 ****
> --- 40,61 ----
> return oact.sa_handler;
> #endif /* !HAVE_POSIX_SIGNALS */
> }
> +
> + pqsigfunc
> + pqsignalinquire(int signo)
> + {
> + #if !defined(HAVE_POSIX_SIGNALS)
> + pqsigfunc old;
> + old = signal(SIGPIPE, SIG_IGN);
> + signal(SIGPIPE, old);
> + return old;
> + #else
> + struct sigaction oact;
> +
> + if (sigaction(SIGPIPE, NULL, &oact) != 0)
> + return SIG_ERR;
> + return oact.sa_handler;
> + #endif /* !HAVE_POSIX_SIGNALS */
> +
> +
> + }
> Index: src/interfaces/libpq/pqsignal.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/pqsignal.h,v
> retrieving revision 1.15
> diff -c -r1.15 pqsignal.h
> *** src/interfaces/libpq/pqsignal.h 4 Aug 2003 02:40:20 -0000 1.15
> --- src/interfaces/libpq/pqsignal.h 16 Nov 2003 11:44:48 -0000
> ***************
> *** 24,27 ****
> --- 24,29 ----
>
> extern pqsigfunc pqsignal(int signo, pqsigfunc func);
>
> + extern pqsigfunc pqsignalinquire(int signo);
> +
> #endif /* PQSIGNAL_H */

> Index: contrib/pgbench/README.pgbench
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/contrib/pgbench/README.pgbench,v
> retrieving revision 1.9
> diff -c -r1.9 README.pgbench
> *** contrib/pgbench/README.pgbench 10 Jun 2003 09:07:15 -0000 1.9
> --- contrib/pgbench/README.pgbench 16 Nov 2003 11:44:39 -0000
> ***************
> *** 112,117 ****
> --- 112,121 ----
> might be a security hole since ps command will
> show the password. Use this for TESTING PURPOSE ONLY.
>
> + -a
> + Disable SIGPIPE delivery globally instead of within each
> + libpq operation.
> +
> -n
> No vacuuming and cleaning the history table prior to the
> test is performed.
> Index: contrib/pgbench/pgbench.c
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/contrib/pgbench/pgbench.c,v
> retrieving revision 1.27
> diff -c -r1.27 pgbench.c
> *** contrib/pgbench/pgbench.c 27 Sep 2003 19:15:34 -0000 1.27
> --- contrib/pgbench/pgbench.c 16 Nov 2003 11:44:39 -0000
> ***************
> *** 28,33 ****
> --- 28,34 ----
> #else
> #include <sys/time.h>
> #include <unistd.h>
> + #include <signal.h>
>
> #ifdef HAVE_GETOPT_H
> #include <getopt.h>
> ***************
> *** 105,112 ****
> static void
> usage()
> {
> ! fprintf(stderr, "usage: pgbench [-h hostname][-p port][-c nclients][-t ntransactions][-s scaling_factor][-n][-C][-v][-S][-N][-l][-U login][-P password][-d][dbname]\n");
> ! fprintf(stderr, "(initialize mode): pgbench -i [-h hostname][-p port][-s scaling_factor][-U login][-P password][-d][dbname]\n");
> }
>
> /* random number generator */
> --- 106,113 ----
> static void
> usage()
> {
> ! fprintf(stderr, "usage: pgbench [-h hostname][-p port][-c nclients][-t ntransactions][-s scaling_factor][-n][-C][-v][-S][-N][-l][-a][-U login][-P password][-d][dbname]\n");
> ! fprintf(stderr, "(initialize mode): pgbench -i [-h hostname][-p port][-s scaling_factor][-U login][-P password][-d][dbname][-a]\n");
> }
>
> /* random number generator */
> ***************
> *** 703,712 ****
> else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
> login = env;
>
> ! while ((c = getopt(argc, argv, "ih:nvp:dc:t:s:U:P:CNSl")) != -1)
> {
> switch (c)
> {
> case 'i':
> is_init_mode++;
> break;
> --- 704,718 ----
> else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
> login = env;
>
> ! while ((c = getopt(argc, argv, "aih:nvp:dc:t:s:U:P:CNSl")) != -1)
> {
> switch (c)
> {
> + case 'a':
> + #ifndef WIN32
> + signal(SIGPIPE, SIG_IGN);
> + #endif
> + break;
> case 'i':
> is_init_mode++;
> break;

>
> ---------------------------(end of broadcast)---------------------------
> TIP 4: Don't 'kill -9' the postmaster

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Is running the rest of the
> application with SIGPIPE <= SIG_IGN a problem?

That is NOT an acceptable thing for a library to do.

regards, tom lane

Bruce Momjian wrote:

>Better. However, I am confused over when we do sigaction. I thought we
>were going to do it only if they had a signal handler defined, meaning
>
> if (pipehandler != SIG_DFL &&
> pipehandler != SIG_IGN &&
> pipehandler != SIG_ERR)
> conn->do_sigaction = true;
> else
> conn->do_sigaction = false;
>
>By doing this, we don't do sigaction in the default case where no
>handler was defined.
>
No. If no handler was definied, then libpq must define a handler.
Without a handler, a network disconnect would result in a SIGPIE that
kills the app.

> I thought we would just set the entire application
>to SIGPIPE <= SIG_IGN. This gives us good performance in all cases
>except when a signal handler is defined.
>
I don't want to change the whole app - perhaps someone expects that
sigpipe works? Perhaps psql for the console input, or something similar?

> Is running the rest of the
>application with SIGPIPE <= SIG_IGN a problem?
>
>
I think that depends on the application, and libpq shouldn't mandate
that SIGPIPE must be SIG_IGN. Right now libpq tries to catch sigpipe
signals by manually installing/restoring a signal handler around send()
calls. This doesn't work for multithreaded apps, because the signal
handlers are per-process, not per-thread.

Thus for multithreaded apps, the libpq user is responsible for handling
sigpipe. The API change should be a big problem - the current system
doesn't work, and there shouldn't be many multithreaded apps.

But how should libpq notice that the caller handles sigpipe signals?
a) autodetection - if the sigpipe handler is not the default, then the
caller knows what he's doing.
b) a new PGsetsignalhandler() function.
c) an additional flag passed to PGconnectdb.

Tom preferred a). One problem is that the autodetection is not perfect:
an app could block the signal with sigprocmask, or it could install a
handler that doesn't expect sigpipe signals from within libpq.
I would prefer b), because it guarantees that the patch has no effect on
existing apps.
c) is bad, Tom explained that the connect string is often directly
specified by the user.

--
Manfred

On Sun, Nov 16, 2003 at 12:56:10PM +0100, Manfred Spraul wrote:
> Hi,
>
> attached is an update of my automatic sigaction patch: I've moved the
> actual sigaction calls into pqsignal.c and added a helper function
> (pgsignalinquire(signo)). I couldn't remove the include <signal.h> from
> fe-connect.c: it's required for the SIGPIPE definition.
> Additionally I've added a -a flag for pgbench that sets the signal
> handler before calling PQconnectdb.

Is there a reason we don't make use of the MSG_NOSIGNAL flag to
send()? Or is the problem in case of SSL?

Kurt

On Sun, Nov 16, 2003 at 06:28:06PM +0100, Kurt Roeckx wrote:
> On Sun, Nov 16, 2003 at 12:56:10PM +0100, Manfred Spraul wrote:
> > Hi,
> >
> > attached is an update of my automatic sigaction patch: I've moved the
> > actual sigaction calls into pqsignal.c and added a helper function
> > (pgsignalinquire(signo)). I couldn't remove the include <signal.h> from
> > fe-connect.c: it's required for the SIGPIPE definition.
> > Additionally I've added a -a flag for pgbench that sets the signal
> > handler before calling PQconnectdb.
>
> Is there a reason we don't make use of the MSG_NOSIGNAL flag to
> send()? Or is the problem in case of SSL?

Oh, seems to be a Linux only thing?

Kurt

Manfred Spraul <manfred(at)colorfullife(dot)com> writes:
> But how should libpq notice that the caller handles sigpipe signals?
> a) autodetection - if the sigpipe handler is not the default, then the
> caller knows what he's doing.
> b) a new PGsetsignalhandler() function.
> c) an additional flag passed to PGconnectdb.

> Tom preferred a). One problem is that the autodetection is not perfect:
> an app could block the signal with sigprocmask, or it could install a
> handler that doesn't expect sigpipe signals from within libpq.
> I would prefer b), because it guarantees that the patch has no effect on
> existing apps.

I have no particular objection to (b) either, but IIRC there was some
dispute about whether it sets a global or per-connection flag. ISTM
that "I have a correct signal handler" is a global assertion (within one
process) and so a global flag is appropriate. Someone else (Bruce?)
didn't like that though.

regards, tom lane

Kurt Roeckx <Q(at)ping(dot)be> writes:
> On Sun, Nov 16, 2003 at 06:28:06PM +0100, Kurt Roeckx wrote:
>> Is there a reason we don't make use of the MSG_NOSIGNAL flag to
>> send()? Or is the problem in case of SSL?

> Oh, seems to be a Linux only thing?

That and the SSL problem. I wouldn't object to implementing it as a
platform-specific optimization if we could get it to handle the SSL
case, but without SSL support I think it's too limited.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Is running the rest of the
> > application with SIGPIPE <= SIG_IGN a problem?
>
> That is NOT an acceptable thing for a library to do.

Yes, I was afraid of that. Here's another idea. If the signal handler
is SIG_DFL, we install our own signal handler for SIGPIPE, and set/clear a
global variable before/after we send(). When our signal handler is
called, we check to see if our global variable is set, and we either
ignore or exit(). Can we do that safely? Seems it only fails when they
register a signal handler after establishing a database connection.

How would this work in a threaded app --- not too well, I think.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> Yes, I was afraid of that. Here's another idea. If the signal handler
> is SIG_DFL, we install our own signal handler for SIGPIPE, and set/clear a
> global variable before/after we send().

That would address the speed issue but not the multithread correctness
issue. Also, what happens if the app replaces the signal handler later?

regards, tom lane

Tom Lane wrote:
> Manfred Spraul <manfred(at)colorfullife(dot)com> writes:
> > But how should libpq notice that the caller handles sigpipe signals?
> > a) autodetection - if the sigpipe handler is not the default, then the
> > caller knows what he's doing.
> > b) a new PGsetsignalhandler() function.
> > c) an additional flag passed to PGconnectdb.
>
> > Tom preferred a). One problem is that the autodetection is not perfect:
> > an app could block the signal with sigprocmask, or it could install a
> > handler that doesn't expect sigpipe signals from within libpq.
> > I would prefer b), because it guarantees that the patch has no effect on
> > existing apps.
>
> I have no particular objection to (b) either, but IIRC there was some
> dispute about whether it sets a global or per-connection flag. ISTM
> that "I have a correct signal handler" is a global assertion (within one
> process) and so a global flag is appropriate. Someone else (Bruce?)
> didn't like that though.

I thought it should be global too, basically testing on the first
connection request.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

>I thought it should be global too, basically testing on the first
>connection request.
>
What if two PQconnect calls happen at the same time?
I would really prefer the manual approach with a new PQsetsighandler
function - the autodetection is fragile, it's trivial to find a special
case where it breaks.
Bruce, you wrote that a new function would be overdesign. Are you sure?
Your simpler proposals all fail with multithreaded apps.
I've attached the patch that implements the global flag with two special
function that access it.

--
Manfred

Manfred Spraul <manfred(at)colorfullife(dot)com> writes:
> + extern void PQsetsighandling(int internal_sigign);

These sorts of things are commonly designed so that the set() operation
incidentally returns the previous setting. I'm not sure if anyone would
care, but it's only a couple more lines of code to make that happen, so
I'd suggest doing so just in case.

Otherwise I think this is a good patch. The documentation could use a
little more wordsmithing, perhaps.

regards, tom lane

Tom Lane wrote:
> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > Yes, I was afraid of that. Here's another idea. If the signal handler
> > is SIG_DFL, we install our own signal handler for SIGPIPE, and set/clear a
> > global variable before/after we send().
>
> That would address the speed issue but not the multithread correctness
> issue. Also, what happens if the app replaces the signal handler later?

Well, our current setup doesn't do multithreaded properly either. In
fact, I am starting to worry about libpq's thread-safety. Should I?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Manfred Spraul wrote:
> Bruce Momjian wrote:
>
> >I thought it should be global too, basically testing on the first
> >connection request.
> >
> What if two PQconnect calls happen at the same time?
> I would really prefer the manual approach with a new PQsetsighandler
> function - the autodetection is fragile, it's trivial to find a special
> case where it breaks.
> Bruce, you wrote that a new function would be overdesign. Are you sure?
> Your simpler proposals all fail with multithreaded apps.
> I've attached the patch that implements the global flag with two special
> function that access it.

Here is my logic --- 99% of apps don't install a SIGPIPE signal handler,
and 90% will not add a SIGPIPE/SIG_IGN call to their applications. I
guess I am looking for something that would allow the performance
benefit of not doing a pgsignal() call around very send() for the
majority of our apps. What was the speed improvement?

Just the fact you had to add the SIG_IGN call to pgbench shows that most
apps need some special handling to get this performance benefit, and I
would like to avoid that.

Your PQsetsighandler() idea --- would that be fore SIGPIPE only? Would
it be acceptable to tell application developers they have to use
PQsetsig*pipe*handler() call to register a SIGPIPE handler? If so, that
would be great because we would do the pgsignal call around send() only
when it was needed. It might be the cleanest way and the most reliable.

Granted, we need to do something because our current setup isn't even
thread-safe. Also, how is your patch more thread-safe than the old one?
The detection is thread-safe, but I don't see how the use is. If you
still pgsignal around the calls, I don't see how two threads couldn't
do:

thread 1 thread 2
-------- --------
pgsignal(SIGPIPE, SIG_IGN);
pgsignal(SIGPIPE, SIG_DFL);
send();
pgsignal(SIGPIPE, SIG_DFL);

send();
pgsignal(SIGPIPE, SIG_DFL);

This runs thread1 with SIGPIPE as SIG_DFL.

What are we ignoring the SIGPIPE for on send anyway? Is this in case
the backend crashed?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

>Here is my logic --- 99% of apps don't install a SIGPIPE signal handler,
>and 90% will not add a SIGPIPE/SIG_IGN call to their applications. I
>guess I am looking for something that would allow the performance
>benefit of not doing a pgsignal() call around very send() for the
>majority of our apps. What was the speed improvement?
>
>
Around 10% for a heavily multithreaded app on an 8-way Xeon server. Far
less for a single threaded app and far less for uniprocessor systems:
the kernel must update the pending queue of all threads and that causes
lots of contention for the (per-process) spinlock that protects the
signal handlers.

>Granted, we need to do something because our current setup isn't even
>thread-safe. Also, how is your patch more thread-safe than the old one?
>The detection is thread-safe, but I don't see how the use is.
>
First function in main():

signal(SIGPIPE, SIG_IGN);
PQsetsighandling(1);

This results in perfectly thread-safe sigpipe handling. If it's a
multithreaded app that needs correct correct per-thread delivery of
SIGPIPE signals for console IO, then the libpq user must implement the
sequence I describe below.

> If you
>still pgsignal around the calls, I don't see how two threads couldn't
>do:
>
> thread 1 thread 2
> -------- --------
> pgsignal(SIGPIPE, SIG_IGN);
> pgsignal(SIGPIPE, SIG_DFL);
> send();
> pgsignal(SIGPIPE, SIG_DFL);
>
> send();
> pgsignal(SIGPIPE, SIG_DFL);
>
>This runs thread1 with SIGPIPE as SIG_DFL.
>
>
Correct. A thread safe sequence might be something like:

pthread_sigmask(SIG_BLOCK,{SIGPIPE});
send();
if (sigpending(SIGPIPE) {
sigwait({SIGPIPE},);
}
pthread_sigmask(SIG_UNBLOCK,{SIGPIPE});

But this sequence only works for users that link against libpthread. And
the same sequence with sigprocmask is undefined for multithreaded apps.

--
Manfred

Manfred Spraul wrote:
> Bruce Momjian wrote:
>
> >Here is my logic --- 99% of apps don't install a SIGPIPE signal handler,
> >and 90% will not add a SIGPIPE/SIG_IGN call to their applications. I
> >guess I am looking for something that would allow the performance
> >benefit of not doing a pgsignal() call around very send() for the
> >majority of our apps. What was the speed improvement?
> >
> >
> Around 10% for a heavily multithreaded app on an 8-way Xeon server. Far
> less for a single threaded app and far less for uniprocessor systems:
> the kernel must update the pending queue of all threads and that causes
> lots of contention for the (per-process) spinlock that protects the
> signal handlers.

OK, I know you had a flag for pgbench, and that doesn't use threads.
What speedup do you see there?

> >Granted, we need to do something because our current setup isn't even
> >thread-safe. Also, how is your patch more thread-safe than the old one?
> >The detection is thread-safe, but I don't see how the use is.
> >
> First function in main():
>
> signal(SIGPIPE, SIG_IGN);
> PQsetsighandling(1);
>
> This results in perfectly thread-safe sigpipe handling. If it's a
> multithreaded app that needs correct correct per-thread delivery of
> SIGPIPE signals for console IO, then the libpq user must implement the
> sequence I describe below.

I would not expect a library to require me to do something in my code to
be thread-safe --- either it is or it isn't. I wonder if we should use
the sequence you list below when we compile using
--enable-thread-safety. We already use thread calls in port/thread.c,
specifically pthread_mutex_lock(). Why not make it work 100% if then
enable that build option?

> >This runs thread1 with SIGPIPE as SIG_DFL.
> >
> >
> Correct. A thread safe sequence might be something like:
>
> pthread_sigmask(SIG_BLOCK,{SIGPIPE});
> send();
> if (sigpending(SIGPIPE) {
> sigwait({SIGPIPE},);
> }
> pthread_sigmask(SIG_UNBLOCK,{SIGPIPE});
>
> But this sequence only works for users that link against libpthread. And
> the same sequence with sigprocmask is undefined for multithreaded apps.

Again, let's get it working perfect if they say they are going to use
threads with libpq. Does it work OK if the app doesn't use threading?

Does sigpending/sigwait work efficiently for threads? Another idea is
to go with a thread-local storage boolean for each thread, and check
that in a signal handler we install. Seems synchronous signals like
SIGPIPE are delivered to the thread that invoked them, and we can check
thread-local storage to see if we were in a send() loop at the time of
signal delivery.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

>OK, I know you had a flag for pgbench, and that doesn't use threads.
>What speedup do you see there?
>
>
Tiny. I added the flag to check that my implementation works, not as a
benchmark tool.

>I would not expect a library to require me to do something in my code to
>be thread-safe --- either it is or it isn't.
>
The library is thread-safe. Just the SIGPIPE handling differs:
- single thread: handled by libpq.
- multi thread: caller must handle SIGPIPE for libpq.
Rationale: posix is broken. Per-thread signal handling is too ugly to
think about.

>Again, let's get it working perfect if they say they are going to use
>threads with libpq. Does it work OK if the app doesn't use threading?
>
>
No. pthread_sigmask is part of libpthread - libpq would have to link
unconditionally against libpthread. Or use __attribute__((weak,
alias())), but that would only work with gcc.

>Does sigpending/sigwait work efficiently for threads? Another idea is
>to go with a thread-local storage boolean for each thread, and check
>that in a signal handler we install.
>
I think installing a signal handler is not an option - libpq is a
library, the signal handler is global.

> Seems synchronous signals like
>SIGPIPE are delivered to the thread that invoked them, and we can check
>thread-local storage to see if we were in a send() loop at the time of
>signal delivery.
>
>
IMHO way to fragile.

--
Manfred

Manfred Spraul wrote:
> Bruce Momjian wrote:
>
> >OK, I know you had a flag for pgbench, and that doesn't use threads.
> >What speedup do you see there?
> >
> >
> Tiny. I added the flag to check that my implementation works, not as a
> benchmark tool.
>
> >I would not expect a library to require me to do something in my code to
> >be thread-safe --- either it is or it isn't.
> >
> The library is thread-safe. Just the SIGPIPE handling differs:
> - single thread: handled by libpq.
> - multi thread: caller must handle SIGPIPE for libpq.
> Rationale: posix is broken. Per-thread signal handling is too ugly to
> think about.

I can accept that we require special code in the app to be thread-safe
_if_ they are installing their own SIGPIPE handler, but I don't think it
is fair to require them to set SIGPIPE ==> SIG_IGN to be thread-safe.

> >Again, let's get it working perfect if they say they are going to use
> >threads with libpq. Does it work OK if the app doesn't use threading?
> >
> >
> No. pthread_sigmask is part of libpthread - libpq would have to link
> unconditionally against libpthread. Or use __attribute__((weak,
> alias())), but that would only work with gcc.

libpq already links against any thread libraries if you configure
--enable-thread-safety. If you don't, we don't have to be thread-safe.
My question was whether a non-threaded app handles pthread_sigmask in a
normal way or does it only work when you are running in a thread,
pthread_create()?

> >Does sigpending/sigwait work efficiently for threads? Another idea is
> >to go with a thread-local storage boolean for each thread, and check
> >that in a signal handler we install.
> >
> I think installing a signal handler is not an option - libpq is a
> library, the signal handler is global.

OK. My suggestion was to add a libpq C function to register a SIGPIPE
handler. That way, if they don't call it, we can install our own and
handle it via SIG_IGN (if in send()), or SIG_DFL (if not in send()).

If they install their own, they have to handle ignoring SIGPIPE from
send(). They can use our code as an example.

You say you don't want to install a SIGPIPE signal handler, but we are
requiring code to make SIGPIPE => SIG_IGN to be thread-safe. That seems
like a pretty strange burden that most threaded apps will not figure out
without a lot of digging. And if you try to install a custom SIGPIPE
handler in a threaded app, libpq will not even be thread-safe because
their signal handler will be called from send() and they have no way to
determine when to ignore it (coming from send()). Whatever the
solution, I would like to have something that requires a minimal change
in application code, and works reliably in a threaded app.

On the one hand, you are saying libpq shouldn't install a signal
handler, and in another you are saying you have to set SIGPIPE to
SIG_IGN for the library to be thread-safe.

> > Seems synchronous signals like
> >SIGPIPE are delivered to the thread that invoked them, and we can check
> >thread-local storage to see if we were in a send() loop at the time of
> >signal delivery.
> >
> >
> IMHO way to fragile.

Why? We have to do something reasonable? I don't like requiring
SIGPIPE => SIG_IGN to be thread-safe.

Let's look at our four use cases:

non-threaded app, no SIGPIPE handler - works fine now
non-threaded app, custom SIGPIPE handler - works fine now
threaded app, no SIGPIPE handler - doesn't work
threaded app, custom SIGPIPE handler - doesn't work

I assume we want to get those last two working without breaking the
earlier ones. I suppose the main argument to _not_ installing our own
SIGPIPE handler is that it would require special work for non-threaded
apps that want to install their own SIGPIPE handler --- they would have
to install the handler _before_ they open a libpq connection, and they
would have to deal with checking the thread-specific send() boolean in
their signal handler to determine if they should ignore the signal.
That does sound like a mess, and is required in non-threaded apps, which
right now work fine without special checking in the custom SIGPIPE
handler.

I thought someone said that an app shouldn't ignore SIGPIPE everywhere?
What happens if an app does that? I assume using the app in a unix pipe
case would cause the process not to die when the input pipe is closed or
the output pipe closed. That seems strange.

I was thinking of using pthread_setspecific() and pthread_getspecific()
around the send() call, and have the SIGPIPE signal handler ignore the
signal if it came from the send() block --- you set/clear the value
before/after send().

Should I try to code up something everyone can look at?

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Attached is my idea for implementing safe SIGPIPE in threaded apps. The
code has the same libpq behavior if not compiled using
--enable-thread-safety.

If compiled with that option, an app wanting to define its own SIGPIPE
handler has to do so before connecting to a database. On first
connection, the code checks to see if there is a SIGPIPE handler, and if
not, installs its own, and creates a thread-local variable. Then, on
each send(), it sets, calls send(), then clears the thread-local
variable. The SIGPIPE handler checks the thread-local variable and
either ignores or exits depending on whether it was in send().

Right now the thread-local variable is static to the file, but we could
export it as a boolean so custom SIGPIPE handlers could check it and
take action or ignore the signal just like our code. Not sure if that
is a good idea or not. In fact, even cleaner, we could create a
function that allows users to define their own SIGPIPE handler and it
would be called only when not called by libpq send(), and it would work
safely for threaded apps.

I think the big problem with my approach is that it requires special
custom SIGPIPE handler code even if the app isn't multi-threaded but
libpq is compiled as multi-threaded.

Another idea is to create PQsigpipefromsend() that returns true/false
depending on whether the SIGPIPE was from libpq's send(). It could be a
global variable set/cleared in non-threaded libpq and a thread-local
variable in threaded libpq. It would allow the same API/behavior for
both libpq versions and all custom SIGPIPE handlers using libpq would
have to check it.

The one good thing about the patch is that it ignores send() SIGPIPE,
and gives default SIG_DFL behavior for libpq apps with no special app
coding, with the downside of requiring extra cost for custom SIGPIPE
handlers.

---------------------------------------------------------------------------

Manfred Spraul wrote:
> Bruce Momjian wrote:
>
> >OK, I know you had a flag for pgbench, and that doesn't use threads.
> >What speedup do you see there?
> >
> >
> Tiny. I added the flag to check that my implementation works, not as a
> benchmark tool.
>
> >I would not expect a library to require me to do something in my code to
> >be thread-safe --- either it is or it isn't.
> >
> The library is thread-safe. Just the SIGPIPE handling differs:
> - single thread: handled by libpq.
> - multi thread: caller must handle SIGPIPE for libpq.
> Rationale: posix is broken. Per-thread signal handling is too ugly to
> think about.
>
> >Again, let's get it working perfect if they say they are going to use
> >threads with libpq. Does it work OK if the app doesn't use threading?
> >
> >
> No. pthread_sigmask is part of libpthread - libpq would have to link
> unconditionally against libpthread. Or use __attribute__((weak,
> alias())), but that would only work with gcc.
>
> >Does sigpending/sigwait work efficiently for threads? Another idea is
> >to go with a thread-local storage boolean for each thread, and check
> >that in a signal handler we install.
> >
> I think installing a signal handler is not an option - libpq is a
> library, the signal handler is global.
>
> > Seems synchronous signals like
> >SIGPIPE are delivered to the thread that invoked them, and we can check
> >thread-local storage to see if we were in a send() loop at the time of
> >signal delivery.
> >
> >
> IMHO way to fragile.

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073