From: Jon Olav Hauglid
Date: January 6 2011 10:14am
Subject: Re: bzr commit into mysql-trunk branch (tor.didriksen:3462) Bug#59309
List-Archive: http://lists.mysql.com/commits/128056
Message-Id: <4D2595EB.6060603@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

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