Re: libpq thread safety

Tom Lane wrote:

>Manfred Spraul <manfred@colorfullife.com> writes:
>
>
>>But what about kerberos: I'm a bit reluctant to add a forth mutex: what
>>if kerberos calls gethostbyname or getpwuid internally?
>>
>>
>
>Wouldn't help anyway, if some other part of the app also calls kerberos.
>
That's why I've proposed to use the system from openssl: The libpq user
must implement a lock callback, and libpq calls it around the critical
sections.
Attached is an untested prototype patch. What do you think?

--
    Manfred
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.267
diff -u -r1.267 fe-connect.c
--- src/interfaces/libpq/fe-connect.c    9 Jan 2004 02:02:43 -0000    1.267
+++ src/interfaces/libpq/fe-connect.c    11 Jan 2004 16:54:06 -0000
@@ -885,12 +885,6 @@
     struct addrinfo hint;
     const char *node = NULL;
     int            ret;
-#ifdef ENABLE_THREAD_SAFETY
-    static pthread_once_t check_sigpipe_once = PTHREAD_ONCE_INIT;
-
-    /* Check only on first connection request */
-    pthread_once(&check_sigpipe_once, check_sigpipe_handler);
-#endif

     if (!conn)
         return 0;
Index: src/interfaces/libpq/fe-secure.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-secure.c,v
retrieving revision 1.36
diff -u -r1.36 fe-secure.c
--- src/interfaces/libpq/fe-secure.c    9 Jan 2004 02:17:15 -0000    1.36
+++ src/interfaces/libpq/fe-secure.c    11 Jan 2004 16:54:07 -0000
@@ -146,11 +146,6 @@
 static SSL_CTX *SSL_context = NULL;
 #endif

-#ifdef ENABLE_THREAD_SAFETY
-static void sigpipe_handler_ignore_send(int signo);
-pthread_key_t thread_in_send;
-#endif
-
 /* ------------------------------------------------------------ */
 /*                         Hardcoded values                        */
 /* ------------------------------------------------------------ */
@@ -212,6 +207,26 @@
 /* ------------------------------------------------------------ */

 /*
+ *    Sigpipe handling.
+ *    Dummy provided even for WIN32 to keep the API consistent
+ */
+pgsigpipehandler_t default_sigpipehandler;
+
+void default_sigpipehandler(bool enable, void **state)
+{
+#ifndef WIN32
+    if (enable) {
+        *state = (void*) pqsignal(SIGPIPE, SIG_IGN);
+    } else {
+        pqsignal(SIGPIPE, (pqsigfunc)*state);
+    }
+#endif
+}
+
+static pgsigpipehandler_t *g_sigpipehandler = default_sigpipehandler;
+
+
+/*
  *    Initialize global context
  */
 int
