=) no problem about disagree, i'm agree now hehe

thanks guys! :)

2013/10/29 Sergey Vojtovich <svoj@mariadb.org>

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@gmail.com>
>
> > 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@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help   : https://help.launchpad.net/ListHelp

--

Roberto Spadim
SPAEmpresarial