maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxxx>]

Hi, Holyfoot!

(Kostja - there're a couple of questions at the end that you might want
to reply to)

On Apr 02, holyfoot@xxxxxxxxxxxx wrote:
> At file:///home/hf/wmar/mdev-3917/
> 
> ------------------------------------------------------------
> revno: 3510
> revision-id: holyfoot@xxxxxxxxxxxx-20130402085219-1sxuzssq7segeig4
> parent: igor@xxxxxxxxxxxx-20130331221855-1n7hagklb3soplxq
> committer: Alexey Botchkov <holyfoot@xxxxxxxxxxxx>
> branch nick: mdev-3917
> timestamp: Tue 2013-04-02 13:52:19 +0500
> message:
>   MDEV-3917 multiple use locks (GET_LOCK) in one connection.
>           The patch contributed by Konstantin Osipov applied.
>           Native comments:
>             Implement multiple user-level locks per connection.
>   
>             GET_LOCK() function in MySQL allows a connection  to hold at most
>             one user level lock. Taking a new lock automatically releases the
>             old lock, if any.
>   
>             The limit of one lock per session existed since  early versions
>             of MySQL didn't have a deadlock detector for SQL locks.
>             MDL patches in MySQL 5.5 added a deadlock detector,
>             so starting from 5.5 it became possible to take multiple locks
>             in any order -- a deadlock, should it occur, would be detected
>             and an error returned to the client which closed the wait chain.
>   
>             This is exactly what is done in this patch: ULLs are moved
>             to use MDL subsystem.
>   
>           also:
>             MDL_key::USER renamed to MDL::USER_LOCK

What does that mean?

>             boundary checks for user lock name added.

