Re: [Maria-developers] metadata_lock_info plugin
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 <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
=) 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
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?
- 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
Fixed. If calling find_ticket() for getting LOCK_DURATION is costly, I will change it. Please review it. 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
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
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
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?
- 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? 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?
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
Would 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
Hi guys! i'm a bit far from mariadb source :( but let me help with some ideas 2013/11/7 kentoku <kentokushiba@gmail.com>
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 the locker for query_id is the same for thread id, not? they are in the same THD class variable at least
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?
like the processlist and KILL QUERY ID, the query id is the only way to know what query have the lock with the thread id we could have the same information, but the query id could change between the first and the second SELECT at information schema for example in other words query id is something to have the precise information about the query with that lock, and not the "thread id" with that lock and the wrong "query id" that's the same point solved by the KILL QUERY ID command
- 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?
i didn't read the patch yet, but duration is a nice information to check what the DBA should do with some problem at slow queries or blocks for example a lock with more than 60 seconds could be killed, and less than 60 not... just to help DBA and to get nice numbers to developers select how many time he should consider in a MDL (statistic information)
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?
i didn't checked that :( sorry maybe just with debug version this could work but let's think about who will use mdl plugin.... i think that MDL plugin is just to developers and DBA that need more information, in this case they probably will use the debug version, right?!
Thanks, Kentoku
thanks guys! =) -- Roberto Spadim SPAEmpresarial
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
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
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
I think protection 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
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
> 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
better private. the the 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
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 <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
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
I think protection 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
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
>> 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
better private. the the 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
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.
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. 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
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
I think protection 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
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
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 <svoj@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 <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
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 <svoj@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
be
query_id when lock was taken. Why do you need query_id in this
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
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
> > 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
wrote: 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
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); > > > > > > 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
> > > > { > > > > 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
no
> protection > > > >> of ticket lists: lists may be updated while we iterate
should table? this them this like: metadata_lock_info_lock_mode_str[]= there is them
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
and 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 >
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 <svoj@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 <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
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 <svoj@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
be
> query_id when lock was taken. Why do you need query_id in this
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
> 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 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
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); > > > > > > > > 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
> > > > > { > > > > > 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
no
> > protection > > > > >> of ticket lists: lists may be updated while we iterate
should table? this them this like: metadata_lock_info_lock_mode_str[]= there is them
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
and 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 > >
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 <svoj@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 <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
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 <svoj@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
be
> > query_id when lock was taken. Why do you need query_id in this
> 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
> > 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 > 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
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); > > > > > > > > > > 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
> > > > > > { > > > > > > 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
no
> > > protection > > > > > >> of ticket lists: lists may be updated while we iterate
should table? this them this like: metadata_lock_info_lock_mode_str[]= there is them
> 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
and 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 > > > >
_______________________________________________ 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
Thanks a lot! 2013/12/13 Sergey Vojtovich <svoj@mariadb.org>
FYI: metadata_lock_info plugin is pushed to 10.0.7.
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 <svoj@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 <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
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 <svoj@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
table?
> > You're right. Probably query_id is not applicable for this
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
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 > > 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
no > > > > protection > > > > > > > of ticket lists: lists may be updated while we iterate
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); > > > > > > > > > > > > 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
is them this like: 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
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
no > > > > protection > > > > > > >> of ticket lists: lists may be updated while we iterate
rest there is 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 <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
On Tue, Dec 10, 2013 at 07:58:34PM +0400, Sergey Vojtovich wrote: this table. think there the 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
participants (3)
-
kentoku -
Roberto Spadim -
Sergey Vojtovich