maria-developers team mailing list archive

[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

=== modified file 'sql/mdl.cc'
--- sql/mdl.cc	2013-07-21 14:39:19 +0000
+++ sql/mdl.cc	2013-11-07 13:31:38 +0000
@@ -158,6 +158,7 @@ class MDL_map_partition
                    I_P_List_counter>
           Lock_cache;
   Lock_cache m_unused_locks_cache;
+  friend int mdl_iterate(int (*)(MDL_ticket *, void *), void *);
 };
 
 
@@ -187,6 +188,7 @@ class MDL_map
   MDL_lock *m_global_lock;
   /** Pre-allocated MDL_lock object for COMMIT namespace. */
   MDL_lock *m_commit_lock;
+  friend int mdl_iterate(int (*)(MDL_ticket *, void *), void *);
 };
 
 
@@ -706,6 +708,46 @@ void mdl_destroy()
 }
 
 
+static inline int mdl_iterate_lock(MDL_lock *lock,
+                                   int (*callback)(MDL_ticket *ticket, void *arg),
+                                   void *arg)
+{
+  MDL_lock::Ticket_iterator ticket_it(lock->m_granted);
+  MDL_ticket *ticket;
+  int res= 0;
+  mysql_prlock_rdlock(&lock->m_rwlock);
+  while ((ticket= ticket_it++) && !(res= callback(ticket, arg))) /* no-op */;
+  mysql_prlock_unlock(&lock->m_rwlock);
+  return res;
+}
+
+
+int mdl_iterate(int (*callback)(MDL_ticket *ticket, void *arg), void *arg)
+{
+  uint i, j;
+  int res;
+  DBUG_ENTER("mdl_iterate");
+
+  if ((res= mdl_iterate_lock(mdl_locks.m_global_lock, callback, arg)) ||
+      (res= mdl_iterate_lock(mdl_locks.m_commit_lock, callback, arg)))
+    DBUG_RETURN(res);
+
+  for (i= 0; i < mdl_locks.m_partitions.elements(); i++)
+  {
+    MDL_map_partition *part= mdl_locks.m_partitions.at(i);
+    mysql_mutex_lock(&part->m_mutex);
+    for (j= 0; j < part->m_locks.records; j++)
+    {
+      if ((res= mdl_iterate_lock((MDL_lock*) my_hash_element(&part->m_locks, j),
+                                 callback, arg)))
+        break;
+    }
+    mysql_mutex_unlock(&part->m_mutex);
+  }
+  DBUG_RETURN(res);
+}
+
+
 /** Initialize the container for all MDL locks. */
 
 void MDL_map::init()

=== modified file 'sql/mdl.h'
--- sql/mdl.h	2013-07-21 14:39:19 +0000
+++ sql/mdl.h	2013-11-07 13:26:07 +0000
@@ -988,4 +988,7 @@ static const ulong MDL_LOCKS_HASH_PARTIT
   to avoid starving out weak, low-prio locks.
 */
 extern "C" ulong max_write_lock_count;
+
+extern MYSQL_PLUGIN_IMPORT
+int mdl_iterate(int (*callback)(MDL_ticket *ticket, void *arg), void *arg);
 #endif