> >> It is not; the main reason why not is a brain-dead part of 
> >> the API that exposes modifiable global variables.
> 
> > Hmm. Really?
> 
> PQconnectdb() is the function that's not thread-safe; if you had
> multiple threads invoking PQconnectdb() in parallel you would see a
> problem.  PQconndefaults() is the function that creates an 
> API problem,
> because it exposes the static variable that PQconnectdb() 
> ought not have
> had in the first place.

Ok. Now I see it. I guess my code worked because I run PQconnectdb() at the
start of the program, and hand down the PGconn:s to the threads later. So
only one thread can call PQconnectdb().


> There might be some other problems too, but that's the main one I'm
> aware of.  If we didn't mind breaking existing apps that use
> PQconndefaults(), it would be straightforward to fix...

Wouldn't it be possible to do something like this (ok, a little bit ugly,
but shouldn't break the clients):

Store the "active" PQconninfoOptions array inside the PGconn struct. That
way, the user should not require to change anything when doing PQconnectdb()
and the likes.

Rewrite the conninfo_xxx functions to take a (PQconninfoOptions *) as
parameter to work on, instead of working on the static array.

Keep the static array, rename it to PQconninfoDefaultOptions, make it
contain the *default* options *from the beginning*, and declare it as
"const". Then have PQconndefaults() return that array. Then the
PQconndefaults() works just like before, and does not break the old
programs.

Shouldn't this be possible to achieve without any changes in the API?

If you don't see anything obviously wrong with this, I can try to put
together a patch to do that. It'd be really nice to have a thread-safe
client lib :-)


> You might want some kind of reference-counting
> mechanism for PGresults though.
In this case, the PGresults are owned by the client connections, and are
only used by one client connection at a time, and they are freed when the
client connection ends. The PGconns are owned one each by the Worker Threads
in the pool, and are freed when the worker thread is stopped (which is when
the application is stopped). So no special reference-counting should be
needed.

//Magnus

Wed, 4 Aug 1999 14:52:15 +0200 
Magnus Hagander <mha@sollentuna.net>
RE: [HACKERS] Threads 
[ interfaces list added, since we are discussing an incompatible libpq
API change to fix the problem that PQconnectdb() is not thread-safe ]

Magnus Hagander <mha@sollentuna.net> writes:
>> There might be some other problems too, but that's the main one I'm
>> aware of.  If we didn't mind breaking existing apps that use
>> PQconndefaults(), it would be straightforward to fix...

> Wouldn't it be possible to do something like this (ok, a little bit ugly,
> but shouldn't break the clients):

> Store the "active" PQconninfoOptions array inside the PGconn struct. That
> way, the user should not require to change anything when doing PQconnectdb()
> and the likes.

I don't think we'd need to store the array on a long-term basis; it is
really only needed as working storage during PQconnectdb.  So it could
be malloc'd at PQconnectdb entry and free'd at exit, in the normal case
where it's just supporting a PQconnectdb call.  As you saw, that'd be
a pretty straightforward set of changes inside fe-connect.c.

The problem is how should PQconndefaults() act.

> Keep the static array, rename it to PQconninfoDefaultOptions, make it
> contain the *default* options *from the beginning*, and declare it as
> "const". Then have PQconndefaults() return that array.

It's not const because the default values are not constants --- they
depend on environment variables.  In theory the results could change
from call to call, if the client program does putenv()'s in between.
I dunno whether there are any thread packages that keep separate
environment-variable sets for each thread, but if so then the results
could theoretically vary across threads.

The most natural thing given the above change would be to say that what
PQconndefaults() returns is a malloc'd array, and the user program is
required to free said array when done with it.  (Actually, required to
call some helper routine inside fe-connect.c, which would know to free
the subsidiary variable strings along with the top-level array...)

The breakage here is that existing code won't know to call the free
routine, and will therefore suffer a memory leak of ~ a few hundred bytes
per PQconndefaults() call.  It might well be that we could live with
that, since I'll bet that most client programs don't use PQconndefaults
at all, much less call it so many times that a leak of that size would be
a problem.  Comments?

What I'm envisioning is a static const array that contains all the
fixed fields of PQconninfoOptions, but the variable fields (just
the "val" current-value field, AFAIR) are permanently NULL.  Then
to create a working copy you do
	ptr = malloc(sizeof(PQconninfoOptions));
	memcpy(ptr, PQconninfoOptions, sizeof(PQconninfoOptions));
The free routine would run down the work array freeing any non-null val
strings, then free the array itself.  No copying or freeing of the
constant subsidiary strings (such as "keyword") is needed.

If you want to work on it, be my guest...

			regards, tom lane

Wed, 04 Aug 1999 10:08:00 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: [HACKERS] Threads 
On  4 Aug, Tom Lane wrote:

> [ Snip discussion regarding PQconnectdb()s thread-safety ]
> 
> If you want to work on it, be my guest...

I don't have time to think about this today, so I can't comment on how
it should work, but I _am_ currently working in this area - I am
providing non-blocking versions of the connect statements, as discussed
on the interfaces list a couple of weeks ago. In fact, it is pretty
much done, apart from a tidy-up, documentation, and testing. I don't
see any point in two people hammering away at the same code - it will
only make work when we try to merge again - so perhaps I should
implement what ever is decided - I don't mind doing so.  However, if I
didn't get it done this weekend it would have to be mid-to-late
September, since I'm going away.  Would that be a problem for anyone?

I had noticed that the connect statements weren't thread-safe, but
was neither aware that that was a problem for anyone, nor inclined to
audit the whole of libpq for thread-safety, so I left it alone.

Ewan.



Wed, 4 Aug 1999 17:18:19 +0100 (BST)
eem21@cam.ac.uk
Re: [INTERFACES] Re: [HACKERS] Threads 