Hi Kentoku, thanks for implementing these requests. Technically it looks much better now. Though I believe it is a good idea to keep mdl private classes private. I think we should add logics of i_s_metadata_lock_info_fill_table() to mdl.cc. I'll do that and get back to you when ready.
More comments inline. On Fri, Nov 01, 2013 at 05:28:32AM +0900, kentoku wrote: > Hi Sergey and Roberto, > > Thank you for reviewing and suggesting! > Currently, branch is the following. > lp:~kentokushiba/maria/10.0.4-metadata_lock_info > > > The most important thing I'm worried about is that there is no protection > > of ticket lists: lists may be updated while we iterate them and it may > cause > > server crash in concurrent environment. From what I can see MDL_context is > > mostly supposed to be accessed only by thread which owns this context. As > such > > it is not protected by any locks. > > > > I believe we should use global lists instead, something like: > > mysql_mutex_lock(&mdl_locks.m_mutex); > > iterate mdl_locks.m_locks (hash of MDL_lock) > > { > > mysql_prlock_rdlock(&lock->m_rwlock); > > iterate lock->m_granted (list of MDL_ticket) > > { > > // store data > > } > > mysql_prlock_unlock(&lock->m_rwlock); > > } > > mysql_mutex_lock(&mdl_locks.m_mutex); > > Fixed. > > > A few more suggestions in no particular order: > > - instead of making m_tickets public I'd suggest to try to make > > i_s_metadata_lock_info_fill_table() a friend function > > (shouldn't be relevant anymore) > > Fixed. > > > - why check for thd->killed? > > (shouldn't be relevant anymore) > > Fixed. > > > - metadata_lock_info_lock_mode_length[] includes ending zero, and then > > we store e.g. "MDL_SHARED\0" whereas we should store "MDL_SHARED" > > > > I'd suggest to declare these arrays as following: > > static const LEX_STRING metadata_lock_info_lock_mode_str[]= > > { > > STRING_WITH_LEN("MDL_INTENTION_EXCLUSIVE"), // or C_STRING_WITH_LEN() > > ... > > }; > > Fixed. > > > - Roberto suggests to rename ID to THREAD_ID and I agree > > Fixed. > > > - Roberto suggests to add QUERY_ID column and I agree > > Not fixed. Where can I get locker's QUERY_ID? It is mdl_ctx->get_owner()->get_thd()->query_id. > > > - Roberto suggests to add LOCK_DURATION column and I agree > > Fixed. If calling find_ticket() for getting LOCK_DURATION is costly, I will > change it. Please review it. It's a bit heavy indeed. What is the other option you're considering? Regards, Sergey > > > - Roberto suggests to use m_namespace_to_wait_state_name instead of > > metadata_lock_info_lock_names, but I tend to disagree because the > former says > > "Waiting for...". Though I'd suggest to make array of LEX_STRING's to > avoid > > calling strdup(). > > Fixed. > > > - Roberto suggests to replace DB with KEY and NAME with SUB_KEY, but I > tend to > > disagree since in most cases it is db and table name. I'd vote for > > TABLE_SCHEMA and TABLE_NAME to stay in line with the rest I_S tables. > > Fixed. > > Thanks, > Kentoku > > > 2013/10/30 Roberto Spadim <robe...@spadim.com.br> > > > =) no problem about disagree, i'm agree now hehe > > thanks guys! :) > > > > > > 2013/10/29 Sergey Vojtovich <s...@mariadb.org> > > > >> Hi Kentoku, > >> > >> thanks for your contribution and sorry for this long delay. I finally > >> managed > >> to go through your code. > >> > >> The most important thing I'm worried about is that there is no protection > >> of ticket lists: lists may be updated while we iterate them and it may > >> cause > >> server crash in concurrent environment. From what I can see MDL_context is > >> mostly supposed to be accessed only by thread which owns this context. As > >> such > >> it is not protected by any locks. > >> > >> I believe we should use global lists instead, something like: > >> mysql_mutex_lock(&mdl_locks.m_mutex); > >> iterate mdl_locks.m_locks (hash of MDL_lock) > >> { > >> mysql_prlock_rdlock(&lock->m_rwlock); > >> iterate lock->m_granted (list of MDL_ticket) > >> { > >> // store data > >> } > >> mysql_prlock_unlock(&lock->m_rwlock); > >> } > >> mysql_mutex_lock(&mdl_locks.m_mutex); > >> > >> A few more suggestions in no particular order: > >> - instead of making m_tickets public I'd suggest to try to make > >> i_s_metadata_lock_info_fill_table() a friend function > >> (shouldn't be relevant anymore) > >> > >> - why check for thd->killed? > >> (shouldn't be relevant anymore) > >> > >> - metadata_lock_info_lock_mode_length[] includes ending zero, and then > >> we store e.g. "MDL_SHARED\0" whereas we should store "MDL_SHARED" > >> > >> I'd suggest to declare these arrays as following: > >> static const LEX_STRING metadata_lock_info_lock_mode_str[]= > >> { > >> STRING_WITH_LEN("MDL_INTENTION_EXCLUSIVE"), // or C_STRING_WITH_LEN() > >> ... > >> }; > >> > >> - Roberto suggests to rename ID to THREAD_ID and I agree > >> - Roberto suggests to add QUERY_ID column and I agree > >> - Roberto suggests to add LOCK_DURATION column and I agree > >> - Roberto suggests to use m_namespace_to_wait_state_name instead of > >> metadata_lock_info_lock_names, but I tend to disagree because the > >> former says > >> "Waiting for...". Though I'd suggest to make array of LEX_STRING's to > >> avoid > >> calling strdup(). > >> > >> - Roberto suggests to replace DB with KEY and NAME with SUB_KEY, but I > >> tend to > >> disagree since in most cases it is db and table name. I'd vote for > >> TABLE_SCHEMA and TABLE_NAME to stay in line with the rest I_S tables. > >> > >> Regards, > >> Sergey > >> > >> On Mon, Jul 01, 2013 at 03:46:22AM +0900, kentoku wrote: > >> > Hi Sergey, > >> > Sorry. I think this plugin needs more locks "mdl_locks.m_mutex", > >> > "mdl_locks.m_global_lock" and "mdl_locks.m_commit_lock". Is it right? > >> Would > >> > you please teach me how can I use it? > >> > Thanks, > >> > Kentoku > >> > > >> > > >> > 2013/7/1 kentoku <kentokush...@gmail.com> > >> > > >> > > Hi Sergey, > >> > > > >> > > I made new information schema plugin which named "metadata_lock_info" > >> > > plugin. This plugin makes it possible knowing "who has metadata > >> locks". In > >> > > development stage, sometimes DBAs meet metadata locks when using alter > >> > > table statement. This metadata locks are sometimes caused by GUI > >> tools. In > >> > > service stage, sometimes DBAs meet metadata locks when using alter > >> table > >> > > statement. This metadata locks are sometimes caused by long batch > >> > > processing. In many cases, the DBAs need to judge immediately. So I > >> made it > >> > > for all DBAs. > >> > > Would you please review this plugin? > >> > > lp:~kentokushiba/maria/10.0.3-metadata_lock_info > >> > > Note: This plugin needs a change of sql/mdl.h > >> > > > >> > > Thanks, > >> > > Kentoku > >> > > > >> > >> _______________________________________________ > >> Mailing list: https://launchpad.net/~maria-developers > >> Post to : maria-developers@lists.launchpad.net > >> Unsubscribe : https://launchpad.net/~maria-developers > >> More help : https://help.launchpad.net/ListHelp > >> > > > > > > > > -- > > Roberto Spadim > > SPAEmpresarial > > _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp