Hi, Holyfoot! (Kostja - there're a couple of questions at the end that you might want to reply to) On Apr 02, holyfoot@askmonty.org wrote:

At file:///home/hf/wmar/mdev-3917/

------------------------------------------------------------ revno: 3510 revision-id: holyfoot@askmonty.org-20130402085219-1sxuzssq7segeig4 parent: igor@askmonty.org-20130331221855-1n7hagklb3soplxq committer: Alexey Botchkov <holyfoot@askmonty.org> 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