List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:January 6 2011 10:27am
Subject:Re: bzr commit into mysql-trunk branch (tor.didriksen:3462) Bug#59309
View as plain text  
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
>
>

Thread
bzr commit into mysql-trunk branch (tor.didriksen:3462) Bug#59309Tor Didriksen5 Jan
  • Re: bzr commit into mysql-trunk branch (tor.didriksen:3462) Bug#59309Jon Olav Hauglid6 Jan
    • Re: bzr commit into mysql-trunk branch (tor.didriksen:3462) Bug#59309Tor Didriksen6 Jan