Re: libpq thread safety

Patch withdrawn by author.

---------------------------------------------------------------------------

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.c    7 Jan 2004 18:56:29 -0000    1.89
> +++ src/interfaces/libpq/fe-auth.c    12 Mar 2004 20:07:02 -0000
> @@ -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 -0000    1.268
> +++ src/interfaces/libpq/fe-connect.c    12 Mar 2004 20:07:03 -0000
> @@ -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 -0000    1.102
> +++ src/interfaces/libpq/libpq-fe.h    12 Mar 2004 20:07:03 -0000
> @@ -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 * PQregisterThreadLock(pgthreadlock_t *newhandler);
> +
>  /* === in fe-exec.c === */
>  
>  /* Simple synchronous query */
> Index: src/interfaces/libpq/libpq-int.h
> ===================================================================
> RCS file: /projects/cvsroot/pgsql-server/src/interfaces/libpq/libpq-int.h,v
> retrieving revision 1.85
> diff -u -r1.85 libpq-int.h
> --- src/interfaces/libpq/libpq-int.h    5 Mar 2004 01:53:59 -0000    1.85
> +++ src/interfaces/libpq/libpq-int.h    12 Mar 2004 20:07:03 -0000
> @@ -359,6 +359,16 @@
>  extern int pqPacketSend(PGconn *conn, char pack_type,
>               const void *buf, size_t buf_len);
>  
> +#ifdef ENABLE_THREAD_SAFETY
> +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
> +     
> +
>  /* === in fe-exec.c === */
>  
>  extern void pqSetResultError(PGresult *res, const char *msg);

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