Code review comment for lp://staging/~clint-fewbar/drizzle/refactor-lock

On Aug 6, 2009, at 3:13 PM, Monty Taylor wrote:

> Review: Needs Fixing
> Looking really good. A few minor issues to work out:
>
> in lock.cc, add a using namespace drizzled; right after using
> namespace std; Then you can remove the drizzled:: prefix throughout
> that file.
>
> +drizzled::Lock *Session::lockTables(Table **tables, uint32_t count,
> + uint32_t flags, bool
> *need_reopen)
> +int Session::lockExternal(Table **tables, uint32_t count)
> +void Session::unlockSomeTables(Table **table,uint32_t count)
> +void Session::abortLock(Table *table, bool upgrade_lock)
> +bool Session::abortLockForThread(Table *table)
> +int Session::unlockExternal(Table **table,uint32_t count)
>
> This looks like it should actually live in session.cc? Or is there a
> reason there here?
>

The only reason I didn't do that was to keep the functions where they
originally lived, and some abiguity in the coding standards. The only
place this is mentioned (just checked again to make sure it hasn't
changed recently) is here:

http://drizzle.org/wiki/Coding_Standards#Class.2FStructure_File_Conventions

which states

"All new classes get their own .h and .cc files. We will need to pull
apart the mess that exists today, but that is a long term goal."

So, if that needs to happen now as part of this change, I'll go ahead
and move all of those methods to another file, however, I was thinking
that should be done as part of the larger effort to cleanup all the
rest of the functions in the file that need to be renamed and probably
done as methods of Session or TableList, namely:

TableList *mysql_lock_have_duplicate(Session *session, TableList
*needle,
                                      TableList *haystack);
bool lock_global_read_lock(Session *session);
void unlock_global_read_lock(Session *session);
bool wait_if_global_read_lock(Session *session, bool abort_on_refresh,
                               bool is_not_commit);
void start_waiting_global_read_lock(Session *session);
bool make_global_read_lock_block_commit(Session *session);

int lock_and_wait_for_table_name(Session *session, TableList
*table_list);
int lock_table_name(Session *session, TableList *table_list, bool
check_in_use);
void unlock_table_name(TableList *table_list);
bool wait_for_locked_table_names(Session *session, TableList
*table_list);
bool lock_table_names(Session *session, TableList *table_list);
void unlock_table_names(TableList *table_list, TableList *last_table);
bool lock_table_names_exclusively(Session *session, TableList
*table_list);

> In:
>
> +void Session::unlockSomeTables(Table **table,uint32_t count)
> +void Session::abortLock(Table *table, bool upgrade_lock)
> +bool Session::abortLockForThread(Table *table)
>
> Seems like you could replace:
>
> drizzled::Lock *sql_lock= new (nothrow) drizzled::Lock;
> with
> drizzled::Lock sql_lock;
> no?
>

Yes, done and tested.

It raises a question which is actually more obvious with the explicit
new/delete;

If initLockData() were to put 'this' into some other structure on the
heap, that would cause bad things to happen as 'this' will be deleted.
So, is there a way to get the compiler to fail if somebody copies
'this' into the heap from a statically allocated object, or must we
rely on vigilance alone to protect against that situation? Seems like
one of those things that must be put on the developer's head.

> + memmove((table+x), (table+x+1),
> + (old_tables - x) * sizeof(Table*));
> + memmove((locks + tbl->lock_data_start),
> + (locks + lock_data_end),
> + (lock_count - lock_data_end) *
> + sizeof(THR_LOCK_DATA*));
>
>
> The line-continuation style here should be:
>
> + memmove((table+x), (table+x+1),
> + (old_tables - x) * sizeof(Table*));
>

Done.

> Is it possible for this:
>
> drizzled::Lock::Lock(Lock *a, Lock *b)
>
> to be
>
> drizzled::Lock::Lock(const Lock &a, const Lock &b)
>
> I won't die if it isn't. Oh! My! No it's not.
>
> + delete a;
> + delete b;
>
> I'm not sure how I feel about that- but something tells me there are
> bigger implications to changing that.
>

A funny thing happened to me on the way to the trunk.. somebody
removed the need for 'mysql_lock_merge', which, in turn, removed the
need for this constructor. Actually I looked at 2009.04.997 and
couldn't find a call to it there.

Hopefully it won't be needed again.. should it be removed?

If not then it would definitely make sense to change them to const
references and remove the deletes.

>
> --
> https://code.edge.launchpad.net/~clint-fewbar/drizzle/refactor-lock/
> +merge/9569
> You are the owner of lp:~clint-fewbar/drizzle/refactor-lock.