On Thu, Jan 6, 2011 at 11:14 AM, Jon Olav Hauglid <jon.hauglid@stripped>wrote:
> 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.
OK
>
>
> /*
>> 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?
I cannot see any reason to capitalize '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?
OK, makes sense.
Moving it from sql_base to sql_class then.
>
>
> - 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?
yes
>
>
> === 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
>
>
thanks, new commit to follow shortly...
-- didrik
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1
>
>