@@ -356,12 +371,9 @@
 {
     ssize_t        n;

-#ifdef ENABLE_THREAD_SAFETY
-    pthread_setspecific(thread_in_send, "t");
-#else
 #ifndef WIN32
-    pqsigfunc    oldsighandler = pqsignal(SIGPIPE, SIG_IGN);
-#endif
+    void *sigstate;
+    g_sigpipehandler(true, &sigstate);
 #endif

 #ifdef USE_SSL
@@ -420,12 +432,8 @@
 #endif
         n = send(conn->sock, ptr, len, 0);

-#ifdef ENABLE_THREAD_SAFETY
-    pthread_setspecific(thread_in_send, "f");
-#else
 #ifndef WIN32
-    pqsignal(SIGPIPE, oldsighandler);
-#endif
+    g_sigpipehandler(false, &sigstate);
 #endif

     return n;
@@ -1066,62 +1074,18 @@

 #endif   /* USE_SSL */

-
-#ifdef ENABLE_THREAD_SAFETY
 /*
- *    Check SIGPIPE handler and perhaps install our own.
+ *    PQregisterSigpipeCallback
  */
-void
-check_sigpipe_handler(void)
+pgsigpipehandler_t *
+PQregisterSigpipeCallback(pgsigpipehandler_t *newhandler)
 {
-    pqsigfunc pipehandler;
+    pgsigpipehandler_t *prev;

-    /*
-     *    If the app hasn't set a SIGPIPE handler, define our own
-     *    that ignores SIGPIPE on libpq send() and does SIG_DFL
-     *    for other SIGPIPE cases.
-     */
-    pipehandler = pqsignalinquire(SIGPIPE);
-    if (pipehandler == SIG_DFL)    /* not set by application */
-    {
-        /*
-         *    Create key first because the signal handler might be called
-         *    right after being installed.
-         */
-        pthread_key_create(&thread_in_send, NULL);
-        pqsignal(SIGPIPE, sigpipe_handler_ignore_send);
-    }
-}
-
-/*
- *    Threaded SIGPIPE signal handler
- */
-void
-sigpipe_handler_ignore_send(int signo)
-{
-    /*
-     *    If we have gotten a SIGPIPE outside send(), exit.
-     *    Synchronous signals are delivered to the thread
-     *    that caused the signal.
-     */
-    if (!PQinSend())
-        exit(128 + SIGPIPE);    /* typical return value for SIG_DFL */
-}
-#endif
-
-/*
- *    Indicates whether the current thread is in send()
- *    For use by SIGPIPE signal handlers;  they should
- *    ignore SIGPIPE when libpq is in send().  This means
- *    that the backend has died unexpectedly.
- */
-pqbool
-PQinSend(void)
-{
-#ifdef ENABLE_THREAD_SAFETY
-    return (pthread_getspecific(thread_in_send) /* has it been set? */ &&
-            *(char *)pthread_getspecific(thread_in_send) == 't') ? true : false;
-#else
-    return false;    /* No threading, so we can't be in send() */
-#endif
+    prev = g_sigpipehandler;
+    if (newhandler)
+        g_sigpipehandler = newhandler;
+    else
+        g_sigpipehandler = default_sigpipehandler;
+    return prev;
 }
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.102
diff -u -r1.102 libpq-fe.h
--- src/interfaces/libpq/libpq-fe.h    9 Jan 2004 02:02:43 -0000    1.102
+++ src/interfaces/libpq/libpq-fe.h    11 Jan 2004 16:54:07 -0000
@@ -453,10 +453,31 @@
 /* === in fe-secure.c === */

 /*
- *    Indicates whether the libpq thread is in send().
- *    Used to ignore SIGPIPE if thread is in send().
+ *    Used to set callback that must protect libpq from SIGPIPE signals.
+ *    A default implementation is provided for single threaded apps.
+ *    Not required for WIN32.
  */
-pqbool PQinSend(void);
+typedef void (pgsigpipehandler_t)(bool enable, void **state);
+
+extern pgsigpipehandler_t *
+PQregisterSigpipeCallback(pgsigpipehandler_t *newhandler);
+
+/* === in thread.c === */
+
+/*
+ *    Used to set callback that prevents concurrent access to
+ *    non-thread safe functions that libpq needs.
+ *    The default implementation uses a libpq internal mutex.
+ *    Only required for multithreaded apps on platforms that
+ *    do not support the thread-safe equivalents and that want
+ *    to use the functions, too.
+ *    List of functions:
+ *    - stderror, getpwuid, gethostbyname.
+ *    TODO: the mutex must be used around kerberos calls, too.
+ */
+typedef void (pgthreadlock_t)(bool acquire);
+
+extern pgthreadlock_t * PQregisterThreadLock(pgthreadlock_t *newhandler);

 #ifdef __cplusplus
 }
Index: src/interfaces/libpq/libpq-int.h
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
retrieving revision 1.84
diff -u -r1.84 libpq-int.h
--- src/interfaces/libpq/libpq-int.h    9 Jan 2004 02:02:43 -0000    1.84
+++ src/interfaces/libpq/libpq-int.h    11 Jan 2004 16:54:07 -0000
@@ -446,8 +446,12 @@
 extern ssize_t pqsecure_read(PGconn *, void *ptr, size_t len);
 extern ssize_t pqsecure_write(PGconn *, const void *ptr, size_t len);
 #ifdef ENABLE_THREAD_SAFETY
