Thread: Win32 Thread safetyness
Unfortunately I just found that we still cannot build in thread safety
mode on Windows, due to an error on my part - specifically, I
concentrated on libpq, not realising that ecpglib is also thread aware.
It seems that ecpglib uses far more of pthreads than libpq does, so our
mini implementation used in libpq just won't cut it. I've bitten the
bullet (well, more of a jelly bean actually) and started rewriting
things to use the official win32 pthreads library, however I ran into an
error that I'm not sure about:
make[3]: Entering directory `/cvs/pgsql/src/interfaces/libpq'
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wold-style-definition -Wendif-labels
-fno-strict-aliasing -D_REENTRANT -D_THREAD_SAFE
-D_POSIX_PTHREAD_SEMANTICS -DFRONTEND -I. -I../../../src/include
-I./src/include/port/win32 -DEXEC_BACKEND
"-I../../../src/include/port/win32" -I../../../src/port -c -o
fe-secure.o fe-secure.c
fe-secure.c: In function `pq_threadidcallback':
fe-secure.c:879: error: aggregate value used where an integer was
expected
Which relates to:
static unsigned long
pq_threadidcallback(void)
{ return (unsigned long) pthread_self();
}
In pthread.h we have:
typedef struct { void * p; /* Pointer to actual object */ unsigned int x; /* Extra
information- reuse count etc
*/
} ptw32_handle_t;
typedef ptw32_handle_t pthread_t;
PTW32_DLLPORT pthread_t PTW32_CDECL pthread_self (void);
Is it enough just to pass p back on Windows? - eg:
static unsigned long
pq_threadidcallback(void)
{
#ifdef WIN32return (unsigned long) pthread_self().p;
#elsereturn (unsigned long) pthread_self();
#endif
}
Everything builds OK with this change - I'm just not sure if it's right.
Regards, Dave
On 2005-08-24, "Dave Page" <dpage@vale-housing.co.uk> wrote:
> Which relates to:
>
> static unsigned long
> pq_threadidcallback(void)
> {
> return (unsigned long) pthread_self();
> }
This is an abuse of pthread_t - it is explicitly not guaranteed in the spec
that pthread_t is an integer type, or even a scalar type; it's permitted to
be a structure. The only valid operations on pthread_t values are assignment
and passing to functions (including pthread_equal() for equality comparison).
--
Andrew, Supernews
http://www.supernews.com - individual and corporate NNTP services
> Unfortunately I just found that we still cannot build in > thread safety mode on Windows, due to an error on my part - > specifically, I concentrated on libpq, not realising that > ecpglib is also thread aware. > > It seems that ecpglib uses far more of pthreads than libpq > does, so our mini implementation used in libpq just won't cut > it. I've bitten the bullet (well, more of a jelly bean > actually) and started rewriting things to use the official > win32 pthreads library, however I ran into an error that I'm > not sure about: Yuck. This sucks :-( I was very much hoping we could avoid an other build *and* runtime dependency. Which will be a cascading runtime dependency to each and every program that uses libpq. double-:-( Anyway, one other concern: Do we *know* how this will interact with win32 native threads? Meaning will a libpq built against the pthreads library be safe for *native threads*. Because you can definitly expect most of the win32 apps that need thread-safeness to be usign native threads - I've so far not come across a single program that's native win32 that uses pthreads, whereas almost every program written uses native threads (though most often not in a way that would need a threadsafe libpq) //Magnus
"Magnus Hagander" <mha@sollentuna.net> writes:
> Yuck. This sucks :-( I was very much hoping we could avoid an other
> build *and* runtime dependency. Which will be a cascading runtime
> dependency to each and every program that uses libpq. double-:-(
That seems like a clear nonstarter :-(
Can we confine the damage to stuff that uses ecpg, rather than adding a
dependency to everything that uses libpq?
regards, tom lane
> -----Original Message----- > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] > Sent: 25 August 2005 16:35 > To: Magnus Hagander > Cc: Dave Page; pgsql-hackers@postgresql.org > Subject: Re: [HACKERS] Win32 Thread safetyness > > "Magnus Hagander" <mha@sollentuna.net> writes: > > Yuck. This sucks :-( I was very much hoping we could avoid an other > > build *and* runtime dependency. Which will be a cascading runtime > > dependency to each and every program that uses libpq. double-:-( > > That seems like a clear nonstarter :-( > > Can we confine the damage to stuff that uses ecpg, rather > than adding a > dependency to everything that uses libpq? Yeah, that could be done with a little makefile hacking - we can use our emulation for libpq and normal pthreads for ecpglib. However, I'm still unsure how to handle the problem I posted about. From what Andrew Supernews has posted about pthread_t not being guaranteed to be an int, the appropriate fix would be to stop casting it to long. I haven't looked at the implications of that yet though - any thoughts before I do? Regards, Dave.
I poked around this one. The SSL locking documentation is at: http://www.die.net/doc/linux/man/man3/crypto_set_id_callback.3.html The description isn't clear. When they talk about locking_function() and id_function(void), they are talking about arguments to the SSL functions listed above, not actual SSL functions. The basic issue with SSL is that it wants some unique identifier for threads. They really should have defined the function to take pthread_t rather than unsigned long because pthreads doesn't really give us a useful way to get an unsigned long value, but we have no choice, so we have to do it. (I have added a comment to libpq code mentioning the reason for the cast.) An additional item in the manual is: id_function(void) is a function that returns a thread ID. It is notneeded on Windows nor on platforms where getpid() returnsa different IDfor each thread (most notably Linux). So, we might be able to get away without this on Win32, or perhaps the pthread_self().p is a valid unique identifier for Win32 threads and pthreads. How is this pthread_self() call working on Win32 now? Is pthreads a requirement for libpq threading on Win32? I didn't think it was. As far as ecpg is concerned, I think the right plan is to use Win32 threads for libpq, but to use pthreads in ecpg. However, will Win32 threads still work in ecpg if we use pthreads to do the locking? Maybe we have to force ecpg users to use pthreads on Win32. Also, there doesn't seem to be a good way for users to know if libpq or ecpg was compiled to be thread-safe. --------------------------------------------------------------------------- Dave Page wrote: > Unfortunately I just found that we still cannot build in thread safety > mode on Windows, due to an error on my part - specifically, I > concentrated on libpq, not realising that ecpglib is also thread aware. > > It seems that ecpglib uses far more of pthreads than libpq does, so our > mini implementation used in libpq just won't cut it. I've bitten the > bullet (well, more of a jelly bean actually) and started rewriting > things to use the official win32 pthreads library, however I ran into an > error that I'm not sure about: > > make[3]: Entering directory `/cvs/pgsql/src/interfaces/libpq' > gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith > -Wdeclaration-after-statement -Wold-style-definition -Wendif-labels > -fno-strict-aliasing -D_REENTRANT -D_THREAD_SAFE > -D_POSIX_PTHREAD_SEMANTICS -DFRONTEND -I. -I../../../src/include > -I./src/include/port/win32 -DEXEC_BACKEND > "-I../../../src/include/port/win32" -I../../../src/port -c -o > fe-secure.o fe-secure.c > fe-secure.c: In function `pq_threadidcallback': > fe-secure.c:879: error: aggregate value used where an integer was > expected > > Which relates to: > > static unsigned long > pq_threadidcallback(void) > { > return (unsigned long) pthread_self(); > } > > In pthread.h we have: > > typedef struct { > void * p; /* Pointer to actual object */ > unsigned int x; /* Extra information - reuse count etc > */ > } ptw32_handle_t; > > typedef ptw32_handle_t pthread_t; > > PTW32_DLLPORT pthread_t PTW32_CDECL pthread_self (void); > > Is it enough just to pass p back on Windows? - eg: > > static unsigned long > pq_threadidcallback(void) > { > #ifdef WIN32 > return (unsigned long) pthread_self().p; > #else > return (unsigned long) pthread_self(); > #endif > } > > Everything builds OK with this change - I'm just not sure if it's right. > > Regards, Dave > > ---------------------------(end of broadcast)--------------------------- > TIP 6: explain analyze is your friend > -- 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
> The basic issue with SSL is that it wants some unique > identifier for threads. They really should have defined the > function to take pthread_t rather than unsigned long because > pthreads doesn't really give us a useful way to get an They absolutely should not have done that. That would've locked them to pthreads and not other type of thread implementation. > How is this pthread_self() call working on Win32 now? Is > pthreads a requirement for libpq threading on Win32? I > didn't think it was. The version that's in CVS now uses our own pthreads wrapper, so it's definitly not a requirement: http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/pt hread-win32.c?rev=1.8 Note that we are not safe per these requirements - pthread_self() returns GetCurrentThread(), which in turn returns a *pseudo handle that has the same value in every thread*. We need to be using GetCurrentThreadId() (it's a DWORD GetCurrentThreadId(void), so it should be a simple replacement in the file). Please make that change (I don't think you need a patch for this, right?) regardless, that's a clear bug in our current implementation. Should be backpatched as well. (No I haven't tested it, so pleas emake sure it compiles :P) If you need a patch for it let me know. > As far as ecpg is concerned, I think the right plan is to use > Win32 threads for libpq, but to use pthreads in ecpg. > However, will Win32 threads still work in ecpg if we use > pthreads to do the locking? Maybe we have to force ecpg > users to use pthreads on Win32. If it works, that's definitly the best. Forcing pthreads on every user will be almost equal to saying we're not thread-safe on win32. While it would still be bad for ecpg users, there aren't as many of those as there are libpq users :-) > Also, there doesn't seem to be a good way for users to know > if libpq or ecpg was compiled to be thread-safe. Right. A runtime function for this might be a good thing? Like "bool PQisThreadSafe()" or such? //Magnus
Magnus Hagander wrote: > > The basic issue with SSL is that it wants some unique > > identifier for threads. They really should have defined the > > function to take pthread_t rather than unsigned long because > > pthreads doesn't really give us a useful way to get an > > They absolutely should not have done that. That would've locked them to > pthreads and not other type of thread implementation. True. > > How is this pthread_self() call working on Win32 now? Is > > pthreads a requirement for libpq threading on Win32? I > > didn't think it was. > > The version that's in CVS now uses our own pthreads wrapper, so it's > definitly not a requirement: > http://developer.postgresql.org/cvsweb.cgi/pgsql/src/interfaces/libpq/pt > hread-win32.c?rev=1.8 > Note that we are not safe per these requirements - pthread_self() > returns GetCurrentThread(), which in turn returns a *pseudo handle that > has the same value in every thread*. Ewe. > We need to be using GetCurrentThreadId() (it's a DWORD > GetCurrentThreadId(void), so it should be a simple replacement in the > file). Please make that change (I don't think you need a patch for this, > right?) regardless, that's a clear bug in our current implementation. > Should be backpatched as well. (No I haven't tested it, so pleas emake > sure it compiles :P) If you need a patch for it let me know. Change made. Thanks. I didn't check the compile but I am sure someone will and report back a failure. :-) > > As far as ecpg is concerned, I think the right plan is to use > > Win32 threads for libpq, but to use pthreads in ecpg. > > However, will Win32 threads still work in ecpg if we use > > pthreads to do the locking? Maybe we have to force ecpg > > users to use pthreads on Win32. > > If it works, that's definitly the best. Forcing pthreads on every user > will be almost equal to saying we're not thread-safe on win32. While it > would still be bad for ecpg users, there aren't as many of those as > there are libpq users :-) > > > > Also, there doesn't seem to be a good way for users to know > > if libpq or ecpg was compiled to be thread-safe. > > Right. A runtime function for this might be a good thing? Like "bool > PQisThreadSafe()" or such? Yes, and a flag to ecpg. Added to TODO: * Add function to return the thread safety status of libpq and ecpg -- 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
> -----Original Message----- > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > Sent: 28 August 2005 18:19 > To: Dave Page > Cc: pgsql-hackers@postgresql.org > Subject: Re: [HACKERS] Win32 Thread safetyness > > > I poked around this one. The SSL locking documentation is at: > > http://www.die.net/doc/linux/man/man3/crypto_set_id_callback.3.html > > The description isn't clear. When they talk about locking_function() > and id_function(void), they are talking about arguments to the SSL > functions listed above, not actual SSL functions. > > The basic issue with SSL is that it wants some unique identifier for > threads. They really should have defined the function to > take pthread_t > rather than unsigned long because pthreads doesn't really give us a > useful way to get an unsigned long value, but we have no choice, so we > have to do it. (I have added a comment to libpq code mentioning the > reason for the cast.) An additional item in the manual is: > > id_function(void) is a function that returns a thread > ID. It is not > needed on Windows nor on platforms where getpid() > returns a different ID > for each thread (most notably Linux). > > So, we might be able to get away without this on Win32, or perhaps the > pthread_self().p is a valid unique identifier for Win32 threads and > pthreads. Right. > How is this pthread_self() call working on Win32 now? Is pthreads a > requirement for libpq threading on Win32? I didn't think it was. Libpq can use our existing minimal implementation of pthreads (this is how it currently works in CVS). The thread test program needs the full pthreads library. > As far as ecpg is concerned, I think the right plan is to use Win32 > threads for libpq, but to use pthreads in ecpg. However, will Win32 > threads still work in ecpg if we use pthreads to do the > locking? Maybe > we have to force ecpg users to use pthreads on Win32. Currently I have it building using the full pthreads library, with static unsigned long pq_threadidcallback(void) { #ifdef WIN32 return (unsigned long) pthread_self().p; #else return (unsigned long) pthread_self(); #endif } I don't know how badly that might be screwed up though - I simply don't know enough about pthreads (or win32 threads for that matter - I only really use wxWidgets threads or C# threads). The ideal way would be to port it to win32 threads of course, but there's no way I'll have time to do that at the moment with pgAdmin and pgInstaller both needing my attention :-(. Anyone else got some time? > Also, there doesn't seem to be a good way for users to know > if libpq or > ecpg was compiled to be thread-safe. No. Mind you, that ties in nicely with a comment that Magnus made to me the other day - we could really use a function to get libpq's version number. Something similar (for 8.2 of course) could tell us more about it, such as thread safetyness. Anyhoo, I can patch ecpglib to use the full pthreads with my hack above if people think that is the way to go for the time being. Regards, Dave.
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Magnus Hagander wrote:
>>> Also, there doesn't seem to be a good way for users to know
>>> if libpq or ecpg was compiled to be thread-safe.
>>
>> Right. A runtime function for this might be a good thing? Like "bool
>> PQisThreadSafe()" or such?
> Yes, and a flag to ecpg. Added to TODO:
Um, it's not clear *when* you need to know this:- application configure time?- application compile time?- application
linktime?- application run time?
Of those possibilities, "add a function" responds to only one, and it's
the one I can see least use-case for. I should think that by run-time
it's probably too late to do much about it other than fail.
You can find out whether thread-safety was mentioned in our configure
settings by looking at the result of pg_config --configure. This might
be enough for the find-out-at-configure-time case.
regards, tom lane
Dave Page wrote:
> > So, we might be able to get away without this on Win32, or perhaps the
> > pthread_self().p is a valid unique identifier for Win32 threads and
> > pthreads.
>
> Right.
>
> > How is this pthread_self() call working on Win32 now? Is pthreads a
> > requirement for libpq threading on Win32? I didn't think it was.
>
> Libpq can use our existing minimal implementation of pthreads (this is
> how it currently works in CVS). The thread test program needs the full
> pthreads library.
Well, actually I tried to run configure --enable-thread-safety on a
MinGW platform and got:
configure: error: pthread.h not found, required for --enable-thread-safetyYou need to run the 'configure' program
first.See the file'INSTALL' for installation instructions.make: *** [all] Error 1
Can we not compile --enable-thread-safety without pthreads on Win32? I
am thinking that is true because we are going to need pthreads for ecpg.
> > As far as ecpg is concerned, I think the right plan is to use Win32
> > threads for libpq, but to use pthreads in ecpg. However, will Win32
> > threads still work in ecpg if we use pthreads to do the
> > locking? Maybe
> > we have to force ecpg users to use pthreads on Win32.
>
> Currently I have it building using the full pthreads library, with
>
> static unsigned long
> pq_threadidcallback(void)
> {
> #ifdef WIN32
> return (unsigned long) pthread_self().p;
> #else
> return (unsigned long) pthread_self();
> #endif
> }
>
> I don't know how badly that might be screwed up though - I simply don't
> know enough about pthreads (or win32 threads for that matter - I only
> really use wxWidgets threads or C# threads).
Seems Mangus gave us the proper Win32 function call and I have changed
pthread_self() on Win32 to return DWORD.
> The ideal way would be to port it to win32 threads of course, but
> there's no way I'll have time to do that at the moment with pgAdmin and
> pgInstaller both needing my attention :-(. Anyone else got some time?
>
> > Also, there doesn't seem to be a good way for users to know
> > if libpq or
> > ecpg was compiled to be thread-safe.
>
> No. Mind you, that ties in nicely with a comment that Magnus made to me
> the other day - we could really use a function to get libpq's version
> number. Something similar (for 8.2 of course) could tell us more about
> it, such as thread safetyness.
True.
> Anyhoo, I can patch ecpglib to use the full pthreads with my hack above
> if people think that is the way to go for the time being.
OK.
-- 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: > Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Magnus Hagander wrote: > >>> Also, there doesn't seem to be a good way for users to know > >>> if libpq or ecpg was compiled to be thread-safe. > >> > >> Right. A runtime function for this might be a good thing? Like "bool > >> PQisThreadSafe()" or such? > > > Yes, and a flag to ecpg. Added to TODO: > > Um, it's not clear *when* you need to know this: > - application configure time? > - application compile time? > - application link time? > - application run time? > > Of those possibilities, "add a function" responds to only one, and it's > the one I can see least use-case for. I should think that by run-time > it's probably too late to do much about it other than fail. Yea, I am thinking a libpq function would say "libpq not thread-safe' and return an error. I would think pg_config output would be fine for any link or compile-time check. > You can find out whether thread-safety was mentioned in our configure > settings by looking at the result of pg_config --configure. This might > be enough for the find-out-at-configure-time case. True. -- 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
> -----Original Message----- > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > Sent: 28 August 2005 22:56 > To: Dave Page > Cc: pgsql-hackers@postgresql.org > Subject: Re: [HACKERS] Win32 Thread safetyness > > > Well, actually I tried to run configure --enable-thread-safety on a > MinGW platform and got: > > configure: error: pthread.h not found, required for > --enable-thread-safety > You need to run the 'configure' program first. See the file > 'INSTALL' for installation instructions. > make: *** [all] Error 1 > > Can we not compile --enable-thread-safety without pthreads on > Win32? I > am thinking that is true because we are going to need > pthreads for ecpg. Correct - it is needed for ecpg, but not libpq. > > Anyhoo, I can patch ecpglib to use the full pthreads with > my hack above > > if people think that is the way to go for the time being. > > OK. > Attached (without the hack). Libpq will use it's minimal pthread implementation, ecpg will use pthreadGC2. You'll need to run autoconf after applying. Regards, Dave.
Patch applied. Autoconf run. Thanks. --------------------------------------------------------------------------- Dave Page wrote: > > > > -----Original Message----- > > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > > Sent: 28 August 2005 22:56 > > To: Dave Page > > Cc: pgsql-hackers@postgresql.org > > Subject: Re: [HACKERS] Win32 Thread safetyness > > > > > > Well, actually I tried to run configure --enable-thread-safety on a > > MinGW platform and got: > > > > configure: error: pthread.h not found, required for > > --enable-thread-safety > > You need to run the 'configure' program first. See the file > > 'INSTALL' for installation instructions. > > make: *** [all] Error 1 > > > > Can we not compile --enable-thread-safety without pthreads on > > Win32? I > > am thinking that is true because we are going to need > > pthreads for ecpg. > > Correct - it is needed for ecpg, but not libpq. > > > > Anyhoo, I can patch ecpglib to use the full pthreads with > > my hack above > > > if people think that is the way to go for the time being. > > > > OK. > > > > Attached (without the hack). Libpq will use it's minimal pthread > implementation, ecpg will use pthreadGC2. > > You'll need to run autoconf after applying. > > Regards, Dave. Content-Description: win32-threads.diff [ Attachment, skipping... ] > > ---------------------------(end of broadcast)--------------------------- > TIP 2: Don't 'kill -9' the postmaster -- 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, Pennsylvania 19073
> > > Yes, and a flag to ecpg. Added to TODO: > > > > Um, it's not clear *when* you need to know this: > > - application configure time? > > - application compile time? > > - application link time? > > - application run time? > > > > Of those possibilities, "add a function" responds to only one, and > > it's the one I can see least use-case for. I should think that by > > run-time it's probably too late to do much about it other than fail. > > Yea, I am thinking a libpq function would say "libpq not thread-safe' > and return an error. I would think pg_config output would be > fine for any link or compile-time check. Another idea might be to compile the library with a different name when it's thread safe. And if your system supports it, we build both by default. (libpq and libpqt or something). //Magnus