Re: 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(at)Yahoo(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)Yahoo(dot)com #

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(at)Yahoo(dot)com #

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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)Yahoo(dot)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(at)Yahoo(dot)com #

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(at)Yahoo(dot)com #
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)Yahoo(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)Yahoo(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)Yahoo(dot)com #
>

--
Bruce Momjian | http://candle.pha.pa.us
pgman(at)candle(dot)pha(dot)pa(dot)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 <pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)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(at)Yahoo(dot)com #

Jan Wieck wrote:
> On 6/10/2004 2:11 AM, Tom Lane wrote:
>
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

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(at)Yahoo(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073

Jan Wieck wrote:
> On 6/10/2004 2:11 AM, Tom Lane wrote:
>
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)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(at)candle(dot)pha(dot)pa(dot)us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup. | Newtown Square, Pennsylvania 19073