Re: --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.

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

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, Pennsylvania 19073

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

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, Pennsylvania 19073

-----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" <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, Pennsylvania 19073

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