Thread: --enable-thread-safety broken + patch regressions
Bruce, the changes you made yesterday to configure for --enable-thread-safety have broken the build, at least for Linux on Redhat 9. 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 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. I've still got the latest (and earlier with some configure work) patches I submitted up at: http://services.csl.co.uk/postgresql/ Thanks, Lee. --ELM1060323025-11567-2_--
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 flagslink flagslink librariesspecial 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, Pennsylvania19073
Bruce Momjian writes:> Lee Kindness wrote:> > Bruce, the changes you made yesterday to configure for> > --enable-thread-safetyhave broken the build, at least for Linux on> > Redhat 9.> OK, how did I break things? Can you showme 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.cthreads.c: In function`pqGetpwuid':threads.c:49: too few arguments to function `getpwuid_r'threads.c:49: warning: assignment makes pointerfrom integer without a castthreads.c: In function `pqGethostbyname':threads.c:74: warning: passing arg 5 of `gethostbyname_r'from incompatible pointer typethreads.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 patchI 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 hasit, but the implmentation doesn't match the POSIX spec.> > > > 3. The OS has it, and the implmentation matches the POSIXspec.> > > > Case 3 is not being considered. In my original patch this was handled> > by the pqGetpwuid etc functionssimply being defined to getpwuid_r> > (except for pqStrerror).> > I believe what we did was that there was no wayto test for #3 (at the> time), so we just went with the normal function and the POSIX one, and> were going to see whathappened 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 thetwo _r> > versions. However I think the others do, and the native/correct _r> > calls should be #defined in if they matchthe POSIX spec.> > > > It's also worth considering that when the _r function is available AND> > the normal functionis 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 whetheryou 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> specialfunctions> > While your configure checks can detect the existance of the last one,> they don't tell us what to doif they don't exist --- are the normal> ones thread-safe.> > So, the big question is whether we gain by having detectionof 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.