> === modified file 'sql/item_func.cc'
> --- a/sql/item_func.cc	2013-03-27 22:41:02 +0000
> +++ b/sql/item_func.cc	2013-04-02 08:52:19 +0000
> @@ -4010,7 +3896,121 @@ int Interruptible_wait::wait(mysql_cond_
>  
>  
>  /**
> -  Get a user level lock.  If the thread has an old lock this is first released.
> +  For locks with EXPLICIT duration, MDL returns a new ticket
> +  every time a lock is granted. This allows to implement recursive
> +  locks without extra allocation or additional data structures, such
> +  as below. However, if there are too many tickets in the same
> +  MDL_context, MDL_context::find_ticket() is getting too slow,
> +  since it's using a linear search.
> +  This is why a separate structure is allocated for a user
> +  level lock, and before requesting a new lock from MDL,
> +  GET_LOCK() checks thd->ull_hash if such lock is already granted,
> +  and if so, simply increments a reference counter.
> +*/
> +
> +class User_level_lock
> +{
> +public:
> +  MDL_ticket *lock;
> +  int refs;
> +};
> +
> +
> +/** Extract a hash key from User_level_lock. */
> +
> +uchar *ull_get_key(const uchar *ptr, size_t *length,
> +                   my_bool not_used __attribute__((unused)))
> +{
> +  User_level_lock *ull = (User_level_lock*) ptr;
> +  MDL_key *key = ull->lock->get_key();
> +  *length= key->length();
> +  return (uchar*) key->ptr();
> +}
> +
> +
> +/**
> +  Release all user level locks for this THD.
> +*/
> +
> +void mysql_ull_cleanup(THD *thd)
> +{
> +  User_level_lock *ull;
> +  DBUG_ENTER("mysql_ull_cleanup");
> +
> +  for (uint i= 0; i < thd->ull_hash.records; i++)
> +  {
> +    ull = (User_level_lock*) my_hash_element(&thd->ull_hash, i);
> +    thd->mdl_context.release_lock(ull->lock);
> +    free(ull);

my_free

> +  }
> +
> +  my_hash_free(&thd->ull_hash);
> +
> +  DBUG_VOID_RETURN;
> +}
> +
> +
> +/**
> +  Set explicit duration for metadata locks corresponding to
> +  user level locks to protect them from being released at the end
> +  of transaction.
> +*/
> +
> +void mysql_ull_set_explicit_lock_duration(THD *thd)
> +{
> +  User_level_lock *ull;
> +  DBUG_ENTER("mysql_ull_set_explicit_lock_duration");
> +
> +  for (uint i= 0; i < thd->ull_hash.records; i++)
> +  {
> +    ull= (User_level_lock*) my_hash_element(&thd->ull_hash, i);
> +    thd->mdl_context.set_lock_duration(ull->lock, MDL_EXPLICIT);
> +  }
> +  DBUG_VOID_RETURN;
> +}
> +
> +
> +/**
> +  When MDL detects a lock wait timeout, it pushes
> +  an error into the statement diagnostics area.
> +  For GET_LOCK(), lock wait timeout is not an error,
> +  but a special return value (0). NULL is returned in
> +  case of error.
> +  Capture and suppress lock wait timeout.
> +*/
> +
> +class Lock_wait_timeout_handler: public Internal_error_handler
> +{
> +public:
> +  Lock_wait_timeout_handler() :m_lock_wait_timeout(false) {}
> +
> +  bool m_lock_wait_timeout;
> +
> +  bool handle_condition(THD * /* thd */, uint sql_errno,
> +                        const char * /* sqlstate */,
> +                        MYSQL_ERROR::enum_warning_level /* level */,
> +                        const char *message,
> +                        MYSQL_ERROR ** /* cond_hdl */);
> +};
> +
> +bool
> +Lock_wait_timeout_handler::
> +handle_condition(THD * /* thd */, uint sql_errno,
> +                 const char * /* sqlstate */,
> +                 MYSQL_ERROR::enum_warning_level /* level */,
> +                 const char *message,
> +                 MYSQL_ERROR ** /* cond_hdl */)
> +{
> +  if (sql_errno == ER_LOCK_WAIT_TIMEOUT)
> +  {
> +    m_lock_wait_timeout= true;
> +    return true;                                /* condition handled */
> +  }
> +  return false;
> +}
> +
> +/**
> +  Get a user level lock.
>  
>    @retval
>      1    : Got lock
> @@ -4039,104 +4038,75 @@ longlong Item_func_get_lock::val_int()
>      it's not guaranteed to be same as on master.
>    */
>    if (thd->slave_thread)
> +  {
> +    null_value= 0;
>      DBUG_RETURN(1);
> +  }
>  
>    if (!res || !res->length())
> +    DBUG_RETURN(0);
> +
> +  if (res->length() > NAME_LEN)
>    {
> +    my_error(ER_TOO_LONG_IDENT, MYF(0), res->c_ptr_safe());
>      DBUG_RETURN(0);
>    }
> +
>    DBUG_PRINT("info", ("lock %.*s, thd=%ld", res->length(), res->ptr(),
>                        (long) thd->real_id));
> +  /* HASH entries are of type User_level_lock. */
> +  if (! my_hash_inited(&thd->ull_hash) &&
> +        my_hash_init(&thd->ull_hash, &my_charset_bin,
> +                     16 /* small hash */, 0, 0, ull_get_key, NULL, 0))
>    {
> +    DBUG_RETURN(0);
>    }
>  
> +  MDL_request ull_request;
> +  ull_request.init(MDL_key::USER_LOCK, res->c_ptr_safe(), "",
> +                   MDL_SHARED_NO_WRITE, MDL_EXPLICIT);
> +  MDL_key *ull_key = &ull_request.key;
>  
>  
> +  if ((ull= (User_level_lock*)
> +       my_hash_search(&thd->ull_hash, ull_key->ptr(), ull_key->length())))
> +  {
> +    /* Recursive lock */
> +    ull->refs++;
> +    null_value = 0;
> +    DBUG_RETURN(1);
> +  }
>  
> +  Lock_wait_timeout_handler lock_wait_timeout_handler;
> +  thd->push_internal_handler(&lock_wait_timeout_handler);
> +  bool error= thd->mdl_context.acquire_lock(&ull_request, timeout);
> +  (void) thd->pop_internal_handler();
> +  if (error)
>    {
> +    if (lock_wait_timeout_handler.m_lock_wait_timeout)
> +      null_value= 0;
> +    DBUG_RETURN(0);
>    }
>  
> +  ull= (User_level_lock*) malloc(sizeof(User_level_lock));

my_malloc() with MYF(MY_WME|MY_THREAD_SPECIFIC)

> +  if (ull == NULL)
>    {
> +    thd->mdl_context.release_lock(ull_request.ticket);
> +    DBUG_RETURN(0);
>    }
> +
> +  ull->lock= ull_request.ticket;
> +  ull->refs= 1;
> +
> +  if (my_hash_insert(&thd->ull_hash, (uchar*) ull))
>    {
> +    thd->mdl_context.release_lock(ull->lock);
> +    free(ull);

my_free

> +    DBUG_RETURN(0);
>    }
> +  null_value= 0;
>  
> +  DBUG_RETURN(1);
>  }
>  
>  
> @@ -4151,43 +4121,104 @@ longlong Item_func_get_lock::val_int()
>  longlong Item_func_release_lock::val_int()
>  {
>    DBUG_ASSERT(fixed == 1);
> +  String *res= args[0]->val_str(&value);
> +  THD *thd= current_thd;
>    DBUG_ENTER("Item_func_release_lock::val_int");
> +  null_value= 1;
> +
>    if (!res || !res->length())
> +    DBUG_RETURN(0);
> +
> +  if (res->length() > NAME_LEN)
>    {
> +    my_error(ER_TOO_LONG_IDENT, MYF(0), res->c_ptr_safe());
>      DBUG_RETURN(0);
>    }
> +
>    DBUG_PRINT("info", ("lock %.*s", res->length(), res->ptr()));
>  
> +  MDL_key ull_key;
> +  ull_key.mdl_key_init(MDL_key::USER_LOCK, res->c_ptr_safe(), "");
> +
> +  User_level_lock *ull;
> +
> +  if (!(ull=
> +        (User_level_lock*) my_hash_search(&thd->ull_hash,
> +                                          ull_key.ptr(), ull_key.length())))
>    {
> +    null_value= thd->mdl_context.get_lock_owner(&ull_key) == 0;
> +    DBUG_RETURN(0);
>    }
> +  null_value= 0;
> +  if (--ull->refs == 0)
>    {
> +    my_hash_delete(&thd->ull_hash, (uchar*) ull);
> +    thd->mdl_context.release_lock(ull->lock);
> +    free(ull);

my_free

>    }
> +  DBUG_RETURN(1);
> +}
> +
> +
> +/**
> +  Check a user level lock.
> +
> +  Sets null_value=TRUE on error.
> +
> +  @retval
> +    1           Available
> +  @retval
> +    0           Already taken, or error
> +*/
> +
> +longlong Item_func_is_free_lock::val_int()
> +{
> +  DBUG_ASSERT(fixed == 1);
> +  String *res= args[0]->val_str(&value);
> +  THD *thd= current_thd;
> +  null_value= 1;
> +

this block ----- from here
> +  if (!res || !res->length())
> +    return 0;
> +
> +  if (res->length() > NAME_LEN)
> +  {
> +    my_error(ER_TOO_LONG_IDENT, MYF(0), res->c_ptr_safe());
> +    return 0;
> +  }
and up ---- to here

is repeated four times. please move it to a separate function.
Like

   if (!ull_name_ok(res))
     return 0;

> +
> +  MDL_key ull_key;
> +  ull_key.mdl_key_init(MDL_key::USER_LOCK, res->c_ptr_safe(), "");
> +
> +  null_value= 0;
> +  return thd->mdl_context.get_lock_owner(&ull_key) == 0;
> +}
> +
> +
> +longlong Item_func_is_used_lock::val_int()
> +{
> +  DBUG_ASSERT(fixed == 1);
> +  String *res= args[0]->val_str(&value);
> +  THD *thd= current_thd;
> +  null_value= 1;
> +
> +  if (!res || !res->length())
> +    return 0;
> +
> +  if (res->length() > NAME_LEN)
> +  {
> +    my_error(ER_TOO_LONG_IDENT, MYF(0), res->c_ptr_safe());
> +    return 0;
> +  }
> +
> +  MDL_key ull_key;
> +  ull_key.mdl_key_init(MDL_key::USER_LOCK, res->c_ptr_safe(), "");
> +  ulong thread_id = thd->mdl_context.get_lock_owner(&ull_key);
> +  if (thread_id == 0)
> +    return 0;
> +
> +  null_value= 0;
> +  return thread_id;
>  }
>  
> === modified file 'sql/mdl.cc'
> --- a/sql/mdl.cc	2013-01-29 14:10:47 +0000
> +++ b/sql/mdl.cc	2013-04-02 08:52:19 +0000
> @@ -2094,8 +2151,6 @@ MDL_context::acquire_lock(MDL_request *m
>  
>    find_deadlock();
>  
> -  if (lock->needs_notification(ticket))
> -  {

okay, so before the patch only locks that needs_notification
were using the loop with short waits. Now all locks do.
Presumably to check whether thd_is_connected still.

Before the patch, how were client disconnects handled?
Like, one could have started to wait on the mdl lock, and then
disconnect. Was the wait aborted?

>      struct timespec abs_shortwait;
>      set_timespec(abs_shortwait, 1);
>      wait_status= MDL_wait::EMPTY;
> @@ -2106,17 +2163,25 @@ MDL_context::acquire_lock(MDL_request *m
>  
>        if (wait_status != MDL_wait::EMPTY)
>          break;
> +    /* Check if the client is gone while we were waiting. */
> +    if (! thd_is_connected(m_thd))
> +    {
> +      /*
> +       * The client is disconnected. Don't wait forever:
> +       * assume it's the same as a wait timeout, this
> +       * ensures all error handling is correct.
> +       */
> +      wait_status= MDL_wait::TIMEOUT;
> +      break;
> +    }
>  
>        mysql_prlock_wrlock(&lock->m_rwlock);
> +    if (lock->needs_notification(ticket))

why this if() is under the prlock, and not the other way around?

>        lock->notify_conflicting_locks(this);
>        mysql_prlock_unlock(&lock->m_rwlock);
>        set_timespec(abs_shortwait, 1);
>      }
>      if (wait_status == MDL_wait::EMPTY)
> -      wait_status= m_wait.timed_wait(m_thd, &abs_timeout, TRUE,
> -                                     mdl_request->key.get_wait_state_name());
> -  }
> -  else
>      wait_status= m_wait.timed_wait(m_thd, &abs_timeout, TRUE,
>                                     mdl_request->key.get_wait_state_name());

Regards,
Sergei