Thread: libpq thread safety
libpq needs additional changes for complete thread safety: - openssl needs different initialization. - kerberos is not thread safe. - functions such as gethostbyname are not thread safe, and could be used by kerberos. Right now protected with a libpq specific mutex. - dito for getpwuid and stderror. openssl is trivial: just proper flags are needed for the init function. But what about kerberos: I'm a bit reluctant to add a forth mutex: what if kerberos calls gethostbyname or getpwuid internally? Usually I would use one single_thread mutex and use that mutex for all operations - races are just too difficult to debug. Any better ideas? Otherwise I'd start searching for the non-threadsafe functions and add pthread_lock around them. Actually I'm not even sure if it should be a libpq specific mutex: what if the calling app needs to access openssl or kerberos as well? Perhaps libpq should use a system similar to openssl: http://www.openssl.org/docs/crypto/threads.html -- Manfred
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.
I think we should just state that kerberos isn't thread safe and it
isn't our problem.
For the same reason, the mutex in (eg) pqGethostbyname is an utter waste
of code space. It guarantees nothing. Furthermore, any machine that
claims to have a thread-safe libc will have either gethostbyname_r()
or a thread-safe implementation of gethostbyname(). There is no value
in our second-guessing this.
regards, tom lane
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)
Manfred Spraul <manfred@colorfullife.com> writes:
> Tom Lane wrote:
>> 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.
... and if the rest of the app doesn't all adopt the same rule, you're
still screwed. Not a big step forward.
I'd also expect that anytime someone gets their callback wrong, we will
get the bug report. I don't think that a system in which people "must"
implement their own locking primitives is desirable.
> Attached is an untested prototype patch. What do you think?
Personally I find diff -u format completely unreadable :-(. Send
"diff -c" if you want useful commentary.
regards, tom lane
Tom Lane wrote:
>Personally I find diff -u format completely unreadable :-(. Send
>"diff -c" if you want useful commentary.
>
>
diff -c is attached. I've removed the signal changes, they are
unrelated. I'll resent them separately.
--
Manfred
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 -c -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 17:29:38 -0000
***************
*** 458,463 ****
--- 458,480 ----
*/
pqbool PQinSend(void);
+ /* === 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
}
#endif
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 -c -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 17:29:38 -0000
***************
*** 448,453 ****
--- 448,460 ----
#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 -c -r1.14 thread.c
*** src/port/thread.c 29 Nov 2003 22:41:31 -0000 1.14
--- src/port/thread.c 11 Jan 2004 17:29:38 -0000
***************
*** 65,70 ****
--- 65,105 ----
* 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,96 ****
#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);
#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);
#endif
return strerrbuf;
--- 117,130 ----
#else
#if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(NEED_REENTRANT_FUNCS) && !defined(HAVE_STRERROR_R)
! 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)
! g_threadlock(false);
#endif
return strerrbuf;
***************
*** 118,125 ****
#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);
#endif
/* no getpwuid_r() available, just use getpwuid() */
--- 152,158 ----
#else
#if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(NEED_REENTRANT_FUNCS) && !defined(HAVE_GETPWUID_R)
! g_threadlock(true);
#endif
/* no getpwuid_r() available, just use getpwuid() */
***************
*** 161,167 ****
errno = ERANGE;
}
! pthread_mutex_unlock(&getpwuid_lock);
#endif
#endif
return (*result == NULL) ? -1 : 0;
--- 194,200 ----
errno = ERANGE;
}
! g_threadlock(false);
#endif
#endif
return (*result == NULL) ? -1 : 0;
***************
*** 192,199 ****
#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);
#endif
/* no gethostbyname_r(), just use gethostbyname() */
--- 225,231 ----
#else
#if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(NEED_REENTRANT_FUNCS) &&
!defined(HAVE_GETHOSTBYNAME_R)
! g_threadlock(true);
#endif
/* no gethostbyname_r(), just use gethostbyname() */
***************
*** 269,275 ****
*herrno = h_errno;
#if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(NEED_REENTRANT_FUNCS) &&
!defined(HAVE_GETHOSTBYNAME_R)
! pthread_mutex_unlock(&gethostbyname_lock);
#endif
if (*result != NULL)
--- 301,307 ----
*herrno = h_errno;
#if defined(FRONTEND) && defined(ENABLE_THREAD_SAFETY) && defined(NEED_REENTRANT_FUNCS) &&
!defined(HAVE_GETHOSTBYNAME_R)
! g_threadlock(false);
#endif
if (*result != NULL)
Manfred Spraul <manfred@colorfullife.com> writes:
> + * 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.
Wait a minute. I am *not* buying into any proposal that we need to
support ENABLE_THREAD_SAFETY on machines where libc is not thread-safe.
We have other things to do than adopt an open-ended commitment to work
around threading bugs on obsolete platforms. I don't believe that any
sane application programmer is going to try to implement a
multi-threaded app on such a platform anyway.
As I said before, I think we should rip out the useless mutex code that
is already there, not introduce a "better" solution to a non-problem.
regards, tom lane
Tom Lane wrote: >Wait a minute. I am *not* buying into any proposal that we need to >support ENABLE_THREAD_SAFETY on machines where libc is not thread-safe. >We have other things to do than adopt an open-ended commitment to work >around threading bugs on obsolete platforms. I don't believe that any >sane application programmer is going to try to implement a >multi-threaded app on such a platform anyway. > I'd agree - convince Bruce and I'll replace the mutexes in thread.c with #error. But I think libpq should support a mutex around kerberos (or at least fail at runtime) - right now it's too easy to corrupt the kerberos authentication state. -- Manfred
Manfred Spraul <manfred@colorfullife.com> writes:
> I'd agree - convince Bruce and I'll replace the mutexes in thread.c with
> #error.
Most of what I said before was aimed at Bruce ;-)
> But I think libpq should support a mutex around kerberos (or at
> least fail at runtime) - right now it's too easy to corrupt the kerberos
> authentication state.
As to the first - if an app wants to support multithreaded use of
kerberos, it will have to put a mutex around uses of kerberos. But then
it can simply extend that same mutex to uses of PQconnect. This isn't
noticeably harder from the app's point of view than what you suggest, so
I don't see the value of cluttering our API for it.
As to the second - if there were a way to detect that the app was
actually using multiple threads, I'd agree with failing at runtime
in that case. But otherwise this amounts to decreeing that you can't
compile with both --enable-thread-safety and --enable-kerberos, which
seems a tad too anal-retentive for my tastes. It seems unlikely that
there'd actually be any problem with re-entrant use of kerberos, at
least compared to common libc routines like strerror.
regards, tom lane
Manfred Spraul wrote: > Tom Lane wrote: > > >Wait a minute. I am *not* buying into any proposal that we need to > >support ENABLE_THREAD_SAFETY on machines where libc is not thread-safe. > >We have other things to do than adopt an open-ended commitment to work > >around threading bugs on obsolete platforms. I don't believe that any > >sane application programmer is going to try to implement a > >multi-threaded app on such a platform anyway. > > > I'd agree - convince Bruce and I'll replace the mutexes in thread.c with > #error. But I think libpq should support a mutex around kerberos (or at > least fail at runtime) - right now it's too easy to corrupt the kerberos > authentication state. Let me tell you where I think we are with this thread stuff, when we can discuss where to go from here. I think we are doing well with 7.4.X on threads. All platforms that have asked for threads have it working with minimal fuss, and I have gotten help on various OS/compiler combinations. We don't have any outstanding thread issues except the unreliable ignoring if send SIGPIPE, but that only happens on a backend crash, which hopefully doesn't happen too often, and we have that fixed in CVS. Now, were do we need to go? Right now we have a very course NEED_REENTRANT_FUNCS variable that says either all libc functions we call are thread-safe, or they are not. (See src/tools/thread for the test program.) However, we really have two types of function tested. The first, strerror, can be thread safe by using thread-local storage _or_ by returning pointers to static strings. The other two function tests require thread-local storage to be thread-safe. One idea I have is to add the thread test compile/link/run test into configure, to be run when you ask for threads. That way, we eliminate per-platform test reports (with the possibility that different OS versions have different thread safety characteristics), and we can throw an error if we don't find the function threadsafe or don't find the *_r version. Another idea is to change the test program to set three variables, one for each function tested, and throw an #error in the code if the function isn't thread-safe and if there is no *_r. I think in those cases we can remove the thread.c code that handles non-thread-safe libc with no *_r function. Basically, I was too coarse-grained with NEED_REENTRANT_FUNCS to throw an error if there isn't a thread-safe function because we might have platforms that have a thread-safe strerror, but not strerror_r, and *_r versions of the other functions. In that case, we would have NEED_REENTRANT_FUNCS=yes, and without strerror_r, we would fail, even though strerror itself might be thread-safe. I will start working on spliting NEED_REENTRANT_FUNCS up into three variables and we can add a configure test later if folks think that is a good idea. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Bruce Momjian wrote: >However, we really have two types of function tested. >The first, strerror, can be thread safe by using thread-local storage >_or_ by returning pointers to static strings. The other two function >tests require thread-local storage to be thread-safe. > > You are completely ignoring that libpq is a library: what if the app itself wants to call gethostbyname or stderror, too? Right now libpq has it's own private mutex. This doesn't work - the locking must be process-wide. The current implementation could be the default, and apps that want to use gethostbyname [or kerberos authentication, etc.] outside libpq must fill in appropriate callbacks. -- Manfred
Manfred Spraul wrote: > Bruce Momjian wrote: > > >However, we really have two types of function tested. > >The first, strerror, can be thread safe by using thread-local storage > >_or_ by returning pointers to static strings. The other two function > >tests require thread-local storage to be thread-safe. > > > > > You are completely ignoring that libpq is a library: what if the app > itself wants to call gethostbyname or stderror, too? > Right now libpq has it's own private mutex. This doesn't work - the > locking must be process-wide. The current implementation could be the > default, and apps that want to use gethostbyname [or kerberos > authentication, etc.] outside libpq must fill in appropriate callbacks. I never thought that far. I have applied a patch to remove the thread locking and throw an error in case a thread-safe function can not be found. I also changed the thread-safe variable to have a separate variable for each function so that *_r functions can be better selected. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
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. > I think we should just state that kerberos isn't thread safe and it > isn't our problem. > > For the same reason, the mutex in (eg) pqGethostbyname is an utter waste > of code space. It guarantees nothing. Furthermore, any machine that > claims to have a thread-safe libc will have either gethostbyname_r() > or a thread-safe implementation of gethostbyname(). There is no value > in our second-guessing this. I have implemented this in CVS. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073
Manfred Spraul wrote: > libpq needs additional changes for complete thread safety: > - openssl needs different initialization. > - kerberos is not thread safe. > - functions such as gethostbyname are not thread safe, and could be used > by kerberos. Right now protected with a libpq specific mutex. > - dito for getpwuid and stderror. > > openssl is trivial: just proper flags are needed for the init function. > But what about kerberos: I'm a bit reluctant to add a forth mutex: what > if kerberos calls gethostbyname or getpwuid internally? > Usually I would use one single_thread mutex and use that mutex for all > operations - races are just too difficult to debug. Any better ideas? > Otherwise I'd start searching for the non-threadsafe functions and add > pthread_lock around them. > Actually I'm not even sure if it should be a libpq specific mutex: what > if the calling app needs to access openssl or kerberos as well? Perhaps > libpq should use a system similar to openssl: > > http://www.openssl.org/docs/crypto/threads.html What else needs to be done/documented? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001+ If your life is a hard drive, | 13 Roberts Road + Christ can be your backup. | Newtown Square, Pennsylvania19073