Code review comment for lp:~stewart/drizzle/bug492391-i_s-concurrency

Stewart Smith wrote:
> On Thu, Jan 07, 2010 at 04:05:19PM -0000, Jay Pipes wrote:
>> Some little things, one bigger thing. :)
>>
>> The bigger thing, and I'm sure you've already thought about this, is
>> the fact that for each call to an I_S table, the underlying table/view
>> rows are copied into the InformationCursor::rows memory. This is
>> clearly a suboptimal strategy, but I'm going to assume you've already
>> discussed this with Brian?
>
> This strategy is complete poo :)
>
> It is, however, the simplest patch to get to working from where we can
> fix things to actually not suck (that patch is... well.. huge).

OK, I figured as much :)

>> Also, the following lines should be reversed IMHO:
>>
>> 46 + pthread_mutex_destroy(&mutex);
>> 47 + pthread_mutexattr_destroy(&mutex_attr);
>>
>> The reason is that pthread_mutex_destroy() may invalidate the data
>> at address &mutex_attr, and pthread_mutexattr_destroy() may return
>> EINVAL. Not sure of the importance, but you may want to place (void)
>> before both the calls...
>
> interesting... where you get this info from?

Per this link:

http://docs.sun.com/app/docs/doc/802-1949/6i5ur8qam?a=view

"If a mutex is dynamically allocated and was initialized with an
attribute object, its attribute object should be freed with
pthread_mutexattr_destroy() before the mutex itself is freed."

>> Finally, instead of calling pthread_mutex_lock() directly, like in InformationCursor::rnd_init():
>>
>> 59 + pthread_mutex_lock(&sch_table->mutex);
>>
>> it would be better to encapsulate the locking in a
>> InfoSchemaTable::lock() method. This would make it much easier to
>> change (for instance if you wanted to switch to a rwlock instead of a
>> mutex...)
>
> I don't want it to be misunderstood as a lock on the
> InfoSchemaTable... just on the rows...

But it *is* essentially a lock on the whole object :)

> but i also don't want the code lasting more than a trivial amount of
> time :)

ack. :)

-jay