Re: [Maria-developers] metadata_lock_info plugin

13 Dec 2013


      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