Re: 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.pgbench 10 Jun 2003 09:07:15 -0000 1.9
> --- contrib/pgbench/README.pgbench 8 Nov 2003 21:43:53 -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 8 Nov 2003 21:43:54 -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,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 -0000 1.141
> --- doc/src/sgml/libpq.sgml 8 Nov 2003 21:43:56 -0000
> ***************
> *** 645,650 ****
> --- 645,693 ----
> </listitem>
> </varlistentry>
>
> + <varlistentry>
> + <term><function>PQsetsighandling</function><indexterm><primary>PQsetsighandling</></></term>
> + <term><function>PQgetsighandling</function><indexterm><primary>PQgetsighandling</></></term>
> + <listitem>
> + <para>
> + Set/query SIGPIPE signal handling.
> + <synopsis>
> + void PQsetsighandling(int internal_sigign);
> + </synopsis>
> + <synopsis>
> + int PQgetsighandling(void);
> + </synopsis>
> + </para>
> +
> + <para>
> + 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.
> + </para>
> + <para>
> + 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.
> + </para>
> + <para>
> + If internal signal handling is disabled, then the caller is
> + responsible for blocking or handling SIGPIPE signals. This is
> + recommended for multithreaded applications.
> + </para>
> + <para>
> + The signal handler setting is a global flag, it affects all
> + connections. The setting has no effect for Win32 clients - Win32
> + doesn't generate SIGPIPE events.
> + </para>
> + </listitem>
> + </varlistentry>
> +
> +
> </variablelist>
> </para>
> </sect1>
> Index: src/interfaces/libpq/blibpqdll.def
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/blibpqdll.def,v
> retrieving revision 1.9
> diff -c -r1.9 blibpqdll.def
> *** src/interfaces/libpq/blibpqdll.def 13 Aug 2003 16:29:03 -0000 1.9
> --- src/interfaces/libpq/blibpqdll.def 8 Nov 2003 21:43:57 -0000
> ***************
> *** 113,118 ****
> --- 113,120 ----
> _PQfformat @ 109
> _PQexecPrepared @ 110
> _PQsendQueryPrepared @ 111
> + _PQsetsighandling @ 112
> + _PQgetsighandling @ 113
>
> ; Aliases for MS compatible names
> PQconnectdb = _PQconnectdb
> ***************
> *** 226,228 ****
> --- 228,232 ----
> PQfformat = _PQfformat
> PQexecPrepared = _PQexecPrepared
> PQsendQueryPrepared = _PQsendQueryPrepared
> + PQsetsighandling = _PQsetsighandling
> + PQgetsighandling = _PQgetsighandling
> 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 8 Nov 2003 21:43:58 -0000
> ***************
> *** 198,203 ****
> --- 198,204 ----
> -----END DH PARAMETERS-----\n";
> #endif
>
> + static int do_sigaction = 1;
> /* ------------------------------------------------------------ */
> /* Procedures common to all secure sessions */
> /* ------------------------------------------------------------ */
> ***************
> *** 348,354 ****
> ssize_t n;
>
> #ifndef WIN32
> ! pqsigfunc oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
> #endif
>
> #ifdef USE_SSL
> --- 349,358 ----
> ssize_t n;
>
> #ifndef WIN32
> ! pqsigfunc oldsighandler = NULL;
> !
> ! if (do_sigaction)
> ! oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
> #endif
>
> #ifdef USE_SSL
> ***************
> *** 408,417 ****
> n = send(conn->sock, ptr, len, 0);
>
> #ifndef WIN32
> ! pqsignal(SIGPIPE, oldsighandler);
> #endif
>
> return n;
> }
>
> /* ------------------------------------------------------------ */
> --- 412,432 ----
> n = send(conn->sock, ptr, len, 0);
>
> #ifndef WIN32
> ! if (do_sigaction)
> ! pqsignal(SIGPIPE, oldsighandler);
> #endif
>
> return n;
> + }
> +
> + void PQsetsighandling(int internal_sigign)
> + {
> + do_sigaction = internal_sigign;
> + }
> +
> + int PQgetsighandling(void)
> + {
> + return do_sigaction;
> }
>
> /* ------------------------------------------------------------ */
> Index: src/interfaces/libpq/libpq-fe.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-fe.h,v
> retrieving revision 1.100
> diff -c -r1.100 libpq-fe.h
> *** src/interfaces/libpq/libpq-fe.h 27 Aug 2003 00:33:34 -0000 1.100
> --- src/interfaces/libpq/libpq-fe.h 8 Nov 2003 21:43:58 -0000
> ***************
> *** 221,226 ****
> --- 221,232 ----
> /* free the data structure returned by PQconndefaults() */
> extern void PQconninfoFree(PQconninfoOption *connOptions);
>
> + /* === in fe-secure.c === */
> +
> + /* get/set SIGPIPE handling */
> + extern void PQsetsighandling(int internal_sigign);
> + extern int PQgetsighandling(void);
> +
> /*
> * close the current connection and restablish a new one with the same
> * parameters

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faqs/FAQ.html

--
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