--enable-thread-safety broken + patch regressions

Lee Kindness wrote:

Bruce, the changes you made yesterday to configure for
--enable-thread-safety have broken the build, at least for Linux on
Redhat 9.

OK, how did I break things? Can you show me the failure.

Also, I took the opportunity to look at port/threads.c. It is missing
important functionality compaired to the patch I originally
submitted. For getpwuid_r, gethostbyname_r and strerror_r there are
three possible scenarios:

1. The OS doesn't have it (but the non _r function can still be thread
safe (i.e. HPUX 11)).

2. The OS has it, but the implmentation doesn't match the POSIX spec.

3. The OS has it, and the implmentation matches the POSIX spec.

Case 3 is not being considered. In my original patch this was handled
by the pqGetpwuid etc functions simply being defined to getpwuid_r
(except for pqStrerror).

I believe what we did was that there was no way to test for #3 (at the
time), so we just went with the normal function and the POSIX one, and
were going to see what happened to see if anyone needed the non-POSIX
one. Do we have any platforms that need it?

I remember discussing with you that the implementation of pqStrerror
didn't really need the distinction between the two _r
versions. However I think the others do, and the native/correct _r
calls should be #defined in if they match the POSIX spec.

It's also worth considering that when the _r function is available AND
the normal function is also thread-safe then the _r version should
still be used since it has a clean API which removes unneeded locking
within the old function.

We have that already. Have you looked in the template files. There you
control whether you should use _r functions.

Also, I doubt that the locking really has any performance hit to it.

I've still got the latest (and earlier with some configure work)
patches I submitted up at:

I just looked at this --- I have not seem them before.

Seems theading requires four things, potentially:

compile flags
link flags
link libraries
special functions

While your configure checks can detect the existance of the last one,
they don't tell us what to do if they don't exist --- are the normal
ones thread-safe.

So, the big question is whether we gain by having detection of non-posix
functions or whether it is better to just have template control it.

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

Bruce Momjian writes:

Lee Kindness wrote:

Bruce, the changes you made yesterday to configure for
--enable-thread-safety have broken the build, at least for Linux on
Redhat 9.

OK, how did I break things? Can you show me the failure.

After a:

./configure --prefix=/var/lib/pgsql/74b --enable-thread-safety

a compile of port/threads.c fails with:

gcc -O2 -Wall -Wmissing-prototypes -Wmissing-declarations -I../../src/include -c -o threads.o threads.c
threads.c: In function `pqGetpwuid':
threads.c:49: too few arguments to function `getpwuid_r'
threads.c:49: warning: assignment makes pointer from integer without a cast
threads.c: In function `pqGethostbyname':
threads.c:74: warning: passing arg 5 of `gethostbyname_r' from incompatible pointer type
threads.c:74: too few arguments to function `gethostbyname_r'
threads.c:74: warning: assignment makes pointer from integer without a cast

And this is what brought me to the issue below... The POSIX version
are getting picked up but handled like broken versions...

What info would help here? config.log?

Also, I took the opportunity to look at port/threads.c. It is missing
important functionality compaired to the patch I originally
submitted. For getpwuid_r, gethostbyname_r and strerror_r there are
three possible scenarios:

1. The OS doesn't have it (but the non _r function can still be thread
safe (i.e. HPUX 11)).

2. The OS has it, but the implmentation doesn't match the POSIX spec.

3. The OS has it, and the implmentation matches the POSIX spec.

Case 3 is not being considered. In my original patch this was handled
by the pqGetpwuid etc functions simply being defined to getpwuid_r
(except for pqStrerror).

I believe what we did was that there was no way to test for #3 (at the
time), so we just went with the normal function and the POSIX one, and
were going to see what happened to see if anyone needed the non-POSIX
one. Do we have any platforms that need it?

Well the code in thread.c will only work if the _r function is the
broken non-POSIX version.

I remember discussing with you that the implementation of pqStrerror
didn't really need the distinction between the two _r
versions. However I think the others do, and the native/correct _r
calls should be #defined in if they match the POSIX spec.

It's also worth considering that when the _r function is available AND
the normal function is also thread-safe then the _r version should
still be used since it has a clean API which removes unneeded locking
within the old function.

We have that already. Have you looked in the template files. There you
control whether you should use _r functions.

Also, I doubt that the locking really has any performance hit to
it.

As do I, but people are using this as an argument for the dumb libpq_r
library idea!

I've still got the latest (and earlier with some configure work)
patches I submitted up at:

I just looked at this --- I have not seem them before.

Everything on that page has been posted/linked to hackers and patches.

Seems theading requires four things, potentially:

compile flags
link flags
link libraries
special functions

While your configure checks can detect the existance of the last one,
they don't tell us what to do if they don't exist --- are the normal
ones thread-safe.

So, the big question is whether we gain by having detection of non-posix
functions or whether it is better to just have template control it.

We want to define & implement wrapper functions with the same API as
the POSIX versions of the _r functions we need. If we have the POSIX
versions then the replacement simply needs to be a #define to
it. Otherwise a stub function is implemented to wrap around either the
broken/old _r function or the legacy function (which may be thread
safe).

It's getting to the stage I think this isn't going to be done
correctly in time for 7.4...

L.