maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

On Mar 15, Sergey Vojtovich wrote:
> At lp:maria/10.0
> 
> ------------------------------------------------------------
> revno: 4053
> revision-id: svoj@xxxxxxxxxxx-20140315185301-ce4r6br023co80dm
> parent: sanja@xxxxxxxxxxxxxxxx-20140314073116-nklb0jkjd4t1w40n
> committer: Sergey Vojtovich <svoj@xxxxxxxxxxx>
> branch nick: 10.0
> timestamp: Sat 2014-03-15 22:53:01 +0400
> message:
>   MDEV-5864 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.free_tables
>   
>   Let TABLE_SHARE::tdc.free_tables, TABLE_SHARE::tdc.all_tables,
>   TABLE_SHARE::tdc.flushed and corresponding invariants be protected by
>   per-share TABLE_SHARE::tdc.LOCK_table_share instead of global LOCK_open.

This looks pretty good to me!
A couple of comments, see below.

> === modified file 'sql/sql_plist.h'
> --- a/sql/sql_plist.h	2013-11-20 11:05:39 +0000
> +++ b/sql/sql_plist.h	2014-03-15 18:53:01 +0000
> @@ -142,6 +142,14 @@ class I_P_List : public C, public I
>  
>      return result;
>    }
> +  inline T *back()
> +  {
> +    T *result= m_first;
> +    if (result)
> +      while (*B::next_ptr(result))
> +        result= *B::next_ptr(result);
> +    return result;
> +  }

I think this is wrong. The correct way would be to use I_P_List<> with
the I_P_List_fast_push_back policy, then you could use
have get_last() method to get the last element in the list.
May be as

    inline T *back()
    {
      return *I::get_last();
    }

>    void swap(I_P_List<T, B, C> &rhs)
>    {
>      swap_variables(T *, m_first, rhs.m_first);
> 
> === modified file 'sql/table_cache.cc'
> --- a/sql/table_cache.cc	2014-03-06 12:19:12 +0000
> +++ b/sql/table_cache.cc	2014-03-15 18:53:01 +0000
> @@ -70,13 +70,6 @@ static int32 tc_count; /**< Number of TA
>  
>  
>  /**
> -  Protects TABLE_SHARE::tdc.free_tables, TABLE_SHARE::tdc.all_tables.
> -*/
> -
> -mysql_mutex_t LOCK_open;

So, you've now removed LOCK_open completely? Cool!

> -
> -
> -/**
>    Protects unused shares list.
>  
>    TABLE_SHARE::tdc.prev
> @@ -360,12 +378,15 @@ bool tc_release_table(TABLE *table)
>    table->in_use= 0;
>    /* Add table to the list of unused TABLE objects for this share. */
>    table->s->tdc.free_tables.push_front(table);
> -  mysql_mutex_unlock(&LOCK_open);
> +  mysql_mutex_unlock(&table->s->tdc.LOCK_table_share);
>    return false;
>  
>  purge:
> +  /* Wait for MDL deadlock detector */
> +  while (table->s->tdc.all_tables_refs)
> +    mysql_cond_wait(&table->s->tdc.COND_release, &table->s->tdc.LOCK_table_share);

you're using it many times, I'd put it in a small static inline
function, like

 static wait_for_mdl_deadlock_detector(TABLE *table)
 {
   mysql_mutex_assert_owner(&table->s->tdc.LOCK_table_share);
   while (table->s->tdc.all_tables_refs)
     mysql_cond_wait(&table->s->tdc.COND_release, &table->s->tdc.LOCK_table_share);
 }

>    tc_remove_table(table);
> -  mysql_mutex_unlock(&LOCK_open);
> +  mysql_mutex_unlock(&table->s->tdc.LOCK_table_share);
>    table->in_use= 0;
>    intern_close_table(table);
>    return true;

Regards,
Sergei