Re: --enable-thread-safety broken + patch regressions

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.