Hi, Holyfoot! (Kostja - there're a couple of questions at the end that you might want to reply to)
On Apr 02, [email protected] wrote: > At file:///home/hf/wmar/mdev-3917/ > > ------------------------------------------------------------ > revno: 3510 > revision-id: [email protected] > parent: [email protected] > committer: Alexey Botchkov <[email protected]> > 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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
