Hi, Sergey!
On Mar 15, Sergey Vojtovich wrote:
> At lp:maria/10.0
>
> ------------------------------------------------------------
> revno: 4053
> revision-id: s...@mariadb.org-20140315185301-ce4r6br023co80dm
> parent: sa...@montyprogram.com-20140314073116-nklb0jkjd4t1w40n
> committer: Sergey Vojtovich <s...@mariadb.org>
> 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
_______________________________________________
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
