Thanks a lot!
2013/12/13 Sergey Vojtovich <s...@mariadb.org> > FYI: metadata_lock_info plugin is pushed to 10.0.7. > > On Tue, Dec 10, 2013 at 07:58:34PM +0400, Sergey Vojtovich wrote: > > Hi Kentoku, > > > > yes, thanks, it worked! I now have revision which is good for merge. > > > > Thanks, > > Sergey > > > > On Wed, Dec 11, 2013 at 12:39:59AM +0900, kentoku wrote: > > > Hi Sergey, > > > > > > > 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 > > > > > > Thank you for your advice. I just overwrote. Is it OK? > > > > > > Thanks, > > > Kentoku > > > > > > > > > > > > 2013/12/10 Sergey Vojtovich <s...@mariadb.org> > > > > > > > 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 <s...@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 > > > > > > > > > > > > On Mon, Dec 09, 2013 at 04:08:48AM +0900, kentoku wrote: > > > > > > > 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 <s...@mariadb.org> > > > > > > > > > > > > > > > Hi Kentoku, > > > > > > > > > > > > > > > > On Fri, Nov 08, 2013 at 04:18:51AM +0900, kentoku wrote: > > > > > > > > > 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 > > > > > > > > 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 <s...@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 > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > _______________________________________________ > > 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 >
_______________________________________________ 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