FreeTDS Development Group

[liam AT inodes.org]

On Mon, Sep 20, 2004 at 05:24:31PM +0200, ZIGLIO, Frediano, VF-IT wrote:

> 1- pthread.h it's not that portable, configure script and perhaps some
> macros/functions are needed

Agreed, but I'm not very familiar with autoconf development, so I left 
that to the maintainers to do.

I don't really agree that POSIX threads are not that portable, but I'll 
defer to your preference as a maintainer on this matter and use the 
macros in your patch.

> 2- you use lock for tds_ctx however I don't know if this it's a good
> idea. Context it's used in libTDS only for read informations (I checked
> this in all libTDS source)... you lock entire dblib for too long and you
> have also to consided that a pointer to context it's stored in TDSSOCKET
> structure so you should lock every libTDS call (a good way to make dblib
> a monothread library... not that fine...). I looker even in dblib code
> and context are allocated and initialized in dbinit and uninitialized
> and freed in dbexit so this seems not a problem... just don't use lock
> if you use tds_ctx.

I know that the patch used the mutex in a fairly course grained manner
but that was on purpose. The POSIX memory visibility rules state that
if you modify data in one thread and access it in another there is no 
guarantee that you will see the correct value unless a mutex is used.

That is, if I did a dbinit() in one thread, dbconnect() in another, 
grabbed the results from a query and processed them in a thread pool,
things would most likely fall apart.

I appreciate that you took the time to look through the underlying TDS 
code and I agree that your patch is an improvement.

I've double-checked your patch and I've made a few modifications to 
ensure that the POSIX memory visibility rules are followed correctly.

Essentially, there were some areas that needed further mutex protection.
Also, declaring variablers volatile makes absolutely no difference to
threaded applications. Volatile can not be used in exchange for a mutex.

Also, I noticed tds_convert() is passed the tds_ctx but it (and any
other convert function it calls internally) do not appear to use it from
my quick check. If I've missed something and it does, it really needs to
be protected with a mutex. Can the API be modified to remove this 
argument? 

Finally, I am concerned about the use of tds_ctx internally by other 
tds functions without a mutex. Is there any reason why each DBPROCESS 
structure should not have its own tds_ctx? 

Thanks.

Attachment: dblib.c.patch.gz
Description: Binary data