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 <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 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
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
On Mon, Jul 01, 2013 at 03:46:22AM +0900, kentoku wrote: 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