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

Reply via email to