-extern void check_sigpipe_handler(void);
-extern pthread_key_t thread_in_send;
+extern pgthreadlock_t *g_threadlock;
+#define pglock_thread() g_threadlock(true);
+#define pgunlock_thread() g_threadlock(false);
+#else
+#define pglock_thread() ((void)0)
+#define pgunlock_thread() ((void)0)
 #endif

 /*
Index: src/port/thread.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/port/thread.c,v
retrieving revision 1.14
diff -u -r1.14 thread.c
--- src/port/thread.c    29 Nov 2003 22:41:31 -0000    1.14
+++ src/port/thread.c    11 Jan 2004 16:54:07 -0000
@@ -65,6 +65,41 @@
  *    non-*_r functions.
  */

+#if defined(FRONTEND)
+#include "libpq-fe.h"
+#include "libpq-int.h"
+/*
+ * To keep the API consistent, the locking stubs are always provided, even
+ * if they are not required.
+ */
+pgthreadlock_t *g_threadlock;
+
+static pgthreadlock_t default_threadlock;
+static void
+default_threadlock(bool acquire)
+{
+#if defined(ENABLE_THREAD_SAFETY)
+    static pthread_mutex_t singlethread_lock = PTHREAD_MUTEX_INITIALIZER;
+    if (acquire)
+        pthread_mutex_lock(&singlethread_lock);
+    else
+        pthread_mutex_unlock(&singlethread_lock);
+#endif
+}
+
+pgthreadlock_t *
+PQregisterThreadLock(pgthreadlock_t *newhandler)
+{
+    pgthreadlock_t *prev;
+
+    prev = g_threadlock;
+    if (newhandler)
+        g_threadlock = newhandler;
+    else
+        g_threadlock = default_threadlock;
+    return prev;
+}
+#endif

 /*
  * Wrapper around strerror and strerror_r to use the former if it is
@@ -82,15 +117,14 @@
 #else

 #if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(NEED_REENTRANT_FUNCS) && !defined(HAVE_STRERROR_R)
-    static pthread_mutex_t strerror_lock = PTHREAD_MUTEX_INITIALIZER;
-    pthread_mutex_lock(&strerror_lock);
+    g_threadlock(true);
 #endif

     /* no strerror_r() available, just use strerror */
     StrNCpy(strerrbuf, strerror(errnum), buflen);

 #if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(NEED_REENTRANT_FUNCS) && !defined(HAVE_STRERROR_R)
-    pthread_mutex_unlock(&strerror_lock);
+    g_threadlock(false);
 #endif

     return strerrbuf;
@@ -118,8 +152,7 @@
 #else

 #if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(NEED_REENTRANT_FUNCS) && !defined(HAVE_GETPWUID_R)
-    static pthread_mutex_t getpwuid_lock = PTHREAD_MUTEX_INITIALIZER;
-    pthread_mutex_lock(&getpwuid_lock);
+    g_threadlock(true);
 #endif

     /* no getpwuid_r() available, just use getpwuid() */
@@ -161,7 +194,7 @@
         errno = ERANGE;
     }

-    pthread_mutex_unlock(&getpwuid_lock);
+    g_threadlock(false);
 #endif
 #endif
     return (*result == NULL) ? -1 : 0;
@@ -192,8 +225,7 @@
 #else

 #if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(NEED_REENTRANT_FUNCS) &&
!defined(HAVE_GETHOSTBYNAME_R)
-    static pthread_mutex_t gethostbyname_lock = PTHREAD_MUTEX_INITIALIZER;
-    pthread_mutex_lock(&gethostbyname_lock);
+    g_threadlock(true);
 #endif

     /* no gethostbyname_r(), just use gethostbyname() */
@@ -269,7 +301,7 @@
         *herrno = h_errno;

 #if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(NEED_REENTRANT_FUNCS) &&
!defined(HAVE_GETHOSTBYNAME_R)
-    pthread_mutex_unlock(&gethostbyname_lock);
+    g_threadlock(false);
 #endif

     if (*result != NULL)