Thread: thread safety tests
I am wondering why thread_test.c is checking for mktemp()? That function is nowhere used in the libpq. Also, I would suggest that we allow to build the libpq with thread specific compiler options, even if it is not entirely thread safe. It would require that a really multithreaded application has to have mutexes around certain operations, but being entirely unable to configure in a way that adds thread compile options to the CFLAGS makes libpq completely useless for multithreaded programs on some platforms (for example Solaris). Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck wrote: > I am wondering why thread_test.c is checking for mktemp()? That function > is nowhere used in the libpq. Uh, it isn't checking for mktemp, it is using it, and it is using it because someone didn't like hard-coded paths I was using in the past. Is there something wrong with using mktemp? I have heard of no portability problems, except some need six X's, and we updated that. > Also, I would suggest that we allow to build the libpq with thread > specific compiler options, even if it is not entirely thread safe. It > would require that a really multithreaded application has to have > mutexes around certain operations, but being entirely unable to > configure in a way that adds thread compile options to the CFLAGS makes > libpq completely useless for multithreaded programs on some platforms > (for example Solaris). I am confused what you are suggesting. Are you saying to use thread flags but not the other things that make is thread-safe? There isn't much else other than the flags actually. Now that more OS's are thread-safe by default, we could consider using threading if it is available by default. We would need some way of reporting that to the user, perhaps via an installed readme file or a pg_config output option. -- 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
Jan Wieck wrote: > On 6/9/2004 9:36 AM, Bruce Momjian wrote: > > > Jan Wieck wrote: > >> I am wondering why thread_test.c is checking for mktemp()? That function > >> is nowhere used in the libpq. > > > > Uh, it isn't checking for mktemp, it is using it, and it is using it > > because someone didn't like hard-coded paths I was using in the past. > > Is there something wrong with using mktemp? I have heard of no > > portability problems, except some need six X's, and we updated that. > > There seems to be a portability issue here. Stefan Kaltenbrunner > reported a configure failure on sparc64-unknown-openbsd3.5 and the > config.log says: > > /tmp//ccx22029.o: In function `main': > /tmp//ccx22029.o(.text+0x8c): warning: mktemp() possibly used unsafely; > consider > using mkstemp() Yes, I was wondering how mktemp was going to guard against concurrent access. I have applied the following patch to use mkstemp(). > Which is only a warning at this time, it fails later on getpwuid(). Oh, I will need to hear more about that failure. -- 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/tools/thread/thread_test.c =================================================================== RCS file: /cvsroot/pgsql-server/src/tools/thread/thread_test.c,v retrieving revision 1.30 diff -c -c -r1.30 thread_test.c *** src/tools/thread/thread_test.c 28 May 2004 18:37:10 -0000 1.30 --- src/tools/thread/thread_test.c 9 Jun 2004 15:03:29 -0000 *************** *** 104,110 **** { pthread_t thread1, thread2; ! if (argc > 1) { fprintf(stderr, "Usage: %s\n", argv[0]); --- 104,111 ---- { pthread_t thread1, thread2; ! int fd; ! if (argc > 1) { fprintf(stderr, "Usage: %s\n", argv[0]); *************** *** 120,130 **** /* Make temp filenames, might not have strdup() */ temp_filename_1 = malloc(strlen(TEMP_FILENAME_1) + 1); strcpy(temp_filename_1, TEMP_FILENAME_1); ! mktemp(temp_filename_1); temp_filename_2 = malloc(strlen(TEMP_FILENAME_2) + 1); strcpy(temp_filename_2, TEMP_FILENAME_2); ! mktemp(temp_filename_2); #if !defined(HAVE_GETADDRINFO) && !defined(HAVE_GETHOSTBYNAME_R) if (gethostname(myhostname, MAXHOSTNAMELEN) != 0) --- 121,133 ---- /* Make temp filenames, might not have strdup() */ temp_filename_1 = malloc(strlen(TEMP_FILENAME_1) + 1); strcpy(temp_filename_1, TEMP_FILENAME_1); ! fd = mkstemp(temp_filename_1); ! close(fd); temp_filename_2 = malloc(strlen(TEMP_FILENAME_2) + 1); strcpy(temp_filename_2, TEMP_FILENAME_2); ! fd = mkstemp(temp_filename_2); ! close(fd); #if !defined(HAVE_GETADDRINFO) && !defined(HAVE_GETHOSTBYNAME_R) if (gethostname(myhostname, MAXHOSTNAMELEN) != 0)
On 6/9/2004 9:36 AM, Bruce Momjian wrote:
>
>> Also, I would suggest that we allow to build the libpq with thread
>> specific compiler options, even if it is not entirely thread safe. It
>> would require that a really multithreaded application has to have
>> mutexes around certain operations, but being entirely unable to
>> configure in a way that adds thread compile options to the CFLAGS makes
>> libpq completely useless for multithreaded programs on some platforms
>> (for example Solaris).
>
> I am confused what you are suggesting. Are you saying to use thread
> flags but not the other things that make is thread-safe? There isn't
> much else other than the flags actually. Now that more OS's are
> thread-safe by default, we could consider using threading if it is
> available by default. We would need some way of reporting that to the
> user, perhaps via an installed readme file or a pg_config output option.
The problem is that if your thread-safety tests fail, there is no way to
build libpq with -pthread and -DREENTRANT or whatever is required on
that platform. On Solaris this results in errno being defined as
extern int errno;
as supposed to
#define errno *(errno())
which makes libpq on Solaris completely useless for every program that
uses threads for "something". There is still value in compiling it with
thread support compiler flags, even if it will not result in a thread
safe libpq.
Jan
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #
On 6/9/2004 9:36 AM, Bruce Momjian wrote: > Jan Wieck wrote: >> I am wondering why thread_test.c is checking for mktemp()? That function >> is nowhere used in the libpq. > > Uh, it isn't checking for mktemp, it is using it, and it is using it > because someone didn't like hard-coded paths I was using in the past. > Is there something wrong with using mktemp? I have heard of no > portability problems, except some need six X's, and we updated that. There seems to be a portability issue here. Stefan Kaltenbrunner reported a configure failure on sparc64-unknown-openbsd3.5 and the config.log says: /tmp//ccx22029.o: In function `main': /tmp//ccx22029.o(.text+0x8c): warning: mktemp() possibly used unsafely; consider using mkstemp() Which is only a warning at this time, it fails later on getpwuid(). Jan > >> Also, I would suggest that we allow to build the libpq with thread >> specific compiler options, even if it is not entirely thread safe. It >> would require that a really multithreaded application has to have >> mutexes around certain operations, but being entirely unable to >> configure in a way that adds thread compile options to the CFLAGS makes >> libpq completely useless for multithreaded programs on some platforms >> (for example Solaris). > > I am confused what you are suggesting. Are you saying to use thread > flags but not the other things that make is thread-safe? There isn't > much else other than the flags actually. Now that more OS's are > thread-safe by default, we could consider using threading if it is > available by default. We would need some way of reporting that to the > user, perhaps via an installed readme file or a pg_config output option. > -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
On 6/9/2004 11:45 AM, Bruce Momjian wrote: > Jan Wieck wrote: >> The problem is that if your thread-safety tests fail, there is no way to >> build libpq with -pthread and -DREENTRANT or whatever is required on >> that platform. On Solaris this results in errno being defined as >> >> extern int errno; >> >> as supposed to >> >> #define errno *(errno()) >> >> which makes libpq on Solaris completely useless for every program that >> uses threads for "something". There is still value in compiling it with >> thread support compiler flags, even if it will not result in a thread >> safe libpq. > > Well, first we should find out how to get the thread test to pass for > that patform, but for cases where we can't (FreeBSD doesn't have > getpwuid_r(), we are stuck. (That might be your Solaris problem as > well.) There is no problem with thread safety on Solaris. The configure script for 7.4.2 is broken for it, leading to a broken libpq when using --enable-thread-safey. > > I looked over the code and the only place getpwuid_r (through > pqGetpwuid) is used is in libpq to look up the default username based on > the euid for the connection to the backend. Unfortunately, I can't find > any other way to do such a lookup in a thread-safe manner unless we do a > system() or lock/read /etc/passwd ourselves, both of which are ugly. > > I can't imagine how some OS's cannot give us a thread-safe way to do > this. > > When FreeBSD can't enable threads in 7.5, folks are going to be upset. > In 7.4 we allowed it by having our own C code lock/copy the passwd > structure, but someone pointed out that calls to getpwuid() in other > places in the client code don't have such locking, so it would not work, > so it was removed for 7.5. I disagree that all or nothing is a good strategy. What you have changed this to is to deny using PostgreSQL from multithreaded applications on platforms that have no getpwuid_r() altogether, if their platform happens to require any thread special compiler options for libpq to work in general. Take Slony as an example. It is multithreaded, and we aren't happy that we have to guard the pg_connect() call with a mutex against multiple concurrent calls. But since our connections are of long living nature this is no problem. And nowhere else in the entire code is any call to getpwuid() or anything else. So we have the situation under control. But I really don't want to tell people in the build instructions that they have to edit libpq's Makefile because PostgreSQL's ./configure script is too restrictive. And just to make your day, your tests for thread safety are incomplete. The reason why we use a mutex now on all platforms, thread safe or not, is because in the event you have a kerberos lib available (which is not thread safe), pg_connect() can crash wether you use kerberos or not. So I think when compiling for --enable-thread-safe we should disable kerberos in libpq, right? Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
On 6/9/2004 11:16 AM, Bruce Momjian wrote:
> Jan Wieck wrote:
>> On 6/9/2004 9:36 AM, Bruce Momjian wrote:
>>
>> > Jan Wieck wrote:
>> >> I am wondering why thread_test.c is checking for mktemp()? That function
>> >> is nowhere used in the libpq.
>> >
>> > Uh, it isn't checking for mktemp, it is using it, and it is using it
>> > because someone didn't like hard-coded paths I was using in the past.
>> > Is there something wrong with using mktemp? I have heard of no
>> > portability problems, except some need six X's, and we updated that.
>>
>> There seems to be a portability issue here. Stefan Kaltenbrunner
>> reported a configure failure on sparc64-unknown-openbsd3.5 and the
>> config.log says:
>>
>> /tmp//ccx22029.o: In function `main':
>> /tmp//ccx22029.o(.text+0x8c): warning: mktemp() possibly used unsafely;
>> consider
>> using mkstemp()
>
> Yes, I was wondering how mktemp was going to guard against concurrent
> access. I have applied the following patch to use mkstemp().
>
>> Which is only a warning at this time, it fails later on getpwuid().
>
> Oh, I will need to hear more about that failure.
The relevant part of the config.log is:
configure:17942: checking thread safety of required library functions
configure:17967: gcc -o conftest -O2 -fno-strict-aliasing -pthread
-D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -DIN_CONFIGURE conftest.c -lz -lreadline -lcurses -lresolv
-lcompat-lm
-lutil >&5
/tmp//ccx22029.o: In function `main':
/tmp//ccx22029.o(.text+0x8c): warning: mktemp() possibly used unsafely;
consider using mkstemp()
configure:17970: $? = 0
configure:17972: ./conftest
Your errno is thread-safe.
Your system has sterror_r(); it does not need strerror().
Your system uses getpwuid() which is not thread-safe. **
Your system has getaddrinfo(); it does not need gethostbyname() or gethostbyname_r().
** YOUR PLATFORM IS NOT THREAD-SAFE. **
configure:17975: $? = 1
configure: program exited with status 1
configure: failed program was:
#line 17961 "configure"
#include "confdefs.h"
#include "./src/tools/thread/thread_test.c"
configure:17984: result: no
configure:17986: error:
*** Thread test program failed. Your platform is not thread-safe.
*** Check the file 'config.log'for the exact reason.
>
>
>
> ------------------------------------------------------------------------
>
> Index: src/tools/thread/thread_test.c
> ===================================================================
> RCS file: /cvsroot/pgsql-server/src/tools/thread/thread_test.c,v
> retrieving revision 1.30
> diff -c -c -r1.30 thread_test.c
> *** src/tools/thread/thread_test.c 28 May 2004 18:37:10 -0000 1.30
> --- src/tools/thread/thread_test.c 9 Jun 2004 15:03:29 -0000
> ***************
> *** 104,110 ****
> {
> pthread_t thread1,
> thread2;
> !
> if (argc > 1)
> {
> fprintf(stderr, "Usage: %s\n", argv[0]);
> --- 104,111 ----
> {
> pthread_t thread1,
> thread2;
> ! int fd;
> !
> if (argc > 1)
> {
> fprintf(stderr, "Usage: %s\n", argv[0]);
> ***************
> *** 120,130 ****
> /* Make temp filenames, might not have strdup() */
> temp_filename_1 = malloc(strlen(TEMP_FILENAME_1) + 1);
> strcpy(temp_filename_1, TEMP_FILENAME_1);
> ! mktemp(temp_filename_1);
>
> temp_filename_2 = malloc(strlen(TEMP_FILENAME_2) + 1);
> strcpy(temp_filename_2, TEMP_FILENAME_2);
> ! mktemp(temp_filename_2);
>
> #if !defined(HAVE_GETADDRINFO) && !defined(HAVE_GETHOSTBYNAME_R)
> if (gethostname(myhostname, MAXHOSTNAMELEN) != 0)
> --- 121,133 ----
> /* Make temp filenames, might not have strdup() */
> temp_filename_1 = malloc(strlen(TEMP_FILENAME_1) + 1);
> strcpy(temp_filename_1, TEMP_FILENAME_1);
> ! fd = mkstemp(temp_filename_1);
> ! close(fd);
>
> temp_filename_2 = malloc(strlen(TEMP_FILENAME_2) + 1);
> strcpy(temp_filename_2, TEMP_FILENAME_2);
> ! fd = mkstemp(temp_filename_2);
> ! close(fd);
>
> #if !defined(HAVE_GETADDRINFO) && !defined(HAVE_GETHOSTBYNAME_R)
> if (gethostname(myhostname, MAXHOSTNAMELEN) != 0)
--
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me. #
#================================================== JanWieck@Yahoo.com #
Jan Wieck wrote: > On 6/9/2004 9:36 AM, Bruce Momjian wrote: > > > >> Also, I would suggest that we allow to build the libpq with thread > >> specific compiler options, even if it is not entirely thread safe. It > >> would require that a really multithreaded application has to have > >> mutexes around certain operations, but being entirely unable to > >> configure in a way that adds thread compile options to the CFLAGS makes > >> libpq completely useless for multithreaded programs on some platforms > >> (for example Solaris). > > > > I am confused what you are suggesting. Are you saying to use thread > > flags but not the other things that make is thread-safe? There isn't > > much else other than the flags actually. Now that more OS's are > > thread-safe by default, we could consider using threading if it is > > available by default. We would need some way of reporting that to the > > user, perhaps via an installed readme file or a pg_config output option. > > The problem is that if your thread-safety tests fail, there is no way to > build libpq with -pthread and -DREENTRANT or whatever is required on > that platform. On Solaris this results in errno being defined as > > extern int errno; > > as supposed to > > #define errno *(errno()) > > which makes libpq on Solaris completely useless for every program that > uses threads for "something". There is still value in compiling it with > thread support compiler flags, even if it will not result in a thread > safe libpq. Well, first we should find out how to get the thread test to pass for that patform, but for cases where we can't (FreeBSD doesn't have getpwuid_r(), we are stuck. (That might be your Solaris problem as well.) I looked over the code and the only place getpwuid_r (through pqGetpwuid) is used is in libpq to look up the default username based on the euid for the connection to the backend. Unfortunately, I can't find any other way to do such a lookup in a thread-safe manner unless we do a system() or lock/read /etc/passwd ourselves, both of which are ugly. I can't imagine how some OS's cannot give us a thread-safe way to do this. When FreeBSD can't enable threads in 7.5, folks are going to be upset. In 7.4 we allowed it by having our own C code lock/copy the passwd structure, but someone pointed out that calls to getpwuid() in other places in the client code don't have such locking, so it would not work, so it was removed for 7.5. -- 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
Jan Wieck wrote: > On 6/9/2004 11:45 AM, Bruce Momjian wrote: > > > Jan Wieck wrote: > >> The problem is that if your thread-safety tests fail, there is no way to > >> build libpq with -pthread and -DREENTRANT or whatever is required on > >> that platform. On Solaris this results in errno being defined as > >> > >> extern int errno; > >> > >> as supposed to > >> > >> #define errno *(errno()) > >> > >> which makes libpq on Solaris completely useless for every program that > >> uses threads for "something". There is still value in compiling it with > >> thread support compiler flags, even if it will not result in a thread > >> safe libpq. > > > > Well, first we should find out how to get the thread test to pass for > > that patform, but for cases where we can't (FreeBSD doesn't have > > getpwuid_r(), we are stuck. (That might be your Solaris problem as > > well.) > > There is no problem with thread safety on Solaris. The configure script > for 7.4.2 is broken for it, leading to a broken libpq when using > --enable-thread-safey. Oh, yes, to be fixed in 7.4.3. > > I looked over the code and the only place getpwuid_r (through > > pqGetpwuid) is used is in libpq to look up the default username based on > > the euid for the connection to the backend. Unfortunately, I can't find > > any other way to do such a lookup in a thread-safe manner unless we do a > > system() or lock/read /etc/passwd ourselves, both of which are ugly. > > > > I can't imagine how some OS's cannot give us a thread-safe way to do > > this. > > > > When FreeBSD can't enable threads in 7.5, folks are going to be upset. > > In 7.4 we allowed it by having our own C code lock/copy the passwd > > structure, but someone pointed out that calls to getpwuid() in other > > places in the client code don't have such locking, so it would not work, > > so it was removed for 7.5. > > I disagree that all or nothing is a good strategy. What you have changed > this to is to deny using PostgreSQL from multithreaded applications on > platforms that have no getpwuid_r() altogether, if their platform > happens to require any thread special compiler options for libpq to work > in general. Yes, this was the agreed approach. It wasn't all my idea. > Take Slony as an example. It is multithreaded, and we aren't happy that > we have to guard the pg_connect() call with a mutex against multiple > concurrent calls. But since our connections are of long living nature > this is no problem. And nowhere else in the entire code is any call to > getpwuid() or anything else. So we have the situation under control. But > I really don't want to tell people in the build instructions that they > have to edit libpq's Makefile because PostgreSQL's ./configure script is > too restrictive. Yep, a problem. However, when we create libpq we can't know that your app isn't going to call getpwuid itself, can we; and even if we did, we couldn't know if other libraries you are calling are using it. I bet you don't even know that. > And just to make your day, your tests for thread safety are incomplete. Why incomplete? Because it doesn't handle kerberos? See below. > The reason why we use a mutex now on all platforms, thread safe or not, > is because in the event you have a kerberos lib available (which is not > thread safe), pg_connect() can crash wether you use kerberos or not. So > I think when compiling for --enable-thread-safe we should disable > kerberos in libpq, right? I thought we added kerberos locking to our current CVS. As for your general issue, I don't think we want to head down the road of having a libpq that is safe only if certain qualifications are made of clients calling it. If folks know enough to deal with those qualifications, they can use CFLAGS to pass compiler flags to their libpq build or hack up the code to bypass the thread checks. What we really need is a way to do the uid->username mapping in a thread-safe way. Could we check the environment for $USER or $LOGNAME? Could we require them to be set for thread builds on OS's without getpwuid_r and in cases where the username is not specified in the connection string? If FreeBSD and Solaris both have this issue, it is important for us to solve 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
Well, looks like you are missing getpwuid_r():
Your system uses getpwuid() which is not thread-safe. **
and we don't have any workaround for this.
---------------------------------------------------------------------------
Jan Wieck wrote:
> On 6/9/2004 11:16 AM, Bruce Momjian wrote:
>
> > Jan Wieck wrote:
> >> On 6/9/2004 9:36 AM, Bruce Momjian wrote:
> >>
> >> > Jan Wieck wrote:
> >> >> I am wondering why thread_test.c is checking for mktemp()? That function
> >> >> is nowhere used in the libpq.
> >> >
> >> > Uh, it isn't checking for mktemp, it is using it, and it is using it
> >> > because someone didn't like hard-coded paths I was using in the past.
> >> > Is there something wrong with using mktemp? I have heard of no
> >> > portability problems, except some need six X's, and we updated that.
> >>
> >> There seems to be a portability issue here. Stefan Kaltenbrunner
> >> reported a configure failure on sparc64-unknown-openbsd3.5 and the
> >> config.log says:
> >>
> >> /tmp//ccx22029.o: In function `main':
> >> /tmp//ccx22029.o(.text+0x8c): warning: mktemp() possibly used unsafely;
> >> consider
> >> using mkstemp()
> >
> > Yes, I was wondering how mktemp was going to guard against concurrent
> > access. I have applied the following patch to use mkstemp().
> >
> >> Which is only a warning at this time, it fails later on getpwuid().
> >
> > Oh, I will need to hear more about that failure.
>
> The relevant part of the config.log is:
>
> configure:17942: checking thread safety of required library functions
> configure:17967: gcc -o conftest -O2 -fno-strict-aliasing -pthread
> -D_REENTRANT -D_THREAD_SAFE -D_POSIX_PTHREAD_SEMANTICS -DIN_CONFIGURE
> conftest.c -lz -lreadline -lcurses -lresolv -lcompat -lm
> -lutil >&5
> /tmp//ccx22029.o: In function `main':
> /tmp//ccx22029.o(.text+0x8c): warning: mktemp() possibly used unsafely;
> consider using mkstemp()
> configure:17970: $? = 0
> configure:17972: ./conftest
> Your errno is thread-safe.
> Your system has sterror_r(); it does not need strerror().
> Your system uses getpwuid() which is not thread-safe. **
> Your system has getaddrinfo(); it does not need gethostbyname()
> or gethostbyname_r().
>
> ** YOUR PLATFORM IS NOT THREAD-SAFE. **
> configure:17975: $? = 1
> configure: program exited with status 1
> configure: failed program was:
> #line 17961 "configure"
> #include "confdefs.h"
> #include "./src/tools/thread/thread_test.c"
> configure:17984: result: no
> configure:17986: error:
> *** Thread test program failed. Your platform is not thread-safe.
> *** Check the file 'config.log'for the exact reason.
>
>
>
> >
> >
> >
> > ------------------------------------------------------------------------
> >
> > Index: src/tools/thread/thread_test.c
> > ===================================================================
> > RCS file: /cvsroot/pgsql-server/src/tools/thread/thread_test.c,v
> > retrieving revision 1.30
> > diff -c -c -r1.30 thread_test.c
> > *** src/tools/thread/thread_test.c 28 May 2004 18:37:10 -0000 1.30
> > --- src/tools/thread/thread_test.c 9 Jun 2004 15:03:29 -0000
> > ***************
> > *** 104,110 ****
> > {
> > pthread_t thread1,
> > thread2;
> > !
> > if (argc > 1)
> > {
> > fprintf(stderr, "Usage: %s\n", argv[0]);
> > --- 104,111 ----
> > {
> > pthread_t thread1,
> > thread2;
> > ! int fd;
> > !
> > if (argc > 1)
> > {
> > fprintf(stderr, "Usage: %s\n", argv[0]);
> > ***************
> > *** 120,130 ****
> > /* Make temp filenames, might not have strdup() */
> > temp_filename_1 = malloc(strlen(TEMP_FILENAME_1) + 1);
> > strcpy(temp_filename_1, TEMP_FILENAME_1);
> > ! mktemp(temp_filename_1);
> >
> > temp_filename_2 = malloc(strlen(TEMP_FILENAME_2) + 1);
> > strcpy(temp_filename_2, TEMP_FILENAME_2);
> > ! mktemp(temp_filename_2);
> >
> > #if !defined(HAVE_GETADDRINFO) && !defined(HAVE_GETHOSTBYNAME_R)
> > if (gethostname(myhostname, MAXHOSTNAMELEN) != 0)
> > --- 121,133 ----
> > /* Make temp filenames, might not have strdup() */
> > temp_filename_1 = malloc(strlen(TEMP_FILENAME_1) + 1);
> > strcpy(temp_filename_1, TEMP_FILENAME_1);
> > ! fd = mkstemp(temp_filename_1);
> > ! close(fd);
> >
> > temp_filename_2 = malloc(strlen(TEMP_FILENAME_2) + 1);
> > strcpy(temp_filename_2, TEMP_FILENAME_2);
> > ! fd = mkstemp(temp_filename_2);
> > ! close(fd);
> >
> > #if !defined(HAVE_GETADDRINFO) && !defined(HAVE_GETHOSTBYNAME_R)
> > if (gethostname(myhostname, MAXHOSTNAMELEN) != 0)
>
>
> --
> #======================================================================#
> # It's easier to get forgiveness for being wrong than for being right. #
> # Let's break this rule - forgive me. #
> #================================================== JanWieck@Yahoo.com #
>
-- 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
On 6/9/2004 1:04 PM, Bruce Momjian wrote: > What we really need is a way to do the uid->username mapping in a > thread-safe way. Could we check the environment for $USER or $LOGNAME? > Could we require them to be set for thread builds on OS's without > getpwuid_r and in cases where the username is not specified in the > connection string? Maybe not as popular, but what about breaking backward compatibility and require the DB name to be specified, no username fallback? How many applications really rely on that feature? And people who are used to it from the commandline can set PGDATABASE in their .profile to get it back. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck wrote: > On 6/9/2004 1:04 PM, Bruce Momjian wrote: > > > What we really need is a way to do the uid->username mapping in a > > thread-safe way. Could we check the environment for $USER or $LOGNAME? > > Could we require them to be set for thread builds on OS's without > > getpwuid_r and in cases where the username is not specified in the > > connection string? > > Maybe not as popular, but what about breaking backward compatibility and > require the DB name to be specified, no username fallback? How many > applications really rely on that feature? And people who are used to it > from the commandline can set PGDATABASE in their .profile to get it back. That is only part of where the username is used. I assume it is also used for connections when the username isn't supplied, not just as the default for the database name. Basically on those platforms, either the username would have to be in the environment, or supplied as part of the connection string. -- 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
On 6/9/2004 1:44 PM, Bruce Momjian wrote: > Jan Wieck wrote: >> On 6/9/2004 1:04 PM, Bruce Momjian wrote: >> >> > What we really need is a way to do the uid->username mapping in a >> > thread-safe way. Could we check the environment for $USER or $LOGNAME? >> > Could we require them to be set for thread builds on OS's without >> > getpwuid_r and in cases where the username is not specified in the >> > connection string? >> >> Maybe not as popular, but what about breaking backward compatibility and >> require the DB name to be specified, no username fallback? How many >> applications really rely on that feature? And people who are used to it >> from the commandline can set PGDATABASE in their .profile to get it back. > > That is only part of where the username is used. I assume it is also > used for connections when the username isn't supplied, not just as the > default for the database name. > > Basically on those platforms, either the username would have to be in > the environment, or supplied as part of the connection string. > We have PGUSER, PGHOST, PGPORT, PGDATABASE, all of them you can set in your .profile, why do we need to lookup the uid at all? Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck wrote: > On 6/9/2004 1:44 PM, Bruce Momjian wrote: > > > Jan Wieck wrote: > >> On 6/9/2004 1:04 PM, Bruce Momjian wrote: > >> > >> > What we really need is a way to do the uid->username mapping in a > >> > thread-safe way. Could we check the environment for $USER or $LOGNAME? > >> > Could we require them to be set for thread builds on OS's without > >> > getpwuid_r and in cases where the username is not specified in the > >> > connection string? > >> > >> Maybe not as popular, but what about breaking backward compatibility and > >> require the DB name to be specified, no username fallback? How many > >> applications really rely on that feature? And people who are used to it > >> from the commandline can set PGDATABASE in their .profile to get it back. > > > > That is only part of where the username is used. I assume it is also > > used for connections when the username isn't supplied, not just as the > > default for the database name. > > > > Basically on those platforms, either the username would have to be in > > the environment, or supplied as part of the connection string. > > > > We have PGUSER, PGHOST, PGPORT, PGDATABASE, all of them you can set in > your .profile, why do we need to lookup the uid at all? In case they don't set it, which is very common, I am sure. -- 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
Are people OK with requiring PGUSER, $USER, $LOGNAME, or the username to be supplied by the connection string in libpq on platforms that want threads and don't have getpwuid_r() (Solaris, FreeBSD, etc.)? If so, I can easily create a patch and apply it. --------------------------------------------------------------------------- Jan Wieck wrote: > On 6/9/2004 1:44 PM, Bruce Momjian wrote: > > > Jan Wieck wrote: > >> On 6/9/2004 1:04 PM, Bruce Momjian wrote: > >> > >> > What we really need is a way to do the uid->username mapping in a > >> > thread-safe way. Could we check the environment for $USER or $LOGNAME? > >> > Could we require them to be set for thread builds on OS's without > >> > getpwuid_r and in cases where the username is not specified in the > >> > connection string? > >> > >> Maybe not as popular, but what about breaking backward compatibility and > >> require the DB name to be specified, no username fallback? How many > >> applications really rely on that feature? And people who are used to it > >> from the commandline can set PGDATABASE in their .profile to get it back. > > > > That is only part of where the username is used. I assume it is also > > used for connections when the username isn't supplied, not just as the > > default for the database name. > > > > Basically on those platforms, either the username would have to be in > > the environment, or supplied as part of the connection string. > > > > We have PGUSER, PGHOST, PGPORT, PGDATABASE, all of them you can set in > your .profile, why do we need to lookup the uid at all? > > > Jan > > -- > #======================================================================# > # It's easier to get forgiveness for being wrong than for being right. # > # Let's break this rule - forgive me. # > #================================================== JanWieck@Yahoo.com # > -- 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 <pgman@candle.pha.pa.us> writes:
> Are people OK with requiring PGUSER, $USER, $LOGNAME, or the username to
> be supplied by the connection string in libpq on platforms that want
> threads and don't have getpwuid_r() (Solaris, FreeBSD, etc.)?
AFAICS that was not what Jan was suggesting at all. I don't like it
either --- changing the user-visible behavior based on whether we think
the platform is thread-safe or not is horrid.
What I understood Jan to be saying is that we should be willing to build
the most thread-safe approximation we can when --enable-thread-safety
is requested. Don't bomb out if you don't have getpwuid_r, just give
a warning and then use getpwuid.
regards, tom lane
On 6/10/2004 2:11 AM, Tom Lane wrote: > Bruce Momjian <pgman@candle.pha.pa.us> writes: >> Are people OK with requiring PGUSER, $USER, $LOGNAME, or the username to >> be supplied by the connection string in libpq on platforms that want >> threads and don't have getpwuid_r() (Solaris, FreeBSD, etc.)? > > AFAICS that was not what Jan was suggesting at all. I don't like it > either --- changing the user-visible behavior based on whether we think > the platform is thread-safe or not is horrid. > > What I understood Jan to be saying is that we should be willing to build > the most thread-safe approximation we can when --enable-thread-safety > is requested. Don't bomb out if you don't have getpwuid_r, just give > a warning and then use getpwuid. Make it so that --enable-thread-safety bombs out, but make another --enable-thread-safey-anyway work the way Tom descibed it. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck wrote: > On 6/10/2004 2:11 AM, Tom Lane wrote: > > > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> Are people OK with requiring PGUSER, $USER, $LOGNAME, or the username to > >> be supplied by the connection string in libpq on platforms that want > >> threads and don't have getpwuid_r() (Solaris, FreeBSD, etc.)? > > > > AFAICS that was not what Jan was suggesting at all. I don't like it > > either --- changing the user-visible behavior based on whether we think > > the platform is thread-safe or not is horrid. > > > > What I understood Jan to be saying is that we should be willing to build > > the most thread-safe approximation we can when --enable-thread-safety > > is requested. Don't bomb out if you don't have getpwuid_r, just give > > a warning and then use getpwuid. > > Make it so that --enable-thread-safety bombs out, but make another > --enable-thread-safey-anyway work the way Tom descibed it. Sure, we can do that by just not running the thread_test program. In fact a cross-compile already skips running the test. -- 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
On 6/10/2004 8:49 AM, Bruce Momjian wrote: > Jan Wieck wrote: >> Make it so that --enable-thread-safety bombs out, but make another >> --enable-thread-safey-anyway work the way Tom descibed it. > > Sure, we can do that by just not running the thread_test program. In > fact a cross-compile already skips running the test. That sounds good to me. Jan -- #======================================================================# # It's easier to get forgiveness for being wrong than for being right. # # Let's break this rule - forgive me. # #================================================== JanWieck@Yahoo.com #
Jan Wieck wrote: > > I looked over the code and the only place getpwuid_r (through > > pqGetpwuid) is used is in libpq to look up the default username based on > > the euid for the connection to the backend. Unfortunately, I can't find > > any other way to do such a lookup in a thread-safe manner unless we do a > > system() or lock/read /etc/passwd ourselves, both of which are ugly. > > > > I can't imagine how some OS's cannot give us a thread-safe way to do > > this. > > > > When FreeBSD can't enable threads in 7.5, folks are going to be upset. > > In 7.4 we allowed it by having our own C code lock/copy the passwd > > structure, but someone pointed out that calls to getpwuid() in other > > places in the client code don't have such locking, so it would not work, > > so it was removed for 7.5. > > I disagree that all or nothing is a good strategy. What you have changed > this to is to deny using PostgreSQL from multithreaded applications on > platforms that have no getpwuid_r() altogether, if their platform > happens to require any thread special compiler options for libpq to work > in general. > > Take Slony as an example. It is multithreaded, and we aren't happy that > we have to guard the pg_connect() call with a mutex against multiple > concurrent calls. But since our connections are of long living nature > this is no problem. And nowhere else in the entire code is any call to > getpwuid() or anything else. So we have the situation under control. But > I really don't want to tell people in the build instructions that they > have to edit libpq's Makefile because PostgreSQL's ./configure script is > too restrictive. OK, I have added a configure option --enable-thread-safety-force that compiles with thread safety even if the OS tests fail. I have not mentioned this in the SGML docs but mention the option in configure --help and when you get a thread test failure with --enable-thread-safety. Patch attached and applied. > And just to make your day, your tests for thread safety are incomplete. > The reason why we use a mutex now on all platforms, thread safe or not, > is because in the event you have a kerberos lib available (which is not > thread safe), pg_connect() can crash wether you use kerberos or not. So > I think when compiling for --enable-thread-safe we should disable > kerberos in libpq, right? We have libpq locking for kerberos in CVS. -- 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: configure =================================================================== RCS file: /cvsroot/pgsql-server/configure,v retrieving revision 1.376 diff -c -c -r1.376 configure *** configure 24 Jun 2004 18:55:17 -0000 1.376 --- configure 10 Jul 2004 01:08:16 -0000 *************** *** 846,851 **** --- 846,852 ---- --enable-depend turn on automatic dependency tracking --enable-cassert enable assertion checks (for debugging) --enable-thread-safety make client libraries thread-safe + --enable-thread-safety-force force thread-safety in spite of thread test failure --disable-largefile omit support for large files Optional Packages: *************** *** 2937,2947 **** case $enableval in yes) ! ! cat >>confdefs.h <<\_ACEOF ! #define ENABLE_THREAD_SAFETY 1 ! _ACEOF ! ;; no) : --- 2938,2944 ---- case $enableval in yes) ! : ;; no) : *************** *** 2958,2963 **** --- 2955,2994 ---- fi; + + + # Check whether --enable-thread-safety-force or --disable-thread-safety-force was given. + if test "${enable_thread_safety_force+set}" = set; then + enableval="$enable_thread_safety_force" + + case $enableval in + yes) + : + ;; + no) + : + ;; + *) + { { echo "$as_me:$LINENO: error: no argument expected for --enable-thread-safety-force option" >&5 + echo "$as_me: error: no argument expected for --enable-thread-safety-force option" >&2;} + { (exit 1); exit 1; }; } + ;; + esac + + else + enable_thread_safety_force=no + + fi; + + if test "$enable_thread_safety" = yes -o + test "$enable_thread_safety_force" = yes; then + enable_thread_safety="yes" # for 'force' + + cat >>confdefs.h <<\_ACEOF + #define ENABLE_THREAD_SAFETY 1 + _ACEOF + + fi echo "$as_me:$LINENO: result: $enable_thread_safety" >&5 echo "${ECHO_T}$enable_thread_safety" >&6 *************** *** 17941,17947 **** # We have to run the thread test near the end so we have all our symbols # defined. Cross compiling throws a warning. # ! if test "$enable_thread_safety" = yes; then echo "$as_me:$LINENO: checking thread safety of required library functions" >&5 echo $ECHO_N "checking thread safety of required library functions... $ECHO_C" >&6 --- 17972,17991 ---- # We have to run the thread test near the end so we have all our symbols # defined. Cross compiling throws a warning. # ! if test "$enable_thread_safety_force" = yes; then ! { echo "$as_me:$LINENO: WARNING: ! *** Skipping thread test program. --enable-thread-safety-force was used. ! *** Run the program in src/tools/thread on the your machine and add ! proper locking function calls to your applications to guarantee thread ! safety. ! " >&5 ! echo "$as_me: WARNING: ! *** Skipping thread test program. --enable-thread-safety-force was used. ! *** Run the program in src/tools/thread on the your machine and add ! proper locking function calls to your applications to guarantee thread ! safety. ! " >&2;} ! elif test "$enable_thread_safety" = yes; then echo "$as_me:$LINENO: checking thread safety of required library functions" >&5 echo $ECHO_N "checking thread safety of required library functions... $ECHO_C" >&6 *************** *** 17954,17964 **** echo "${ECHO_T}maybe" >&6 { echo "$as_me:$LINENO: WARNING: *** Skipping thread test program because of cross-compile build. ! *** Run the program in src/tools/thread on the target matchine. " >&5 echo "$as_me: WARNING: *** Skipping thread test program because of cross-compile build. ! *** Run the program in src/tools/thread on the target matchine. " >&2;} else cat >conftest.$ac_ext <<_ACEOF --- 17998,18008 ---- echo "${ECHO_T}maybe" >&6 { echo "$as_me:$LINENO: WARNING: *** Skipping thread test program because of cross-compile build. ! *** Run the program in src/tools/thread on the target machine. " >&5 echo "$as_me: WARNING: *** Skipping thread test program because of cross-compile build. ! *** Run the program in src/tools/thread on the target machine. " >&2;} else cat >conftest.$ac_ext <<_ACEOF *************** *** 17988,17997 **** echo "${ECHO_T}no" >&6 { { echo "$as_me:$LINENO: error: *** Thread test program failed. Your platform is not thread-safe. ! *** Check the file 'config.log'for the exact reason." >&5 echo "$as_me: error: *** Thread test program failed. Your platform is not thread-safe. ! *** Check the file 'config.log'for the exact reason." >&2;} { (exit 1); exit 1; }; } fi rm -f core core.* *.core conftest$ac_exeext conftest.$ac_objext conftest.$ac_ext --- 18032,18053 ---- echo "${ECHO_T}no" >&6 { { echo "$as_me:$LINENO: error: *** Thread test program failed. Your platform is not thread-safe. ! *** Check the file 'config.log'for the exact reason. ! *** ! *** You can use the configure option --enable-thread-safety-force ! *** to force threads to be enabled. However, you must then run ! *** the program in src/tools/thread and add locking function calls ! *** to your applications to guarantee thread safety. ! " >&5 echo "$as_me: error: *** Thread test program failed. Your platform is not thread-safe. ! *** Check the file 'config.log'for the exact reason. ! *** ! *** You can use the configure option --enable-thread-safety-force ! *** to force threads to be enabled. However, you must then run ! *** the program in src/tools/thread and add locking function calls ! *** to your applications to guarantee thread safety. ! " >&2;} { (exit 1); exit 1; }; } fi rm -f core core.* *.core conftest$ac_exeext conftest.$ac_objext conftest.$ac_ext Index: configure.in =================================================================== RCS file: /cvsroot/pgsql-server/configure.in,v retrieving revision 1.365 diff -c -c -r1.365 configure.in *** configure.in 24 Jun 2004 18:55:18 -0000 1.365 --- configure.in 10 Jul 2004 01:08:17 -0000 *************** *** 358,366 **** # Enable thread-safe client libraries # AC_MSG_CHECKING([allow thread-safe client libraries]) ! PGAC_ARG_BOOL(enable, thread-safety, no, [ --enable-thread-safety make client libraries thread-safe], ! [AC_DEFINE([ENABLE_THREAD_SAFETY], 1, ! [Define to 1 to build client libraries as thread-safe code. (--enable-thread-safety)])]) AC_MSG_RESULT([$enable_thread_safety]) AC_SUBST(enable_thread_safety) --- 358,371 ---- # Enable thread-safe client libraries # AC_MSG_CHECKING([allow thread-safe client libraries]) ! PGAC_ARG_BOOL(enable, thread-safety, no, [ --enable-thread-safety make client libraries thread-safe]) ! PGAC_ARG_BOOL(enable, thread-safety-force, no, [ --enable-thread-safety-force force thread-safety in spite of threadtest failure]) ! if test "$enable_thread_safety" = yes -o ! test "$enable_thread_safety_force" = yes; then ! enable_thread_safety="yes" # for 'force' ! AC_DEFINE([ENABLE_THREAD_SAFETY], 1, ! [Define to 1 to build client libraries as thread-safe code. (--enable-thread-safety)]) ! fi AC_MSG_RESULT([$enable_thread_safety]) AC_SUBST(enable_thread_safety) *************** *** 1184,1190 **** # We have to run the thread test near the end so we have all our symbols # defined. Cross compiling throws a warning. # ! if test "$enable_thread_safety" = yes; then AC_MSG_CHECKING([thread safety of required library functions]) _CFLAGS="$CFLAGS" --- 1189,1202 ---- # We have to run the thread test near the end so we have all our symbols # defined. Cross compiling throws a warning. # ! if test "$enable_thread_safety_force" = yes; then ! AC_MSG_WARN([ ! *** Skipping thread test program. --enable-thread-safety-force was used. ! *** Run the program in src/tools/thread on the your machine and add ! proper locking function calls to your applications to guarantee thread ! safety. ! ]) ! elif test "$enable_thread_safety" = yes; then AC_MSG_CHECKING([thread safety of required library functions]) _CFLAGS="$CFLAGS" *************** *** 1196,1206 **** [AC_MSG_RESULT(no) AC_MSG_ERROR([ *** Thread test program failed. Your platform is not thread-safe. ! *** Check the file 'config.log'for the exact reason.])], [AC_MSG_RESULT(maybe) AC_MSG_WARN([ *** Skipping thread test program because of cross-compile build. ! *** Run the program in src/tools/thread on the target matchine. ])]) CFLAGS="$_CFLAGS" LIBS="$_LIBS" --- 1208,1224 ---- [AC_MSG_RESULT(no) AC_MSG_ERROR([ *** Thread test program failed. Your platform is not thread-safe. ! *** Check the file 'config.log'for the exact reason. ! *** ! *** You can use the configure option --enable-thread-safety-force ! *** to force threads to be enabled. However, you must then run ! *** the program in src/tools/thread and add locking function calls ! *** to your applications to guarantee thread safety. ! ])], [AC_MSG_RESULT(maybe) AC_MSG_WARN([ *** Skipping thread test program because of cross-compile build. ! *** Run the program in src/tools/thread on the target machine. ])]) CFLAGS="$_CFLAGS" LIBS="$_LIBS"
Jan Wieck wrote: > On 6/10/2004 2:11 AM, Tom Lane wrote: > > > Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> Are people OK with requiring PGUSER, $USER, $LOGNAME, or the username to > >> be supplied by the connection string in libpq on platforms that want > >> threads and don't have getpwuid_r() (Solaris, FreeBSD, etc.)? > > > > AFAICS that was not what Jan was suggesting at all. I don't like it > > either --- changing the user-visible behavior based on whether we think > > the platform is thread-safe or not is horrid. > > > > What I understood Jan to be saying is that we should be willing to build > > the most thread-safe approximation we can when --enable-thread-safety > > is requested. Don't bomb out if you don't have getpwuid_r, just give > > a warning and then use getpwuid. > > Make it so that --enable-thread-safety bombs out, but make another > --enable-thread-safey-anyway work the way Tom descibed it. Done as --enable-thread-safety-force. -- 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