Hi Kentoku, On Tue, Dec 10, 2013 at 11:38:15PM +0900, kentoku wrote:
Hi Sergey,
looks great now. Just one minor thing: rev.3914 overwrites mdl.h with DOS line endings (CRLF), which are not permitted by coding guidelines.
Sorry. No problems.
If you fix this file, I can merge your revision as is. Or I can fix it myself, but I'll have to submit metadata_lock_info on my behalf.
I just fixed and pushed. Please start the merge process.
This way we'll loose all history of mdl.h modifications. In other words bzr annotate says all lines were added revision 3915. I meant we either need clean revision or I can pick these changes myself. One of the ways to make clean revision is (!!! assuming nobody else is using this tree): (on rev.3914) bzr uncommit edit mdl.h bzr commit bzr push --overwrite lp:~kentokushiba/maria/10.0.6-metadata_lock_info Regards, Sergey
Thanks, Kentoku
2013/12/10 Sergey Vojtovich <svoj@mariadb.org>
Hi Kentoku,
looks great now. Just one minor thing: rev.3914 overwrites mdl.h with DOS line endings (CRLF), which are not permitted by coding guidelines.
If you fix this file, I can merge your revision as is. Or I can fix it myself, but I'll have to submit metadata_lock_info on my behalf.
When the above is solved, I will start the merge process.
Thanks, Sergey
Hi Sergey,
I just pushed again. Please review it. lp:~kentokushiba/maria/10.0.6-metadata_lock_info
Thanks, Kentoku
2013/11/8 Sergey Vojtovich <svoj@mariadb.org>
Hi Kentoku,
Hi Sergey,
please find iterator prototype attached. I didn't test it. Thanks a lot. I'll try it.
> > - 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.
I think it is owner thread's current query_id. This query_id should be query_id when lock was taken. Why do you need query_id in this table? You're right. Probably query_id is not applicable for this table. I was
On Fri, Nov 08, 2013 at 04:18:51AM +0900, kentoku wrote: thinking about possibility to kill stale queries by query_id, like in https://mariadb.atlassian.net/browse/MDEV-4911
Anyway it is not a requirement, one can join I_S.PROCESSLIST to get 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?
On debug build, MDL_ticket class has enum_mdl_duration. I think this enum_mdl_duration can use for non debug build, if remove "#ifndef DBUG_OFF" for enum_mdl_duration. How do you think about it? It could be an option, but I'd go with find_ticket() for now.
Regards, Sergey
Thanks, Kentoku
2013/11/7 Sergey Vojtovich <svoj@mariadb.org>
Kentoku,
please find iterator prototype attached. I didn't test it.
Regards, Sergey
On Thu, Nov 07, 2013 at 03:18:04PM +0400, Sergey Vojtovich wrote: > Hi Kentoku, > > thanks for implementing these requests. Technically it looks much
now. > Though I believe it is a good idea to keep mdl private classes
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
better private. 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 <roberto@spadim.com.br> > > > > > =) no problem about disagree, i'm agree now hehe > > > thanks guys! :) > > > > > > > > > 2013/10/29 Sergey Vojtovich <svoj@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
context. As > > >> such > > >> it is not protected by any locks. > > >> > > >> I believe we should use global lists instead, something
> > >> 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
On Mon, Dec 09, 2013 at 04:08:48AM +0900, kentoku wrote: this like: 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 <kentokushiba@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