maria-developers team mailing list archive

[kentoku <kentokushiba@xxxxxxxxx>]

Hi Sergey,

> please find iterator prototype attached. I didn't test it.
Thanks a lot. I'll try it.

> > > - Roberto suggests to add QUERY_ID column and I agree
> >
> > Not fixed. Where can I get locker's QUERY_ID?
> It is mdl_ctx->get_owner()->get_thd()->query_id.

I think it is owner thread's current query_id. This query_id should be
query_id when lock was taken. Why do you need query_id in this table?

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

On debug build, MDL_ticket class has enum_mdl_duration. I think this
enum_mdl_duration can use for non debug build, if remove "#ifndef DBUG_OFF"
for enum_mdl_duration. How do you think about it?

Thanks,
Kentoku



2013/11/7 Sergey Vojtovich <svoj@xxxxxxxxxxx>

> Kentoku,
>
> please find iterator prototype attached. I didn't test it.
>
> Regards,
> Sergey
>
> On Thu, Nov 07, 2013 at 03:18:04PM +0400, Sergey Vojtovich wrote:
> > Hi Kentoku,
> >
> > thanks for implementing these requests. Technically it looks much better
> now.
> > Though I believe it is a good idea to keep mdl private classes private.
> I think
> > we should add logics of i_s_metadata_lock_info_fill_table() to mdl.cc.
> I'll do
> > that and get back to you when ready.
> >
> > More comments inline.
> >
> > On Fri, Nov 01, 2013 at 05:28:32AM +0900, kentoku wrote:
> > > Hi Sergey and Roberto,
> > >
> > > Thank you for reviewing and suggesting!
> > > Currently, branch is the following.
> > > lp:~kentokushiba/maria/10.0.4-metadata_lock_info
> > >
> > > > The most important thing I'm worried about is that there is no
> protection
> > > > of ticket lists: lists may be updated while we iterate them and it
> may
> > > cause
> > > > server crash in concurrent environment. From what I can see
> MDL_context is
> > > > mostly supposed to be accessed only by thread which owns this
> context. As
> > > such
> > > > it is not protected by any locks.
> > > >
> > > > I believe we should use global lists instead, something like:
> > > >   mysql_mutex_lock(&mdl_locks.m_mutex);
> > > >   iterate mdl_locks.m_locks (hash of MDL_lock)
> > > >   {
> > > >     mysql_prlock_rdlock(&lock->m_rwlock);
> > > >     iterate lock->m_granted (list of MDL_ticket)
> > > >     {
> > > >       // store data
> > > >     }
> > > >     mysql_prlock_unlock(&lock->m_rwlock);
> > > >   }
> > > >   mysql_mutex_lock(&mdl_locks.m_mutex);
> > >
> > > Fixed.
> > >
> > > > A few more suggestions in no particular order:
> > > > - instead of making m_tickets public I'd suggest to try to make
> > > >   i_s_metadata_lock_info_fill_table() a friend function
> > > >   (shouldn't be relevant anymore)
> > >
> > > Fixed.
> > >
> > > > - why check for thd->killed?
> > > >   (shouldn't be relevant anymore)
> > >
> > > Fixed.
> > >
> > > > - metadata_lock_info_lock_mode_length[] includes ending zero, and
> then
> > > >   we store e.g. "MDL_SHARED\0" whereas we should store "MDL_SHARED"
> > > >
> > > >   I'd suggest to declare these arrays as following:
> > > >   static const LEX_STRING metadata_lock_info_lock_mode_str[]=
> > > >   {
> > > >     STRING_WITH_LEN("MDL_INTENTION_EXCLUSIVE"), // or
> C_STRING_WITH_LEN()
> > > >     ...
> > > >   };
> > >
> > > Fixed.
> > >
> > > > - Roberto suggests to rename ID to THREAD_ID and I agree
> > >
> > > Fixed.
> > >
> > > > - Roberto suggests to add QUERY_ID column and I agree
> > >
> > > Not fixed. Where can I get locker's QUERY_ID?
> > It is mdl_ctx->get_owner()->get_thd()->query_id.
> >
> > >
> > > > - Roberto suggests to add LOCK_DURATION column and I agree
> > >
> > > Fixed. If calling find_ticket() for getting LOCK_DURATION is costly, I
> will
> > > change it. Please review it.
> > It's a bit heavy indeed. What is the other option you're considering?
> >
> > Regards,
> > Sergey
> >
> > >
> > > > - Roberto suggests to use m_namespace_to_wait_state_name instead of
> > > >   metadata_lock_info_lock_names, but I tend to disagree because the
> > > former says
> > > >   "Waiting for...". Though I'd suggest to make array of LEX_STRING's
> to
> > > avoid
> > > >   calling strdup().
> > >
> > > Fixed.
> > >
> > > > - Roberto suggests to replace DB with KEY and NAME with SUB_KEY, but
> I
> > > tend to
> > > >   disagree since in most cases it is db and table name. I'd vote for
> > > >   TABLE_SCHEMA and TABLE_NAME to stay in line with the rest I_S
> tables.
> > >
> > > Fixed.
> > >
> > > Thanks,
> > > Kentoku
> > >
> > >
> > > 2013/10/30 Roberto Spadim <roberto@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
> > > >
> >
> > _______________________________________________
> > 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
>