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

Hi!

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?

Tiny style fixups:

77 + while(copy_iter != sch_table->getRows().end())

One space after while, please :)

There is some space/indent weirdness going on in the constructors (see below on diff). Indents should be 2 spaces (there is one area they are 4 spaces below.)

Unless there's a good reason not to do so, the general trend I've seen in the code is to have constructors and other method definitions be in the .cc files and not the .h files where they are implicitly inlined. You have the setup of the mutex attr and the mutex itself duplicated in both constructors and they are in the .h file. I'd prefer to see the mutex and mutex attr initialization put into an init() private method and see the constructors defined in the .cc file.

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...

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...)

Cheers!

Jay