Thread: --enable-thread-safety on Win32
I've been looking into fixing the --enable-thread-safety option on Windows. At the moment, we have some simple pthread emulation that may be used in libpq if --enable-thread-safety is used. The Makefile is slightly broken, however this should be easy to fix (properly) for someone more proficient with Make than I am. Thread safety cannot currently be enabled through configure on Windows for two reasons however: - If the POSIX Signals test fails, configure fails. We have our own signal code on Windows, so it's no surprise that configure fails this test. This is easily fixed with the addition of ' -a "$PORTNAME" != "win32"' to the test at line 1179 of configure.in. Why are signals needed for thread safety anyway? - The thread_test program fails to even compile on Windows. This second problem is the main issue, the main point being that our pthread emulation doesn't implement enough of the API for the test program to run, only that that's needed for libpq. To fix this we must either convert it to use Windows threads, use a full implementation of the pthread library, or implement more of the API ourselves. the first option will obviously take some effort, and probably be best implemented as a Windows specific version of the test program. The second introduces extra dependencies, at worst at runtime, at best just build time. The third is also additional, potentially significant work. However, fixing this issue using any of those methods seems somewhat pointless. All the versions of Windows that we support are thread-safe anyway (and this doesn't vary like it can on Unixes) and given that threaded apps are the standard on Windows, I don't suppose this is likely to change in future releases. It therefore seems to me that the sensible course of action is to skip the thread test altogether on Windows. Sound reasonable? Regards, Dave
Did anyone get a chance to think about this? I'd like to fix this for 8.1, but it should also make life easy with the new libpq based ODBC driver improvements if I can produce an appropriate patch sooner rather than later! Regards, Dave. > -----Original Message----- > From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Dave Page > Sent: 21 July 2005 15:00 > To: PostgreSQL-development > Subject: [HACKERS] --enable-thread-safety on Win32 > > I've been looking into fixing the --enable-thread-safety option on > Windows. At the moment, we have some simple pthread emulation that > may be used in libpq if --enable-thread-safety is used. The Makefile > is slightly broken, however this should be easy to fix (properly) > for someone more proficient with Make than I am. > > Thread safety cannot currently be enabled through configure on > Windows for two reasons however: > > - If the POSIX Signals test fails, configure fails. We have our own > signal code on Windows, so it's no surprise that configure fails > this test. This is easily fixed with the addition of > ' -a "$PORTNAME" != "win32"' to the test at line 1179 of > configure.in. Why are signals needed for thread safety anyway? > > - The thread_test program fails to even compile on Windows. > > This second problem is the main issue, the main point being that our > pthread emulation doesn't implement enough of the API for the test > program to run, only that that's needed for libpq. To fix this we > must either convert it to use Windows threads, use a full > implementation of the pthread library, or implement more of the API > ourselves. the first option will obviously take some effort, and > probably be best implemented as a Windows specific version of the > test program. The second introduces extra dependencies, at worst at > runtime, at best just build time. The third is also additional, > potentially significant work. > > However, fixing this issue using any of those methods seems somewhat > pointless. All the versions of Windows that we support are > thread-safe anyway (and this doesn't vary like it can on Unixes) > and given that threaded apps are the standard on Windows, I don't > suppose this is likely to change in future releases. It therefore > seems to me that the sensible course of action is to skip the thread > test altogether on Windows. > > Sound reasonable? > > Regards, Dave
Dave Page wrote: > Did anyone get a chance to think about this? I'd like to fix this for > 8.1, but it should also make life easy with the new libpq based ODBC > driver improvements if I can produce an appropriate patch sooner rather > than later! I have thought about it and it needs to be addressed before 8.1 final, but I don't have time at the moment. --------------------------------------------------------------------------- > > Regards, Dave. > > > -----Original Message----- > > From: pgsql-hackers-owner@postgresql.org > > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Dave Page > > Sent: 21 July 2005 15:00 > > To: PostgreSQL-development > > Subject: [HACKERS] --enable-thread-safety on Win32 > > > > I've been looking into fixing the --enable-thread-safety option on > > Windows. At the moment, we have some simple pthread emulation that > > may be used in libpq if --enable-thread-safety is used. The Makefile > > is slightly broken, however this should be easy to fix (properly) > > for someone more proficient with Make than I am. > > > > Thread safety cannot currently be enabled through configure on > > Windows for two reasons however: > > > > - If the POSIX Signals test fails, configure fails. We have our own > > signal code on Windows, so it's no surprise that configure fails > > this test. This is easily fixed with the addition of > > ' -a "$PORTNAME" != "win32"' to the test at line 1179 of > > configure.in. Why are signals needed for thread safety anyway? > > > > - The thread_test program fails to even compile on Windows. > > > > This second problem is the main issue, the main point being that our > > pthread emulation doesn't implement enough of the API for the test > > program to run, only that that's needed for libpq. To fix this we > > must either convert it to use Windows threads, use a full > > implementation of the pthread library, or implement more of the API > > ourselves. the first option will obviously take some effort, and > > probably be best implemented as a Windows specific version of the > > test program. The second introduces extra dependencies, at worst at > > runtime, at best just build time. The third is also additional, > > potentially significant work. > > > > However, fixing this issue using any of those methods seems somewhat > > pointless. All the versions of Windows that we support are > > thread-safe anyway (and this doesn't vary like it can on Unixes) > > and given that threaded apps are the standard on Windows, I don't > > suppose this is likely to change in future releases. It therefore > > seems to me that the sensible course of action is to skip the thread > > test altogether on Windows. > > > > Sound reasonable? > > > > Regards, Dave > > ---------------------------(end of broadcast)--------------------------- > TIP 3: Have you checked our extensive FAQ? > > http://www.postgresql.org/docs/faq > -- 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 July 2005 02:39 > To: Dave Page > Cc: PostgreSQL-development > Subject: Re: [HACKERS] --enable-thread-safety on Win32 > > Dave Page wrote: > > Did anyone get a chance to think about this? I'd like to > fix this for > > 8.1, but it should also make life easy with the new libpq based ODBC > > driver improvements if I can produce an appropriate patch > sooner rather > > than later! > > I have thought about it and it needs to be addressed before 8.1 final, > but I don't have time at the moment. I'm happy to work on it if we can agree on the best way forward :-) Regards, Dave
> -----Original Message----- > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > Sent: 28 July 2005 14:29 > To: Dave Page > Cc: PostgreSQL-development > Subject: Re: [HACKERS] --enable-thread-safety on Win32 > > Dave Page wrote: > > > > > > > -----Original Message----- > > > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > > > Sent: 28 July 2005 02:39 > > > To: Dave Page > > > Cc: PostgreSQL-development > > > Subject: Re: [HACKERS] --enable-thread-safety on Win32 > > > > > > Dave Page wrote: > > > > Did anyone get a chance to think about this? I'd like to > > > fix this for > > > > 8.1, but it should also make life easy with the new > libpq based ODBC > > > > driver improvements if I can produce an appropriate patch > > > sooner rather > > > > than later! > > > > > > I have thought about it and it needs to be addressed > before 8.1 final, > > > but I don't have time at the moment. > > > > I'm happy to work on it if we can agree on the best way forward :-) > > Uh, good question. As I remember the issue is that there are too many > unix-isms in the test program (thread creation) that are not actually > used in libpq. I would like to see how hard it would be to > add #ifdefs > to get the test program to run on Win32. I did manage to get it to compile (though not work fully just yet), however I had to use the full pthreads library rather than our minimal implementation. That's why I question whether we should go this way - an extra dependency (pthreads is not normally on a Windows box, even with Msys) to test an OS that we know 100% for sure is not going to vary in it's thread-safetyness(!) from system to system. Regards, Dave
Dave Page wrote: > > > > -----Original Message----- > > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > > Sent: 28 July 2005 02:39 > > To: Dave Page > > Cc: PostgreSQL-development > > Subject: Re: [HACKERS] --enable-thread-safety on Win32 > > > > Dave Page wrote: > > > Did anyone get a chance to think about this? I'd like to > > fix this for > > > 8.1, but it should also make life easy with the new libpq based ODBC > > > driver improvements if I can produce an appropriate patch > > sooner rather > > > than later! > > > > I have thought about it and it needs to be addressed before 8.1 final, > > but I don't have time at the moment. > > I'm happy to work on it if we can agree on the best way forward :-) Uh, good question. As I remember the issue is that there are too many unix-isms in the test program (thread creation) that are not actually used in libpq. I would like to see how hard it would be to add #ifdefs to get the test program to run on Win32. -- 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
"Dave Page" <dpage@vale-housing.co.uk> writes:
> From: Bruce Momjian [mailto:pgman@candle.pha.pa.us]
>> I would like to see how hard it would be to
>> add #ifdefs to get the test program to run on Win32.
> I did manage to get it to compile (though not work fully just yet),
> however I had to use the full pthreads library rather than our minimal
> implementation. That's why I question whether we should go this way - an
> extra dependency (pthreads is not normally on a Windows box, even with
> Msys) to test an OS that we know 100% for sure is not going to vary in
> it's thread-safetyness(!) from system to system.
I agree --- that seems like useless make-work. Just put a special case
for Windows into configure.in and be done with it.
regards, tom lane
Tom Lane wrote: > "Dave Page" <dpage@vale-housing.co.uk> writes: > > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > >> I would like to see how hard it would be to > >> add #ifdefs to get the test program to run on Win32. > > > I did manage to get it to compile (though not work fully just yet), > > however I had to use the full pthreads library rather than our minimal > > implementation. That's why I question whether we should go this way - an > > extra dependency (pthreads is not normally on a Windows box, even with > > Msys) to test an OS that we know 100% for sure is not going to vary in > > it's thread-safetyness(!) from system to system. > > I agree --- that seems like useless make-work. Just put a special case > for Windows into configure.in and be done with it. OK, but I would then like someone to actually run the tests we do in thread_test.c and make sure they _would_ pass on Win32. -- 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 July 2005 16:08 > To: Tom Lane > Cc: Dave Page; PostgreSQL-development > Subject: Re: [HACKERS] --enable-thread-safety on Win32 > > Tom Lane wrote: > > "Dave Page" <dpage@vale-housing.co.uk> writes: > > > From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] > > >> I would like to see how hard it would be to > > >> add #ifdefs to get the test program to run on Win32. > > > > > I did manage to get it to compile (though not work fully > just yet), > > > however I had to use the full pthreads library rather > than our minimal > > > implementation. That's why I question whether we should > go this way - an > > > extra dependency (pthreads is not normally on a Windows > box, even with > > > Msys) to test an OS that we know 100% for sure is not > going to vary in > > > it's thread-safetyness(!) from system to system. > > > > I agree --- that seems like useless make-work. Just put a > special case > > for Windows into configure.in and be done with it. > > OK, but I would then like someone to actually run the tests we do in > thread_test.c and make sure they _would_ pass on Win32. OK, I will work on this, and subsequently fixing configure etc. Regards, Dave
> -----Original Message----- > From: pgsql-hackers-owner@postgresql.org > [mailto:pgsql-hackers-owner@postgresql.org] On Behalf Of Dave Page > Sent: 28 July 2005 16:16 > To: Bruce Momjian; Tom Lane > Cc: PostgreSQL-development > Subject: Re: [HACKERS] --enable-thread-safety on Win32 > > > > OK, but I would then like someone to actually run the tests we do in > > thread_test.c and make sure they _would_ pass on Win32. > > OK, I will work on this, and subsequently fixing configure etc. OK, I have the thread test working with the fully pthreads library on Windows, and everything passes except errno (well, and getpwuid which we don't have anyway). It is supposed to be thread safe when apps are either built against libcmt.lib or msvcrt.dll (which we use), however it still seems to fail on Mingw. From what I can find, the 'usual' way to make errno thread safe is by using _beginthreadex() instead of CreateThread(), but that is done by the application of course, not libpq (in the test case, it would be done by pthreads). See http://www.microsoft.com/msj/0799/Win32/Win320799.aspx for a discussion of this if interested. However.... In all but one place in libpq, we don't use errno anyway (actually 2, but one is a bug anyway) because we use GetLastError() instead (which tested thread safe as well FWIW). The only place it's used is PQoidValue(): result = strtoul(res->cmdStatus + 7, &endptr, 10); if (!endptr || (*endptr != ' ' && *endptr != '\0') || errno == ERANGE) return InvalidOid;else return (Oid) result; We don't believe strtoul() works with GetLastError() unfortunately. One (hackish) solution would be to check that it doesn't return 0 or ULONG_MAX. Any suggestions? Aside from this issue, the hacked test app, and the changes to make all this compile are done. Regards, Dave
"Dave Page" <dpage@vale-housing.co.uk> writes:
> However.... In all but one place in libpq, we don't use errno anyway
> (actually 2, but one is a bug anyway) because we use GetLastError()
> instead (which tested thread safe as well FWIW). The only place it's
> used is PQoidValue():
> result = strtoul(res->cmdStatus + 7, &endptr, 10);
> if (!endptr || (*endptr != ' ' && *endptr != '\0') || errno ==
> ERANGE)
> return InvalidOid;
> else
> return (Oid) result;
> We don't believe strtoul() works with GetLastError() unfortunately. One
> (hackish) solution would be to check that it doesn't return 0 or
> ULONG_MAX.
I'm not sure why we bother with an overflow check there at all. It'd be
worth checking that there is a digit at cmdStatus + 7, but as long as
there is one, it's difficult to see why an overflow check is needed.
The only justification that comes to mind is that if someday there are
versions of Postgres that have 64-bit OIDs, you could get an overflow
here if you had a 32-bit-OID libpq talking to a 64-bit server. However,
I don't see a particularly good reason to return InvalidOid instead of
an overflowed value anyway in that situation. For PQoidValue,
InvalidOid is supposed to mean "there is no OID in this command status"
not "there is an OID but I cannot represent it".
regards, tom lane
Tom Lane wrote: > "Dave Page" <dpage@vale-housing.co.uk> writes: > > However.... In all but one place in libpq, we don't use errno anyway > > (actually 2, but one is a bug anyway) because we use GetLastError() > > instead (which tested thread safe as well FWIW). The only place it's > > used is PQoidValue(): > > > result = strtoul(res->cmdStatus + 7, &endptr, 10); > > > if (!endptr || (*endptr != ' ' && *endptr != '\0') || errno == > > ERANGE) > > return InvalidOid; > > else > > return (Oid) result; > > > We don't believe strtoul() works with GetLastError() unfortunately. One > > (hackish) solution would be to check that it doesn't return 0 or > > ULONG_MAX. > > I'm not sure why we bother with an overflow check there at all. It'd be > worth checking that there is a digit at cmdStatus + 7, but as long as > there is one, it's difficult to see why an overflow check is needed. > > The only justification that comes to mind is that if someday there are > versions of Postgres that have 64-bit OIDs, you could get an overflow > here if you had a 32-bit-OID libpq talking to a 64-bit server. However, > I don't see a particularly good reason to return InvalidOid instead of > an overflowed value anyway in that situation. For PQoidValue, > InvalidOid is supposed to mean "there is no OID in this command status" > not "there is an OID but I cannot represent it". I disabled the check on Win32, and added a comment explaining why. We could disable it just when we use thread-safety, but changing the behavior for threading didn't seems wise. -- 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 Index: src/interfaces/libpq/fe-exec.c =================================================================== RCS file: /cvsroot/pgsql/src/interfaces/libpq/fe-exec.c,v retrieving revision 1.170 diff -c -c -r1.170 fe-exec.c *** src/interfaces/libpq/fe-exec.c 2 Jul 2005 17:01:54 -0000 1.170 --- src/interfaces/libpq/fe-exec.c 12 Aug 2005 23:58:16 -0000 *************** *** 2166,2172 **** #endif result = strtoul(res->cmdStatus + 7, &endptr, 10); ! if (!endptr || (*endptr != ' ' && *endptr != '\0') || errno == ERANGE) return InvalidOid; else return (Oid) result; --- 2166,2180 ---- #endif result = strtoul(res->cmdStatus + 7, &endptr, 10); ! if (!endptr || (*endptr != ' ' && *endptr != '\0') ! #ifndef WIN32 ! /* ! * On WIN32, errno is not thread-safe and GetLastError() isn't set by ! * strtoul(), so we can't check on this platform. ! */ ! || errno == ERANGE ! #endif ! ) return InvalidOid; else return (Oid) result;