Re: [PATCHES] SIGPIPE handling
Manfred Spraul wrote: > Bruce Momjian wrote: > > >> > >>+ /* > >>+* We could lose a signal during this test. > >>+* In a multi-threaded application, this might > >>+* be a problem. Do any non-threaded platforms > >> > Threaded or non-threaded? > > >>+* lack sigaction()? > >>+*/ > >> > Additionally, the problem is not restricted to multithreaded apps: > signal(,SIG_IGN) clears all pending signals. OK, new function using sigblock(): pqsigfunc pqsignalinquire(int signo) { #if !defined(HAVE_POSIX_SIGNALS) pqsigfunc old_sigfunc; int old_sigmask; /* Prevent signal handler calls during test */ old_sigmask = sigblock(sigmask(signo)); old_sigfunc = signal(signo, SIG_DFL); signal(signo, old_sigfunc); sigblock(old_sigmask); return old_sigfunc; #else struct sigaction oact; if (sigaction(signo, NULL, &oact) < 0) return SIG_ERR; return oact.sa_handler; #endif /* !HAVE_POSIX_SIGNALS */ } -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] SIGPIPE handling
Manfred Spraul wrote: > Bruce Momjian wrote: > > >> > >>+ /* > >>+* We could lose a signal during this test. > >>+* In a multi-threaded application, this might > >>+* be a problem. Do any non-threaded platforms > >> > Threaded or non-threaded? OK, yea, I will use threaded. > >>+* lack sigaction()? > >>+*/ > >> > Additionally, the problem is not restricted to multithreaded apps: > signal(,SIG_IGN) clears all pending signals. Oh, yuck. Would SIG_DFL be better here? I am thinking of adding sigblock into that code on the assumption that if they have signal(), they have sigblock(). Should we disable threaded builds unless they have sigaction()? I suppose the sigblock() would take care of the pending signal problem too. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] SIGPIPE handling
Bruce Momjian wrote: + /* +* We could lose a signal during this test. +* In a multi-threaded application, this might +* be a problem. Do any non-threaded platforms Threaded or non-threaded? +* lack sigaction()? +*/ Additionally, the problem is not restricted to multithreaded apps: signal(,SIG_IGN) clears all pending signals. -- Manfred ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] SIGPIPE handling
One issue I had is in the following function. How can I easily find the current signal value without causing possible signal loss during testing, or possible abort if signals were previously ignored. I could use sigblock, and I think that would exist on a system that doesn't have sigaction, but is it worth the portability issue? Does any platform have threads and not sigaction? --- > + > + pqsigfunc > + pqsignalinquire(int signo) > + { > + #if !defined(HAVE_POSIX_SIGNALS) > + pqsigfunc old; > + > + /* > + * We could lose a signal during this test. > + * In a multi-threaded application, this might > + * be a problem. Do any non-threaded platforms > + * lack sigaction()? > + */ > + old = signal(signo, SIG_IGN); > + signal(signo, old); > + return old; > + #else > + struct sigaction oact; > + > + if (sigaction(signo, NULL, &oact) < 0) > +return SIG_ERR; > + return oact.sa_handler; > + #endif /* !HAVE_POSIX_SIGNALS */ > + } -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] SIGPIPE handling
Here is my solution to ignoring SIGPIPE in libpq's send() for threaded apps. It defines a custom SIGPIPE handler if one is not already defined by the application, and uses a thread-local variable that is checked in the SIGPIPE handler to know if the SIGPIPE was caused by a libpq send() call. The documentation is at the top of the patch. Changed from the description below is that applications can define their own SIGPIPE handler after establishing the first database connection. However, custom SIGPIPE handlers must check PQinSend() to determine if the signal should be ignored. --- pgman wrote: > > 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. -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 Index: doc/src/sgml/libpq.sgml === RCS file: /cvsroot/pgsql-server/doc/src/sgml/libpq.sgml,v retrieving revision 1.144 diff -c -c -r1.144 libpq.sgml *** doc/src/sgml/libpq.sgml 13 Dec 2003 23:59:06 - 1.144 --- doc/src/sgml/libpq.sgml 7 Jan 2004 21:42:56 - *** *** 3587,3593 One restriction is that no two threads attempt to manipulate the same PGconn object at the same time. In particular, you cannot issue concurrent commands from different threads through the same ! connection object. (If you need to run concurrent commands, start up multiple connections.) --- 3587,3593 One restriction is that no two threads attempt to manipulate the same PGconn object at the same time. In particular, you cannot issue concurrent commands from different threads through the same ! connection object. (If you need to run concurrent commands, use multiple connections.) *** *** 3611,3616 --- 3611,3635 not thread-safe.cryptthread safety It is better to use the md5 method, which is thread-safe on all platforms. + + + + libpq must ignore SIGPIPE signals + generated internally by send() calls to backend processes. + When PostgreSQL is configured without + --enable-thread-safety, libpq sets + SIGPIPE to SIG_IGN before each + send() call and restores the original signal handler after + completion. When --enable-thread-safety is used, + libpq installs its own SIGPIPE handler + before the first database connection if no custom SIGPIPE + handler has been installed previously. This handler uses thread-local + storage to determine if a SIGPIPE signal has been generated + by an internal send(). If an application wants to install + its own SIGPIPE signal handler, it should call + PQinSend() to determine if it should ignore the + SIGPIPE signal. This function is available in both + thread-safe and non-thread-safe versions of libpq. Index: src/backend/nodes/read.c === RCS file: /cvsroot/pgsql-server/src/backend/node
Re: [PATCHES] SIGPIPE handling
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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 Index: src/interfaces/libpq/fe-connect.c === RCS file: /cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v retrieving revision 1.263 diff -c -c -r1.263 fe-connect.c *** src/interfaces/libpq/fe-connect.c 18 Oct 2003 05:02:06 - 1.263 --- src/interfaces/libpq/fe-connect.c 18 Nov 2003 22:42:44 - *** *** 43,48 --- 43,52 #include #endif + #if defined(USE_THREADS) && !defined(WIN32) + #include + #endif + #include "libpq/ip.h" #include "mb/pg_wchar.h" *** *** 66,72 #define DefaultSSLMode"disable" #endif - /* -- * Definition of the conninfo parameters and their fallback resources. * --- 70,75 *** *** 198,203 --- 201,207 static char *PasswordFromFile(char *hostname, char *port, char *dbname, char *username); + /* *Connecting to a Database * *** *** 881,886 --- 885,895 struct addrinfo hint; const char *node = NULL; int ret; + #if defined(USE_THREADS) && !defined(WIN32) + static pthread_once_t check_sigpipe_once = PTHREAD_ONCE_INIT; + + pthread_once(&chec
Re: [PATCHES] SIGPIPE handling
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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073
Re: [PATCHES] SIGPIPE handling
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 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] SIGPIPE handling
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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] SIGPIPE handling
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 1thread 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 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] SIGPIPE handling
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 1thread 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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] SIGPIPE handling
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] SIGPIPE handling
Manfred Spraul <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] SIGPIPE handling
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 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 - 1.9 --- contrib/pgbench/README.pgbench 8 Nov 2003 21:43:53 - *** *** 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 - 1.27 --- contrib/pgbench/pgbench.c 8 Nov 2003 21:43:54 - *** *** 28,33 --- 28,34 #else #include #include + #include #ifdef HAVE_GETOPT_H #include *** *** 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,719 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 + PQsetsighandling(0); + break; case 'i': is_init_mode++; break; Index: doc/src/sgml/libpq.sgml === RCS file: /projects/cvsroot/pgsql-server/doc/src/sgml/libpq.sgml,v retrieving revision 1.141 diff -c -r1.141 libpq.sgml *** doc/src/sgml/libpq.sgml 1 Nov 2003 01:56:29 - 1.141 --- doc/src/sgml/libpq.sgml 8 Nov 2003 21:43:56 - *** *** 645,650 --- 645,693 + + PQsetsighandlingPQsetsighandling + PQgetsighandlingPQgetsighandling + + +Set/query SIGPIPE signal handling. + + void PQsetsighandling(int internal_sigign); + + + int PQgetsighandling(void); + + + + + These functions allow to query and set the SIGPIPE signal handling + of libpq: by default, Unix systems generate a (fatal) SIGPIPE signal + on write attempts to a disconnected socket. Most callers expect a + normal error return instead of the signal. A normal error return can + be achieved by blocking or ignoring the SIGPIPE signal. This can be + done either globally in the application or inside libpq. + + + If internal signal handling is enabled (this is the default), then + libpq sets the SIGPIPE handler to SIG_IGN before every socket send + operation and restores it afterwards. This prevents libpq from + killing the application, at the cost of a slight performance + dec
Re: [PATCHES] SIGPIPE handling
Tom Lane wrote: > Manfred Spraul <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] SIGPIPE handling
Bruce Momjian <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] SIGPIPE handling
Tom Lane wrote: > Bruce Momjian <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] SIGPIPE handling
Kurt Roeckx <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] SIGPIPE handling
Manfred Spraul <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] SIGPIPE handling
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 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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] SIGPIPE handling
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 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 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] SIGPIPE handling
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 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] SIGPIPE handling
Bruce Momjian <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] SIGPIPE handling
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 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 - 1.263 > --- src/interfaces/libpq/fe-connect.c 16 Nov 2003 11:44:47 - > *** > *** 41,46 > --- 41,48 > #include > #endif > #include > + #include > + #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 - 1.32 > --- src/interfaces/libpq/fe-secure.c 16 Nov 2003 11:44:47 - > *** > *** 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 - 1.82 > --- src/interfaces/libpq/libpq-int.h 16 Nov 2003 11:44:48 - > *
[PATCHES] 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 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 - 1.263 --- src/interfaces/libpq/fe-connect.c 16 Nov 2003 11:44:47 - *** *** 41,46 --- 41,48 #include #endif #include + #include + #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.c29 Sep 2003 16:38:04 - 1.32 --- src/interfaces/libpq/fe-secure.c16 Nov 2003 11:44:47 - *** *** 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.h5 Sep 2003 02:08:36 - 1.82 --- src/interfaces/libpq/libpq-int.h16 Nov 2003 11:44:48 - *** *** 329,334 --- 329,337 charpeer_dn[256 + 1]; /* peer distinguished name */ charpeer_cn[SM_USER + 1]; /* peer common name */ #endif + #ifndef WIN32 + booldo_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 - 1.17 --- src/interfaces/libpq/pqsignal.c 16 Nov 2003 11:44:48 - *** *** 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; +
Re: [PATCHES] SIGPIPE handling, take two.
Manfred Spraul <[EMAIL PROTECTED]> writes: > ... But the SIG_IGN/restore > sequence affects the whole app - PQconnectdb calls would result in > randomly dropped SIGPIPE signals. Good point. AFAICS we lose anyway if we don't have sigaction() available, but hopefully any multithreaded platform has sigaction(). I still don't like modifying pqsignal's API though. What I suggest is adding a function like "pqsignalinquire(signalno)" to pqsignal.c, defined to return the signal handler without changing it ... that is, take the system-dependent code you were going to put in fe-connect.c and put it in pqsignal.c instead. regards, tom lane ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send "unregister YourEmailAddressHere" to [EMAIL PROTECTED])
Re: [PATCHES] SIGPIPE handling, take two.
Tom Lane wrote: I don't think we need to complicate pqsignal's API for this. Instead we'd better document that SIGPIPE handling has to be set up and kept stable before doing any libpq operations in a multithread app. Not reliable. An app could install it's own signal handler and block SIGPIPE around all libpq calls. Signal blocking is per-thread. But the SIG_IGN/restore sequence affects the whole app - PQconnectdb calls would result in randomly dropped SIGPIPE signals. -- Manfred ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] SIGPIPE handling, take two.
Manfred Spraul <[EMAIL PROTECTED]> writes: > What about multithreaded apps? > old = pgsignal(SIPEPIPE, SIG_IGN); > ** another thread calls sigaction(SIGPIPE,,); > pgsignal(SIGPIPE, old); > And the signal state is corrupted. If other threads are changing the signal state mid-flight, we are screwed anyway; if not here then later when we are doing sends, or even more directly because our test here may not reflect reality later. I don't think we need to complicate pqsignal's API for this. Instead we'd better document that SIGPIPE handling has to be set up and kept stable before doing any libpq operations in a multithread app. regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] SIGPIPE handling, take two.
Tom Lane wrote: Bruce Momjian <[EMAIL PROTECTED]> writes: I think this is the patch I like. The #if coding is messy and unnecessary. You could do the test as per the non-POSIX variant using two calls of pqsignal(), and not have any system dependence here, nor a need for . What about multithreaded apps? old = pgsignal(SIPEPIPE, SIG_IGN); ** another thread calls sigaction(SIGPIPE,,); pgsignal(SIGPIPE, old); And the signal state is corrupted. What about extending pgsignal: pgsignal(signo, SIG_ERR); reads the current signal handler. I'll update my patch. From your other mail: No, because this patch does not have any global effect on the signal handling. It might be unnecessary to check per-connection, but it doesn't hurt, and on grounds of cleanliness I'd prefer to avoid a global variable. I agree - global state would require global synchronization. -- Manfred ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] SIGPIPE handling, take two.
Oh, forgot to mention ... Bruce Momjian <[EMAIL PROTECTED]> writes: > My only issue is that this is per-connection, while I think you have to > create a global variable that defaults to false, and on first connect, > check, and not after. No, because this patch does not have any global effect on the signal handling. It might be unnecessary to check per-connection, but it doesn't hurt, and on grounds of cleanliness I'd prefer to avoid a global variable. regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] SIGPIPE handling, take two.
Bruce Momjian <[EMAIL PROTECTED]> writes: > I think this is the patch I like. The #if coding is messy and unnecessary. You could do the test as per the non-POSIX variant using two calls of pqsignal(), and not have any system dependence here, nor a need for . regards, tom lane ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] SIGPIPE handling, take two.
Gaetano Mendola wrote: Bruce Momjian wrote: I think this is the patch I like. It does the auto-detect handling as I hoped. I will just do the doc updates to mention it. My only issue is that this is per-connection, while I think you have to create a global variable that defaults to false, and on first connect, check, and not after. Based on the code below, a second connection would have the SIGPIPE signal set to SIG_IGN, not SIG_DEF, and you would be back to setting SIG_IGN around each send, even though it was already set. Are others OK with this too? I believe that the are some errors on the following code: #if !defined(HAVE_POSIX_SIGNALS) { pqsigfunc old; old = signal(SIGPIPE, SIG_IGN); if (old != SIG_DFL) conn->do_sigaction = false; signal(SIGPIPE, old); } #else { struct sigaction oact; if (sigaction(SIGPIPE, NULL, &oact) == 0 && oact.sa_handler != SIG_DFL) conn->do_sigaction = false; } #endif /* !HAVE_POSIX_SIGNALS */ the old signal handler is not reinstated in case of HAVE_POSIX_SIGNAL Forget the message :-) Gaetano ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] SIGPIPE handling, take two.
Bruce Momjian wrote: I think this is the patch I like. It does the auto-detect handling as I hoped. I will just do the doc updates to mention it. My only issue is that this is per-connection, while I think you have to create a global variable that defaults to false, and on first connect, check, and not after. Based on the code below, a second connection would have the SIGPIPE signal set to SIG_IGN, not SIG_DEF, and you would be back to setting SIG_IGN around each send, even though it was already set. Are others OK with this too? I believe that the are some errors on the following code: #if !defined(HAVE_POSIX_SIGNALS) { pqsigfunc old; old = signal(SIGPIPE, SIG_IGN); if (old != SIG_DFL) conn->do_sigaction = false; signal(SIGPIPE, old); } #else { struct sigaction oact; if (sigaction(SIGPIPE, NULL, &oact) == 0 && oact.sa_handler != SIG_DFL) conn->do_sigaction = false; } #endif /* !HAVE_POSIX_SIGNALS */ the old signal handler is not reinstated in case of HAVE_POSIX_SIGNAL May be this sound better: #if !defined(HAVE_POSIX_SIGNALS) { pqsigfunc old; old = signal(SIGPIPE, SIG_IGN); if (old != SIG_DFL && old != SIG_ERR) conn->do_sigaction = false; if ( old != SIG_ERR ) signal(SIGPIPE, old); } #else { struct sigaction oact; int err; if ( (err = sigaction(SIGPIPE, NULL, &oact)) == 0 && oact.sa_handler != SIG_DFL) conn->do_sigaction = false; if ( err == 0 ) sigaction(SIGPIPE, &oact, NULL); } #endif /* !HAVE_POSIX_SIGNALS */ Regards Gaetano Mendola ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] SIGPIPE handling, take two.
I think this is the patch I like. It does the auto-detect handling as I hoped. I will just do the doc updates to mention it. My only issue is that this is per-connection, while I think you have to create a global variable that defaults to false, and on first connect, check, and not after. Based on the code below, a second connection would have the SIGPIPE signal set to SIG_IGN, not SIG_DEF, and you would be back to setting SIG_IGN around each send, even though it was already set. Are others OK with this too? --- Manfred Spraul wrote: > pqsecure_write tries to catch SIGPIPE signals generated by network > disconnects by setting the signal handler to SIG_IGN. The current > approach causes several problems: > - it always sets SA_RESTART when it restores the old handler. > - it's not reliable for multi threaded apps, because another thread > could change the signal handler inbetween. > - it's slow, because after setting a signal handler to SIG_IGN the > kernel must enumerate all threads and clear all pending signals (at > least FreeBSD-5.1 and linux-2.6 do that. Earlier linux kernels don't - > their signal handling is known to be broken for multithreaded apps). > > Initially I proposed a new option for PQconnectdb, but Tom didn't like > that. The attached patch autodetects if it should set the signal > handler, Tom proposed that. The code doesn't try to check if the signal > is "handled" by blocking it, because I haven't figured out how to check > that: sigprocmask is undefined for multithreaded apps and calling > pthread_sigmask would force every libpq user to link against libpthread. > > -- > Manfred > ? src/interfaces/libpq/libpq.so.3.1 > 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 - 1.263 > --- src/interfaces/libpq/fe-connect.c 2 Nov 2003 18:29:40 - > *** > *** 41,46 > --- 41,47 > #include > #endif > #include > + #include > #endif > > #include "libpq/ip.h" > *** > *** 951,956 > --- 952,983 > else if (conn->sslmode[0] == 'a') /* "allow" */ > conn->wait_ssl_try = true; > #endif > + /* > + * 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. > + */ > + conn->do_sigaction = true; > + #if !defined(HAVE_POSIX_SIGNALS) > + { > + pqsigfunc old; > + old = signal(SIGPIPE, SIG_IGN); > + if (old != SIG_DFL) > + conn->do_sigaction = false; > + signal(SIGPIPE, old); > + } > + #else > + { > + struct sigaction oact; > + > + if (sigaction(SIGPIPE, NULL, &oact) == 0 && oact.sa_handler != SIG_DFL) > + conn->do_sigaction = false; > + } > + #endif /* !HAVE_POSIX_SIGNALS */ > > /* >* Set up to try to connect, with protocol 3.0 as the first attempt. > 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 - 1.32 > --- src/interfaces/libpq/fe-secure.c 2 Nov 2003 18:29:41 - > *** > *** 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/in
Re: [PATCHES] sigpipe handling
Sorry, but this just seems overdesigned. I liked Tom's suggestion: > One possibility that comes to mind is simply to test whether the SIGPIPE > handler is already SIG_IGN before we munge it. Ideally we'd do that > once when the conn object is created, but even if it had to be done more > often, it might still be a net win. Can't we just check the SIGPIPE handler when the first connection is opened, and if it is the default, we can set it to ignore --- if not, we use our special disable code around each send call, and we document it in the libpq manual, and mention it in the release notes. If someone wants to muddle with SIGPIPE after the first connection, let them set the SIGPIPE to point to a dummy function before making the first connection --- that seems like a clear-enough API for the few folks who want to play with this. --- Manfred Spraul wrote: > Attatched is the latest version of my patch that makes the > signal(SIG_PIPE, SIG_IGN) calls around the send() syscall conditional: > they are not sufficient to ensure that multithreaded libpq users are not > killed by SIGPIPE signals, and they cause a noticable slowdown. > I've switched to a global flag, and two functions to get/set the flag. > Any other ideas how to protect libpq against SIGPIPE? > > -- > Manfred > 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.pgbench10 Jun 2003 09:07:15 - 1.9 > --- contrib/pgbench/README.pgbench8 Nov 2003 21:43:53 - > *** > *** 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 - 1.27 > --- contrib/pgbench/pgbench.c 8 Nov 2003 21:43:54 - > *** > *** 28,33 > --- 28,34 > #else > #include > #include > + #include > > #ifdef HAVE_GETOPT_H > #include > *** > *** 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,719 > 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 > + PQsetsighandling(0); > + break; > case 'i': > is_init_mode++; > break; > Index: doc/src/sgml/libpq.sgml > === > RCS file: /projects/cvsroot/pgsql-server/doc/src/sgml/libpq.sgml,v > retrieving revision 1.141 > diff -c -r1.141 libpq.sgml > *** doc/src/sgml/libpq.sgml 1 Nov 2003 01:56:29 - 1.141 > --- doc/src/sgml/libpq.sgml 8 Nov 2003 21:43:56 - > *** > *** 645,650 > --- 645,693 > > > > + > + > PQsetsighandling
[PATCHES] sigpipe handling
Attatched is the latest version of my patch that makes the signal(SIG_PIPE, SIG_IGN) calls around the send() syscall conditional: they are not sufficient to ensure that multithreaded libpq users are not killed by SIGPIPE signals, and they cause a noticable slowdown. I've switched to a global flag, and two functions to get/set the flag. Any other ideas how to protect libpq against SIGPIPE? -- Manfred 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 - 1.9 --- contrib/pgbench/README.pgbench 8 Nov 2003 21:43:53 - *** *** 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 - 1.27 --- contrib/pgbench/pgbench.c 8 Nov 2003 21:43:54 - *** *** 28,33 --- 28,34 #else #include #include + #include #ifdef HAVE_GETOPT_H #include *** *** 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,719 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 + PQsetsighandling(0); + break; case 'i': is_init_mode++; break; Index: doc/src/sgml/libpq.sgml === RCS file: /projects/cvsroot/pgsql-server/doc/src/sgml/libpq.sgml,v retrieving revision 1.141 diff -c -r1.141 libpq.sgml *** doc/src/sgml/libpq.sgml 1 Nov 2003 01:56:29 - 1.141 --- doc/src/sgml/libpq.sgml 8 Nov 2003 21:43:56 - *** *** 645,650 --- 645,693 + + PQsetsighandlingPQsetsighandling + PQgetsighandlingPQgetsighandling + + +Set/query SIGPIPE signal handling. + + void PQsetsighandling(int internal_sigign); + + + int PQgetsighandling(void); + + + + + These functions allow to query and set the SIGPIPE signal handling + of libpq: by default, Unix systems generate a (fatal) SIGPIPE signal + on write attempts to a disconnected socket. Most callers expect a + normal error return instead of the signal. A normal error return can + be achieved by blocking or ignoring the SIGPIPE signal. This can be + done either globally in the application or inside libpq. + + + If internal signal handling is enabled (this is the default), then + libpq sets the SIGPIPE handler to SIG_IGN before every socket send + operation and restores it afterwards. This prevents libpq from + killing the application, at the cost of a slight performance + decrease. This approach is not reliable for multithreaded applications. + + + If internal signal handling is disabled, then the caller is + respons
[PATCHES] SIGPIPE handling, take two.
pqsecure_write tries to catch SIGPIPE signals generated by network disconnects by setting the signal handler to SIG_IGN. The current approach causes several problems: - it always sets SA_RESTART when it restores the old handler. - it's not reliable for multi threaded apps, because another thread could change the signal handler inbetween. - it's slow, because after setting a signal handler to SIG_IGN the kernel must enumerate all threads and clear all pending signals (at least FreeBSD-5.1 and linux-2.6 do that. Earlier linux kernels don't - their signal handling is known to be broken for multithreaded apps). Initially I proposed a new option for PQconnectdb, but Tom didn't like that. The attached patch autodetects if it should set the signal handler, Tom proposed that. The code doesn't try to check if the signal is "handled" by blocking it, because I haven't figured out how to check that: sigprocmask is undefined for multithreaded apps and calling pthread_sigmask would force every libpq user to link against libpthread. -- Manfred ? src/interfaces/libpq/libpq.so.3.1 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 - 1.263 --- src/interfaces/libpq/fe-connect.c 2 Nov 2003 18:29:40 - *** *** 41,46 --- 41,47 #include #endif #include + #include #endif #include "libpq/ip.h" *** *** 951,956 --- 952,983 else if (conn->sslmode[0] == 'a') /* "allow" */ conn->wait_ssl_try = true; #endif + /* +* 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. +*/ + conn->do_sigaction = true; + #if !defined(HAVE_POSIX_SIGNALS) + { + pqsigfunc old; + old = signal(SIGPIPE, SIG_IGN); + if (old != SIG_DFL) + conn->do_sigaction = false; + signal(SIGPIPE, old); + } + #else + { + struct sigaction oact; + + if (sigaction(SIGPIPE, NULL, &oact) == 0 && oact.sa_handler != SIG_DFL) + conn->do_sigaction = false; + } + #endif /* !HAVE_POSIX_SIGNALS */ /* * Set up to try to connect, with protocol 3.0 as the first attempt. 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.c29 Sep 2003 16:38:04 - 1.32 --- src/interfaces/libpq/fe-secure.c2 Nov 2003 18:29:41 - *** *** 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.h5 Sep 2003 02:08:36 - 1.82 --- src/interfaces/libpq/libpq-int.h2 Nov 2003 18:29:42 - *** *** 329,334 --- 329,335 charpeer_dn[256 + 1]; /* peer distinguished name */ charpeer_cn[SM_USER + 1]; /* peer common name */ #endif + booldo_sigaction; /* set SIGPIPE to SIG_IGN around every send() call */ /* Buffer for current error message */ PQExpBufferData errorMessage; /* expansible string */ ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster