Re: [Maria-developers] metadata_lock_info plugin

newer
[Maria-developers] MariaDB r3955...

older
Re: [Maria-developers] Rev 3915:...

Sergey Vojtovich

29 Oct 2013 29 Oct '13

12:29 p.m.

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

...
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

Show replies by date

Roberto Spadim

29 Oct 29 Oct

11:11 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

=) no problem about disagree, i'm agree now hehe thanks guys! :) 2013/10/29 Sergey Vojtovich

...

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

...
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

31 Oct 31 Oct

9:28 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...

=) no problem about disagree, i'm agree now hehe thanks guys! :)

2013/10/29 Sergey Vojtovich

...
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

...
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

Sergey Vojtovich

7 Nov 7 Nov

12:18 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...
=) no problem about disagree, i'm agree now hehe thanks guys! :)

2013/10/29 Sergey Vojtovich

...
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

...
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

Sergey Vojtovich

2:35 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...
=) no problem about disagree, i'm agree now hehe thanks guys! :)

2013/10/29 Sergey Vojtovich

...
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

...
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

kentoku

8:18 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...

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

...
=) no problem about disagree, i'm agree now hehe thanks guys! :)

2013/10/29 Sergey Vojtovich

...
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

> 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

Roberto Spadim

8 Nov 8 Nov

1:07 a.m.

New subject: [Maria-developers] metadata_lock_info plugin

Hi guys! i'm a bit far from mariadb source :( but let me help with some ideas 2013/11/7 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 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

Sergey Vojtovich

7:36 a.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...
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

...
=) no problem about disagree, i'm agree now hehe thanks guys! :)

2013/10/29 Sergey Vojtovich

...
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 > > > 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

kentoku

8 Dec 8 Dec

8:08 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...

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

...
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

...
=) no problem about disagree, i'm agree now hehe thanks guys! :)

2013/10/29 Sergey Vojtovich

> 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 > > > > > 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

Sergey Vojtovich

10 Dec 10 Dec

11:07 a.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...
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

...
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

> =) no problem about disagree, i'm agree now hehe > thanks guys! :) > > > 2013/10/29 Sergey Vojtovich > >> 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 >> > >> > > 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

kentoku

3:38 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...

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

...
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

...
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 > > > =) no problem about disagree, i'm agree now hehe > > thanks guys! :) > > > > > > 2013/10/29 Sergey Vojtovich > > > >> 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 > >> > > >> > > 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

Sergey Vojtovich

3:51 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...
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

...
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

...
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 > > > > > =) no problem about disagree, i'm agree now hehe > > > thanks guys! :) > > > > > > > > > 2013/10/29 Sergey Vojtovich > > > > > >> 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 > > >> > > > >> > > 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

kentoku

4:39 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...

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

...
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

...
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

> 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 > > > > > > > =) no problem about disagree, i'm agree now hehe > > > > thanks guys! :) > > > > > > > > > > > > 2013/10/29 Sergey Vojtovich > > > > > > > >> 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 > > > >> > > > > >> > > 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 >

Sergey Vojtovich

4:58 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...
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

...
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

...
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 > > > 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 > > > > > > > > > =) no problem about disagree, i'm agree now hehe > > > > > thanks guys! :) > > > > > > > > > > > > > > > 2013/10/29 Sergey Vojtovich > > > > > > > > > >> 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 > > > > >> > > > > > >> > > 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 > >

Sergey Vojtovich

13 Dec 13 Dec

9:54 a.m.

New subject: [Maria-developers] metadata_lock_info plugin

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

...
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

...
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

> 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 > > > > > 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 > > > > > > > > > > > =) no problem about disagree, i'm agree now hehe > > > > > > thanks guys! :) > > > > > > > > > > > > > > > > > > 2013/10/29 Sergey Vojtovich > > > > > > > > > > > >> 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 > > > > > >> > > > > > > >> > > 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

kentoku

4:04 p.m.

New subject: [Maria-developers] metadata_lock_info plugin

Thanks a lot! 2013/12/13 Sergey Vojtovich

...

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

...
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

...
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 > > > 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 > > > > > > > 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 > > > > > > > > > > > > > =) no problem about disagree, i'm agree now hehe > > > > > > > thanks guys! :) > > > > > > > > > > > > > > > > > > > > > 2013/10/29 Sergey Vojtovich > > > > > > > > > > > > > >> 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 > > > > > > >> > > > > > > > >> > > 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

3879

Age (days ago)

3924

Last active (days ago)

List overview

Download

15 comments

3 participants

participants (3)

kentoku
Roberto Spadim
Sergey Vojtovich