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

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

> Tiny style fixups:
>
> 77 + while(copy_iter != sch_table->getRows().end())
>
> One space after while, please :)

i want a commit hook :)

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

hrrm... maybe emacs mode fail there.

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

okay. not that much of a deal either way, but sure.

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

> 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 i also don't want the code lasting more than a trivial amount of
time :)

--
Stewart Smith