maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #06429
Re: 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
Fixed. If calling find_ticket() for getting LOCK_DURATION is costly, I will
change it. Please review it.
> - Roberto suggests to use m_namespace_to_wait_state_name instead of
> metadata_lock_info_lock_names, but I tend to disagree because the
former says
> "Waiting for...". Though I'd suggest to make array of LEX_STRING's to
avoid
> calling strdup().
Fixed.
> - Roberto suggests to replace DB with KEY and NAME with SUB_KEY, but I
tend to
> disagree since in most cases it is db and table name. I'd vote for
> TABLE_SCHEMA and TABLE_NAME to stay in line with the rest I_S tables.
Fixed.
Thanks,
Kentoku
2013/10/30 Roberto Spadim <roberto@xxxxxxxxxxxxx>
> =) no problem about disagree, i'm agree now hehe
> thanks guys! :)
>
>
> 2013/10/29 Sergey Vojtovich <svoj@xxxxxxxxxxx>
>
>> Hi Kentoku,
>>
>> thanks for your contribution and sorry for this long delay. I finally
>> managed
>> to go through your code.
>>
>> The most important thing I'm worried about is that there is no protection
>> of ticket lists: lists may be updated while we iterate them and it may
>> cause
>> server crash in concurrent environment. From what I can see MDL_context is
>> mostly supposed to be accessed only by thread which owns this context. As
>> such
>> it is not protected by any locks.
>>
>> I believe we should use global lists instead, something like:
>> mysql_mutex_lock(&mdl_locks.m_mutex);
>> iterate mdl_locks.m_locks (hash of MDL_lock)
>> {
>> mysql_prlock_rdlock(&lock->m_rwlock);
>> iterate lock->m_granted (list of MDL_ticket)
>> {
>> // store data
>> }
>> mysql_prlock_unlock(&lock->m_rwlock);
>> }
>> mysql_mutex_lock(&mdl_locks.m_mutex);
>>
>> A few more suggestions in no particular order:
>> - instead of making m_tickets public I'd suggest to try to make
>> i_s_metadata_lock_info_fill_table() a friend function
>> (shouldn't be relevant anymore)
>>
>> - why check for thd->killed?
>> (shouldn't be relevant anymore)
>>
>> - metadata_lock_info_lock_mode_length[] includes ending zero, and then
>> we store e.g. "MDL_SHARED\0" whereas we should store "MDL_SHARED"
>>
>> I'd suggest to declare these arrays as following:
>> static const LEX_STRING metadata_lock_info_lock_mode_str[]=
>> {
>> STRING_WITH_LEN("MDL_INTENTION_EXCLUSIVE"), // or C_STRING_WITH_LEN()
>> ...
>> };
>>
>> - Roberto suggests to rename ID to THREAD_ID and I agree
>> - Roberto suggests to add QUERY_ID column and I agree
>> - Roberto suggests to add LOCK_DURATION column and I agree
>> - Roberto suggests to use m_namespace_to_wait_state_name instead of
>> metadata_lock_info_lock_names, but I tend to disagree because the
>> former says
>> "Waiting for...". Though I'd suggest to make array of LEX_STRING's to
>> avoid
>> calling strdup().
>>
>> - Roberto suggests to replace DB with KEY and NAME with SUB_KEY, but I
>> tend to
>> disagree since in most cases it is db and table name. I'd vote for
>> TABLE_SCHEMA and TABLE_NAME to stay in line with the rest I_S tables.
>>
>> Regards,
>> Sergey
>>
>> On Mon, Jul 01, 2013 at 03:46:22AM +0900, kentoku wrote:
>> > Hi Sergey,
>> > Sorry. I think this plugin needs more locks "mdl_locks.m_mutex",
>> > "mdl_locks.m_global_lock" and "mdl_locks.m_commit_lock". Is it right?
>> Would
>> > you please teach me how can I use it?
>> > Thanks,
>> > Kentoku
>> >
>> >
>> > 2013/7/1 kentoku <kentokushiba@xxxxxxxxx>
>> >
>> > > 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@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~maria-developers
>> More help : https://help.launchpad.net/ListHelp
>>
>
>
>
> --
> Roberto Spadim
> SPAEmpresarial
>
Follow ups
References