Thread: RE: [HACKERS] Threads
> >> 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
[ 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 doptr = 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
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.