maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Kentoku,

thanks for implementing these requests. Technically it looks much better now.
Though I believe it is a good idea to keep mdl private classes private. I think
we should add logics of i_s_metadata_lock_info_fill_table() to mdl.cc. I'll do
that and get back to you when ready.

More comments inline.

On Fri, Nov 01, 2013 at 05:28:32AM +0900, kentoku wrote:
> Hi Sergey and Roberto,
> 
> Thank you for reviewing and suggesting!
> Currently, branch is the following.
> lp:~kentokushiba/maria/10.0.4-metadata_lock_info
> 
> > The most important thing I'm worried about is that there is no protection
> > of ticket lists: lists may be updated while we iterate them and it may
> cause
> > server crash in concurrent environment. From what I can see MDL_context is
> > mostly supposed to be accessed only by thread which owns this context. As
> such
> > it is not protected by any locks.
> >
> > I believe we should use global lists instead, something like:
> >   mysql_mutex_lock(&mdl_locks.m_mutex);
> >   iterate mdl_locks.m_locks (hash of MDL_lock)
> >   {
> >     mysql_prlock_rdlock(&lock->m_rwlock);
> >     iterate lock->m_granted (list of MDL_ticket)
> >     {
> >       // store data
> >     }
> >     mysql_prlock_unlock(&lock->m_rwlock);
> >   }
> >   mysql_mutex_lock(&mdl_locks.m_mutex);
> 
> Fixed.
> 
> > A few more suggestions in no particular order:
> > - instead of making m_tickets public I'd suggest to try to make
> >   i_s_metadata_lock_info_fill_table() a friend function
> >   (shouldn't be relevant anymore)
> 
> Fixed.
> 
> > - why check for thd->killed?
> >   (shouldn't be relevant anymore)
> 
> Fixed.
> 
> > - metadata_lock_info_lock_mode_length[] includes ending zero, and then
> >   we store e.g. "MDL_SHARED\0" whereas we should store "MDL_SHARED"
> >
> >   I'd suggest to declare these arrays as following:
> >   static const LEX_STRING metadata_lock_info_lock_mode_str[]=
> >   {
> >     STRING_WITH_LEN("MDL_INTENTION_EXCLUSIVE"), // or C_STRING_WITH_LEN()
> >     ...
> >   };
> 
> Fixed.
> 
> > - Roberto suggests to rename ID to THREAD_ID and I agree
> 
> Fixed.
> 
> > - Roberto suggests to add QUERY_ID column and I agree
> 
> Not fixed. Where can I get locker's QUERY_ID?
It is mdl_ctx->get_owner()->get_thd()->query_id.

> 
> > - Roberto suggests to add LOCK_DURATION column and I agree
> 
> Fixed. If calling find_ticket() for getting LOCK_DURATION is costly, I will
> change it. Please review it.
It's a bit heavy indeed. What is the other option you're considering?

Regards,
Sergey

> 
> > - Roberto suggests to use m_namespace_to_wait_state_name instead of
> >   metadata_lock_info_lock_names, but I tend to disagree because the
> former says
> >   "Waiting for...". Though I'd suggest to make array of LEX_STRING's to
> avoid
> >   calling strdup().
> 
> Fixed.
> 
> > - Roberto suggests to replace DB with KEY and NAME with SUB_KEY, but I
> tend to
> >   disagree since in most cases it is db and table name. I'd vote for
> >   TABLE_SCHEMA and TABLE_NAME to stay in line with the rest I_S tables.
> 
> Fixed.
> 
> Thanks,
> Kentoku
> 
> 
> 2013/10/30 Roberto Spadim <roberto@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
> >