Re: [HACKERS] libpq thread safety

[Bruce Momjian]

Tom Lane wrote:
 Manfred Spraul [EMAIL PROTECTED] 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
  [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 8: explain analyze is your friend

Re: [HACKERS] libpq thread safety

[Manfred Spraul]

Bruce Momjian wrote:

Your patch has been added to the PostgreSQL unapplied patches list at:

	http://momjian.postgresql.org/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.
 

You are too fast: the patch was a proof of concept, not really tested 
(actually quite buggy).
Attached are two patches:

- ready-sigpipe: check_sigpipe_handler skips pthread_create_key if a 
signal handler was installed. This is wrong - the key is always required.
- ready-locking: locking around kerberos and openssl.

The patches pass the regression tests on i386 linux. Kerberos is 
untested, ssl only partially tested due to the lack of a test setup.
I'm still not sure if the new code is the right thing for the openssl 
initialization: libpq calls SSL_library_init() unconditionally. If the 
calling app uses ssl, too, this might confuse openssl.

Could you replace my initial proposal with these two patches?

Btw, is it intentional that THREAD_SUPPORT is not set in src/template/linux?

--
   Manfred
Index: src/backend/libpq/md5.c
===
RCS file: /projects/cvsroot/pgsql-server/src/backend/libpq/md5.c,v
retrieving revision 1.22
diff -c -r1.22 md5.c
*** src/backend/libpq/md5.c 29 Nov 2003 19:51:49 -  1.22
--- src/backend/libpq/md5.c 14 Mar 2004 10:46:54 -
***
*** 271,277 
  static void
  bytesToHex(uint8 b[16], char *s)
  {
!   static char *hex = 0123456789abcdef;
int q,
w;
  
--- 271,277 
  static void
  bytesToHex(uint8 b[16], char *s)
  {
!   static const char *hex = 0123456789abcdef;
int q,
w;
  
Index: src/interfaces/libpq/fe-auth.c
===
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.89
diff -c -r1.89 fe-auth.c
*** src/interfaces/libpq/fe-auth.c  7 Jan 2004 18:56:29 -   1.89
--- src/interfaces/libpq/fe-auth.c  14 Mar 2004 10:46:55 -
***
*** 590,595 
--- 590,596 
  
case AUTH_REQ_KRB4:
  #ifdef KRB4
+   pglock_thread();
if (pg_krb4_sendauth(PQerrormsg, conn-sock,
   (struct sockaddr_in *)  
conn-laddr.addr,
   (struct sockaddr_in *)  
conn-raddr.addr,
***
*** 597,604 
--- 598,607 
{
snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext(Kerberos 4 authentication 
failed\n));
+   pgunlock_thread();
return STATUS_ERROR;
}
+   pgunlock_thread();
break;
  #else
snprintf(PQerrormsg, PQERRORMSG_LENGTH,
***
*** 608,620 
--- 611,626 
  
case AUTH_REQ_KRB5:
  #ifdef KRB5
+   pglock_thread();
if (pg_krb5_sendauth(PQerrormsg, conn-sock,
 hostname) != 
STATUS_OK)
{
snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext(Kerberos 5 authentication 
failed\n));
+   pgunlock_thread();
return STATUS_ERROR;
}
+   pgunlock_thread();
break;
  #else
snprintf(PQerrormsg, PQERRORMSG_LENGTH,
***
*** 722,727 
--- 728,734 
if (authsvc == 0)
return NULL;/* leave original error message in 
place */
  
+   pglock_thread();
  #ifdef KRB4
if (authsvc == STARTUP_KRB4_MSG)
name = pg_krb4_authname(PQerrormsg);
***
*** 759,763 
--- 766,771 
  
if (name  (authn = (char *) malloc(strlen(name) + 1)))
strcpy(authn, name);
+   pgunlock_thread();
return authn;
  }
Index: src/interfaces/libpq/fe-connect.c
===
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.268
diff -c -r1.268 fe-connect.c
*** src/interfaces/libpq/fe-connect.c   10 Mar 2004 21:12:47 -  1.268
--- src/interfaces/libpq/fe-connect.c   14 Mar 2004 10:46:56 -
***
*** 2902,2908 
  PQsetClientEncoding(PGconn *conn, const char *encoding)
  {
charqbuf[128];
!   static char query[] = set client_encoding to '%s';
PGresult   *res;
int status;
  
--- 2902,2908 
  

Re: [HACKERS] libpq thread safety

[Manfred Spraul]

Bruce Momjian wrote:

How can we test if libpq needs to call that?  Seems that is an issue
whether we are threaded or not, no?
 

I think it's always an issue: in the non-threaded case, it's just not 
fatal. At least some openssl init functions are protected with if 
(done) return; done = 1;, and it the worst case, it's a memory leak.
With threaded apps,  it might corrupt a concurrent ssl transaction. 
Perhaps PQenableSSLLocks could handle that case, too - a special flag 
for skip SSL_library_init().

There is a new test program in src/tools/thread that needs to be run for
every platform for 7.5.  We can't use the 7.4.X tests because it didn't
report individual function tests, just one general value.  We need
individual test reports for 7.5.  Run the test program and post the
results and I will get it updated.  The test output on my bsd/os machine
is:
 

RedHat Fedora Core 1 and Debian 3.0 both report


Make sure you have added any needed 'THREAD_CPPFLAGS' and 'THREAD_LIBS'
defines to your template/$port file before compiling this program.
Add this to your template/$port file:

STRERROR_THREADSAFE=yes
GETPWUID_THREADSAFE=no
GETHOSTBYNAME_THREADSAFE=no

The uname's are
Linux snip 2.4.25-1-686 #1 Tue Feb 24 10:55:59 EST 2004 i686 unknown 
unknown GNU/Linux
and
Linux ab 2.4.22-1.2174.nptl #1 Wed Feb 18 16:38:32 EST 2004 i686 i686 
i386 GNU/Linux

Both glibc 2.3.2, one with nptl, one with linuxthreads as the pthread 
library.

--
   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: [HACKERS] libpq thread safety

[Bruce Momjian]

Manfred Spraul wrote:
 Bruce Momjian wrote:
 
 What killed the idea of doing ssl or kerberos locking inside libpq was
 that there was no way to be sure that outside code didn't also access
 those routines.
 
 A callback based implementation can handle that: libpq has a default 
 implementation for apps that do not use openssl or kerberos themself. If 
 the app wants to use the libraries, too, then it must replace the hooks 
 with their own locks.
 
 I've attached a simple proposal, just for kerberos 4. If you agree on 
 the general approach, I'll add it to all functions that are not thread safe.
 
   I have documented that SSL and Kerberos are not
 thread-safe in the libpq docs.  Let's wait and see If we need additional
 work in this area.
   
 
 It means that multithreading is not usable: As Tom explained, the 
 connect string is often set directly by the end user. Setting sslmode 
 would result is races - impossible to support. In the very least, 
 sslmode and Kerberos would have to fail if the app is multithreaded.
 

I think it is even worse than you state.  SSL and Kerberos is mostly
controlled by pg_hba.conf, not the client connection string.

Also, while we create a thread-aware libpq, we don't actually have any
way to test if threads are being used by the application.

Let's go in this direction for Kerberos and SSL, and I can modify the
libpq docs on threading.

-- 
  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 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]

Re: [HACKERS] libpq thread safety

[Bruce Momjian]

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

---


Manfred Spraul wrote:
 Bruce Momjian wrote:
 
 What killed the idea of doing ssl or kerberos locking inside libpq was
 that there was no way to be sure that outside code didn't also access
 those routines.
 
 A callback based implementation can handle that: libpq has a default 
 implementation for apps that do not use openssl or kerberos themself. If 
 the app wants to use the libraries, too, then it must replace the hooks 
 with their own locks.
 
 I've attached a simple proposal, just for kerberos 4. If you agree on 
 the general approach, I'll add it to all functions that are not thread safe.
 
   I have documented that SSL and Kerberos are not
 thread-safe in the libpq docs.  Let's wait and see If we need additional
 work in this area.
   
 
 It means that multithreading is not usable: As Tom explained, the 
 connect string is often set directly by the end user. Setting sslmode 
 would result is races - impossible to support. In the very least, 
 sslmode and Kerberos would have to fail if the app is multithreaded.
 
 --
 Manfred

 Index: src/interfaces/libpq/fe-auth.c
 ===
 RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-auth.c,v
 retrieving revision 1.89
 diff -u -r1.89 fe-auth.c
 --- src/interfaces/libpq/fe-auth.c7 Jan 2004 18:56:29 -   1.89
 +++ src/interfaces/libpq/fe-auth.c12 Mar 2004 20:07:02 -
 @@ -590,6 +590,7 @@
  
   case AUTH_REQ_KRB4:
  #ifdef KRB4
 + pglock_thread();
   if (pg_krb4_sendauth(PQerrormsg, conn-sock,
  (struct sockaddr_in *)  
 conn-laddr.addr,
  (struct sockaddr_in *)  
 conn-raddr.addr,
 @@ -597,8 +598,10 @@
   {
   snprintf(PQerrormsg, PQERRORMSG_LENGTH,
   libpq_gettext(Kerberos 4 authentication 
 failed\n));
 + pgunlock_thread();
   return STATUS_ERROR;
   }
 + pgunlock_thread();
   break;
  #else
   snprintf(PQerrormsg, PQERRORMSG_LENGTH,
 @@ -722,6 +725,7 @@
   if (authsvc == 0)
   return NULL;/* leave original error message in 
 place */
  
 + pglock_thread();
  #ifdef KRB4
   if (authsvc == STARTUP_KRB4_MSG)
   name = pg_krb4_authname(PQerrormsg);
 @@ -759,5 +763,6 @@
  
   if (name  (authn = (char *) malloc(strlen(name) + 1)))
   strcpy(authn, name);
 + pgunlock_thread();
   return authn;
  }
 Index: src/interfaces/libpq/fe-connect.c
 ===
 RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
 retrieving revision 1.268
 diff -u -r1.268 fe-connect.c
 --- src/interfaces/libpq/fe-connect.c 10 Mar 2004 21:12:47 -  1.268
 +++ src/interfaces/libpq/fe-connect.c 12 Mar 2004 20:07:03 -
 @@ -3163,4 +3163,34 @@
  
  #undef LINELEN
  }
 +/*
 + * 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;
 +}
 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 -   1.102
 +++ src/interfaces/libpq/libpq-fe.h   12 Mar 2004 20:07:03 -
 @@ -274,6 +274,22 @@
PQnoticeProcessor proc,
void *arg);
  
 +typedef void (pgsigpipehandler_t)(bool enable, void **state);
 +
 +extern pgsigpipehandler_t *
 +PQregisterSigpipeCallback(pgsigpipehandler_t *newhandler);
 +
 +/*
 + * Used to set callback that prevents 

Re: [HACKERS] libpq thread safety

[Manfred Spraul]

Bruce Momjian wrote:

What killed the idea of doing ssl or kerberos locking inside libpq was
that there was no way to be sure that outside code didn't also access
those routines.
A callback based implementation can handle that: libpq has a default 
implementation for apps that do not use openssl or kerberos themself. If 
the app wants to use the libraries, too, then it must replace the hooks 
with their own locks.

I've attached a simple proposal, just for kerberos 4. If you agree on 
the general approach, I'll add it to all functions that are not thread safe.

 I have documented that SSL and Kerberos are not
thread-safe in the libpq docs.  Let's wait and see If we need additional
work in this area.
 

It means that multithreading is not usable: As Tom explained, the 
connect string is often set directly by the end user. Setting sslmode 
would result is races - impossible to support. In the very least, 
sslmode and Kerberos would have to fail if the app is multithreaded.

--
   Manfred
Index: src/interfaces/libpq/fe-auth.c
===
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-auth.c,v
retrieving revision 1.89
diff -u -r1.89 fe-auth.c
--- src/interfaces/libpq/fe-auth.c  7 Jan 2004 18:56:29 -   1.89
+++ src/interfaces/libpq/fe-auth.c  12 Mar 2004 20:07:02 -
@@ -590,6 +590,7 @@
 
case AUTH_REQ_KRB4:
 #ifdef KRB4
+   pglock_thread();
if (pg_krb4_sendauth(PQerrormsg, conn-sock,
   (struct sockaddr_in *)  
conn-laddr.addr,
   (struct sockaddr_in *)  
conn-raddr.addr,
@@ -597,8 +598,10 @@
{
snprintf(PQerrormsg, PQERRORMSG_LENGTH,
libpq_gettext(Kerberos 4 authentication 
failed\n));
+   pgunlock_thread();
return STATUS_ERROR;
}
+   pgunlock_thread();
break;
 #else
snprintf(PQerrormsg, PQERRORMSG_LENGTH,
@@ -722,6 +725,7 @@
if (authsvc == 0)
return NULL;/* leave original error message in 
place */
 
+   pglock_thread();
 #ifdef KRB4
if (authsvc == STARTUP_KRB4_MSG)
name = pg_krb4_authname(PQerrormsg);
@@ -759,5 +763,6 @@
 
if (name  (authn = (char *) malloc(strlen(name) + 1)))
strcpy(authn, name);
+   pgunlock_thread();
return authn;
 }
Index: src/interfaces/libpq/fe-connect.c
===
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.268
diff -u -r1.268 fe-connect.c
--- src/interfaces/libpq/fe-connect.c   10 Mar 2004 21:12:47 -  1.268
+++ src/interfaces/libpq/fe-connect.c   12 Mar 2004 20:07:03 -
@@ -3163,4 +3163,34 @@
 
 #undef LINELEN
 }
+/*
+ * 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;
+}
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 -   1.102
+++ src/interfaces/libpq/libpq-fe.h 12 Mar 2004 20:07:03 -
@@ -274,6 +274,22 @@
 PQnoticeProcessor proc,
 void *arg);
 
+typedef void (pgsigpipehandler_t)(bool enable, void **state);
+
+extern pgsigpipehandler_t *
+PQregisterSigpipeCallback(pgsigpipehandler_t *newhandler);
+
+/*
+ * 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 that use kerberos
+ * both within their app and for postgresql connections.
+ */
+typedef void (pgthreadlock_t)(bool acquire);
+
+extern pgthreadlock_t * 

Re: [HACKERS] libpq thread safety

[Bruce Momjian]

Bruce Momjian wrote:
 Manfred Spraul wrote:
  Hi,
  
  I've searched through libpq and looked for global or static variables as 
  indicators of non-threadsafe code. I found:
  - Win32 and BeOS: there is a global ioctlsocket_ret variable, but it 
  seems to be a dummy variable that is always discarded.
 
 Right, and it is moving into a compatibility function in 7.5 where it
 will be a local function variable.

Done.

 
  - pg_krb4_init(): Are the kerberos libraries thread safe? Additionally, 
  setting init_done is racy.
 
 No idea.
 
  - pg_krb4_authname(): uses a static buffer.
  - kerberos 5: Is the library thread safe? the initialization could run 
  twice, I'm not sure if that's intentional.
  - pg_krb4_authname(): relies on the global variable pg_krb5_name.
 
 Seems kerberos isn't.
 
  - PQoidStatus: uses a static buffer.
 
 Yes, known documented problem.
 
  - libpq_gettext: setting already_bound is racy.
 
 Does that happen in different threads?
 
  - openssl: According to
  http://www.openssl.org/docs/crypto/threads.html
  libpq must register locking callbacks within openssl, otherwise there 
  will be random corruptions. Additionally the SSL_context initialization 
  is not properly synchronized, and SSLerrmessage relies on a static buffer.
 
 Oh.
 
  PQoidStatus is already documented as not thread safe, but what about 
  OpenSSL and kerberos? It seems openssl needs support with callbacks, and 
  according to google searches MIT kerberos 5 is not thread safe, and 
  libpq must use mutexes to prevent concurrent calls into the kerberos 
  library.
 
 Oh, seems like a TODO here.  We already know how to do thread locking in
 port/thread.c so maybe we just need to add some locks in there.

What killed the idea of doing ssl or kerberos locking inside libpq was
that there was no way to be sure that outside code didn't also access
those routines.  I have documented that SSL and Kerberos are not
thread-safe in the libpq docs.  Let's wait and see If we need additional
work in this area.

-- 
  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: [HACKERS] libpq thread safety

[Bruce Momjian]

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
  [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: [HACKERS] libpq thread safety

[Bruce Momjian]

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
  [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: [HACKERS] libpq thread safety

[Bruce Momjian]

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
  [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: [HACKERS] libpq thread safety

[Manfred Spraul]

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
---(end of broadcast)---
TIP 8: explain analyze is your friend

[HACKERS] libpq thread safety

[Manfred Spraul]

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
---(end of broadcast)---
TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]

Re: [HACKERS] libpq thread safety

[Tom Lane]

Manfred Spraul [EMAIL PROTECTED] 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

---(end of broadcast)---
TIP 8: explain analyze is your friend

Re: [HACKERS] libpq thread safety

[Manfred Spraul]

Tom Lane wrote:

Manfred Spraul [EMAIL PROTECTED] 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 -   1.267
+++ src/interfaces/libpq/fe-connect.c   11 Jan 2004 16:54:06 -
@@ -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.c9 Jan 2004 02:17:15 -   1.36
+++ src/interfaces/libpq/fe-secure.c11 Jan 2004 16:54:07 -
@@ -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.
- */

Re: [HACKERS] libpq thread safety

[Tom Lane]

Manfred Spraul [EMAIL PROTECTED] 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

---(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: [HACKERS] libpq thread safety

[Manfred Spraul]

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 -   1.102
--- src/interfaces/libpq/libpq-fe.h 11 Jan 2004 17:29:38 -
***
*** 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.h9 Jan 2004 02:02:43 -   1.84
--- src/interfaces/libpq/libpq-int.h11 Jan 2004 17:29:38 -
***
*** 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 -  1.14
--- src/port/thread.c   11 Jan 2004 17:29:38 -
***
*** 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
  

Re: [HACKERS] libpq thread safety

[Tom Lane]

Manfred Spraul [EMAIL PROTECTED] 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

---(end of broadcast)---
TIP 6: Have you searched our list archives?

   http://archives.postgresql.org

Re: [HACKERS] libpq thread safety

[Manfred Spraul]

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
---(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: [HACKERS] libpq thread safety

[Tom Lane]

Manfred Spraul [EMAIL PROTECTED] 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

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

[HACKERS] libpq thread safety

[Manfred Spraul]

Hi,

I've searched through libpq and looked for global or static variables as 
indicators of non-threadsafe code. I found:
- Win32 and BeOS: there is a global ioctlsocket_ret variable, but it 
seems to be a dummy variable that is always discarded.
- pg_krb4_init(): Are the kerberos libraries thread safe? Additionally, 
setting init_done is racy.
- pg_krb4_authname(): uses a static buffer.
- kerberos 5: Is the library thread safe? the initialization could run 
twice, I'm not sure if that's intentional.
- pg_krb4_authname(): relies on the global variable pg_krb5_name.
- PQoidStatus: uses a static buffer.
- libpq_gettext: setting already_bound is racy.
- openssl: According to
http://www.openssl.org/docs/crypto/threads.html
libpq must register locking callbacks within openssl, otherwise there 
will be random corruptions. Additionally the SSL_context initialization 
is not properly synchronized, and SSLerrmessage relies on a static buffer.

PQoidStatus is already documented as not thread safe, but what about 
OpenSSL and kerberos? It seems openssl needs support with callbacks, and 
according to google searches MIT kerberos 5 is not thread safe, and 
libpq must use mutexes to prevent concurrent calls into the kerberos 
library.

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

Re: [HACKERS] libpq thread safety

[Bruce Momjian]

Manfred Spraul wrote:
 Hi,
 
 I've searched through libpq and looked for global or static variables as 
 indicators of non-threadsafe code. I found:
 - Win32 and BeOS: there is a global ioctlsocket_ret variable, but it 
 seems to be a dummy variable that is always discarded.

Right, and it is moving into a compatibility function in 7.5 where it
will be a local function variable.

 - pg_krb4_init(): Are the kerberos libraries thread safe? Additionally, 
 setting init_done is racy.

No idea.

 - pg_krb4_authname(): uses a static buffer.
 - kerberos 5: Is the library thread safe? the initialization could run 
 twice, I'm not sure if that's intentional.
 - pg_krb4_authname(): relies on the global variable pg_krb5_name.

Seems kerberos isn't.

 - PQoidStatus: uses a static buffer.

Yes, known documented problem.

 - libpq_gettext: setting already_bound is racy.

Does that happen in different threads?

 - openssl: According to
 http://www.openssl.org/docs/crypto/threads.html
 libpq must register locking callbacks within openssl, otherwise there 
 will be random corruptions. Additionally the SSL_context initialization 
 is not properly synchronized, and SSLerrmessage relies on a static buffer.

Oh.

 PQoidStatus is already documented as not thread safe, but what about 
 OpenSSL and kerberos? It seems openssl needs support with callbacks, and 
 according to google searches MIT kerberos 5 is not thread safe, and 
 libpq must use mutexes to prevent concurrent calls into the kerberos 
 library.

Oh, seems like a TODO here.  We already know how to do thread locking in
port/thread.c so maybe we just need to add some locks in there.

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