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 <svoj@mariadb.org>
Kentoku,
please find iterator prototype attached. I didn't test it.
Regards, Sergey
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
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
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
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
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
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
On Thu, Nov 07, 2013 at 03:18:04PM +0400, Sergey Vojtovich wrote: protection then tables. protection then 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