Hello,
On 01/05/2011 03:51 PM, Tor Didriksen wrote:
>
> 3462 Tor Didriksen 2011-01-05
> Bug #59309 Cleanup MDL - THD interface
See comments below.
> === modified file 'sql/mdl.cc'
> @@ -1690,15 +1691,16 @@ void MDL_object_lock::notify_conflicting
>
> {
> MDL_context *conflicting_ctx= conflicting_ticket->get_ctx();
> + MDL_context_owner *conflicting_owner= conflicting_ctx->get_owner();
I'd prefer using conflicting_ctx->get_owner() directly in the function
call below, but that's a nitpick.
> /*
> If thread which holds conflicting lock is waiting on table-level
> lock or some other non-MDL resource we might need to wake it up
> by calling code outside of MDL.
> */
> - mysql_notify_thread_having_shared_lock(ctx->get_thd(),
> - conflicting_ctx->get_thd(),
> - conflicting_ctx->get_needs_thr_lock_abort());
> + ctx->get_owner()->
> + notify_shared_lock(conflicting_owner,
> + conflicting_ctx->get_needs_thr_lock_abort());
> }
> }
> }
> === modified file 'sql/mdl.h'
> +
> +/**
> + An iterface to separate the MDL classes from the THD,
> + so that we can do unit testing.
> + */
Interface?
> + /**
> + @see mysql_notify_thread_having_shared_lock()
> + */
> + virtual bool notify_shared_lock(MDL_context_owner *in_use,
> + bool needs_thr_lock_abort) = 0;
I wonder if something should be done with
mysql_notify_thread_having_shared_lock(). It's only used by the THD
implementation of notify_shared_lock() from what I can see. Maybe the
function could be removed and the code moved there?
> - inline THD *get_thd() const { return m_thd; }
> + THD *get_thd() const { return m_owner->get_thd(); }
> + MDL_context_owner *get_owner() { return m_owner; }
I think get_thd() could be made private or just removed. Seems it's only
used by two DEBUG_SYNC calls in MDL_context::acquire_lock() and
MDL_context::upgrade_shared_lock_to_exclusive().
> === modified file 'sql/sql_base.cc'
Update the copyright notice in sql_base.cc as well?
> === modified file 'sql/sql_class.h'
> +extern bool mysql_notify_thread_having_shared_lock(THD *thd, THD *in_use,
> + bool needs_thr_lock_abort);
> + bool notify_shared_lock(MDL_context_owner *in_use,
> + bool needs_thr_lock_abort)
> + {
> + return
> + mysql_notify_thread_having_shared_lock(get_thd(),
> + in_use->get_thd(),
> + needs_thr_lock_abort);
> + }
> +
See earlier comment about mysql_notify_thread_having_shared_lock().
Otherwise the patch looks great to me.
--- Jon Olav
