maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

thanks for this tricky review. My answers inline.

On Sat, Dec 27, 2014 at 08:39:28PM +0100, Sergei Golubchik wrote:
> > --- a/sql/sql_class.cc
> > +++ b/sql/sql_class.cc
> > @@ -912,17 +912,17 @@ bool Drop_table_error_handler::handle_condition(THD *thd,
> >  #endif /* defined(ENABLED_DEBUG_SYNC) */
> >     wait_for_commit_ptr(0),
> >     main_da(0, false, false),
> > -   m_stmt_da(&main_da)
> > +   m_stmt_da(&main_da),
> >  #ifdef WITH_WSREP
> > -  ,
> >     wsrep_applier(is_wsrep_applier),
> >     wsrep_applier_closing(false),
> >     wsrep_client_thread(false),
> >     wsrep_apply_toi(false),
> >     wsrep_po_handle(WSREP_PO_INITIALIZER),
> >     wsrep_po_cnt(0),
> > -   wsrep_apply_format(0)
> > +   wsrep_apply_format(0),
> >  #endif
> > +  tdc_hash_pins(0)
> 
> Eh... I actually wanted wsrep related data to the at the end of the THD.
> Just to keep the rest of the THD layout identical for wsrep and non-wsrep
> builds. In case a wsrep-enabled plugin is loaded into non-wsrep server or
> visa versa. That was an early change, though, I've done more cleanups after
> that and moved wsrep into a service, so perhaps (even, most probably)
> this is not needed anymore. But I'd better stay on the safe side and keep
> wsrep at the end of THD.
Ok, I moved tdc_hash_pins before wsrep stuff. Though there is already
"query_timer" after wsrep vars.

> 
> >  {
> >    ulong tmp;
> >  
> > @@ -1698,6 +1698,8 @@ void THD::cleanup(void)
> >  
> >    free_root(&main_mem_root, MYF(0));
> >    main_da.free_memory();
> > +  if (tdc_hash_pins)
> > +    lf_hash_put_pins(tdc_hash_pins);
> 
> no tdc_hash_pins=0 here?
Hmm... that's the second time I see git picking up wrong function name. In fact
this is THD::~THD. I believe it is Ok not to reset tdc_hash_pins here.

Should we move this to THD::cleanup()? Probably. This way we'd better do
tdc_hash_pins= 0 in THD::init().

I'll keep this as is unless you'll object.

> 
> >    if (status_var.memory_used != 0)
> >    {
> >      DBUG_PRINT("error", ("memory_used: %lld", status_var.memory_used));
> > diff --git a/sql/table.h b/sql/table.h
> > --- a/sql/table.h
> > +++ b/sql/table.h
> > @@ -611,32 +612,7 @@ struct TABLE_SHARE
> >    mysql_mutex_t LOCK_ha_data;           /* To protect access to ha_data */
> >    mysql_mutex_t LOCK_share;             /* To protect TABLE_SHARE */
> >  
> > -  typedef I_P_List <TABLE, TABLE_share> TABLE_list;
> > -  typedef I_P_List <TABLE, All_share_tables> All_share_tables_list;
> > -  struct
> > -  {
> > -    /**
> > -      Protects ref_count, m_flush_tickets, all_tables, free_tables, flushed,
> > -      all_tables_refs.
> > -    */
> > -    mysql_mutex_t LOCK_table_share;
> > -    mysql_cond_t COND_release;
> > -    TABLE_SHARE *next, **prev;            /* Link to unused shares */
> > -    uint ref_count;                       /* How many TABLE objects uses this */
> > -    uint all_tables_refs;                 /* Number of refs to all_tables */
> > -    /**
> > -      List of tickets representing threads waiting for the share to be flushed.
> > -    */
> > -    Wait_for_flush_list m_flush_tickets;
> > -    /*
> > -      Doubly-linked (back-linked) lists of used and unused TABLE objects
> > -      for this share.
> > -    */
> > -    All_share_tables_list all_tables;
> > -    TABLE_list free_tables;
> > -    ulong version;
> > -    bool flushed;
> > -  } tdc;
> > +  TDC_element *tdc;
> 
> Hmm. Ok. I think I like this change - TABLE_SHARE now contains table metadata
> and everything related to tdc is moved to TDC_element. Right?
Right. I was going to do this for a long time already. My plan is to make
TDC_element private to table_cache.cc.

There're still a few table cache leftovers on TABLE_SHARE, but they're used
outside of table cache and I preferred to leave them for now.

With a few extra tricks I could've used the whole TABLE_SHARE as lf-hash
element, but it doesn't go well inline with the above plan.

> 
> >  
> >    LEX_CUSTRING tabledef_version;
> >  
> > diff --git a/sql/table.cc b/sql/table.cc
> > --- a/sql/table.cc
> > +++ b/sql/table.cc
> > @@ -3845,11 +3844,11 @@ bool TABLE_SHARE::visit_subgraph(Wait_for_flush *wait_for_flush,
> >      because we won't try to acquire tdc.LOCK_table_share while
> >      holding a write-lock on MDL_lock::m_rwlock.
> >    */
> > -  mysql_mutex_lock(&tdc.LOCK_table_share);
> > -  tdc.all_tables_refs++;
> > -  mysql_mutex_unlock(&tdc.LOCK_table_share);
> > +  mysql_mutex_lock(&tdc->LOCK_table_share);
> > +  tdc->all_tables_refs++;
> > +  mysql_mutex_unlock(&tdc->LOCK_table_share);
> 
> Do I understand it correctly that a TDC_element can never be deleted from
> LF_HASH as long as at least one TABLE_SHARE references it?
You understand it correctly, but this question doesn't go well with the context.

Here deadlock detector says: hey, I'm traversing TDC_element::all_tables list,
please don't remove or add elements until I'm done.

TDC_element::wait_for_mdl_deadlock_detector() does the wait.

> 
> >  
> > -  All_share_tables_list::Iterator tables_it(tdc.all_tables);
> > +  TDC_element::All_share_tables_list::Iterator tables_it(tdc->all_tables);
> >  
> >    /*
> >      In case of multiple searches running in parallel, avoid going
> > @@ -3946,21 +3945,10 @@ bool TABLE_SHARE::wait_for_old_version(THD *thd, struct timespec *abstime,
> >  
> >    mdl_context->done_waiting_for();
> >  
> > -  mysql_mutex_lock(&tdc.LOCK_table_share);
> > -
> > -  tdc.m_flush_tickets.remove(&ticket);
> > -
> > -  if (tdc.m_flush_tickets.is_empty() && tdc.ref_count == 0)
> > -  {
> > -    /*
> > -      If our thread was the last one using the share,
> > -      we must destroy it here.
> > -    */
> > -    mysql_mutex_unlock(&tdc.LOCK_table_share);
> > -    destroy();
> > -  }
> > -  else
> > -    mysql_mutex_unlock(&tdc.LOCK_table_share);
> > +  mysql_mutex_lock(&tdc->LOCK_table_share);
> > +  tdc->m_flush_tickets.remove(&ticket);
> > +  mysql_cond_broadcast(&tdc->COND_release);
> > +  mysql_mutex_unlock(&tdc->LOCK_table_share);
> 
> Why is the logic here different?
Here we signal tdc_delete_share_from_hash() that we completed waiting for old
version and there is one element less in m_flush_tickets. That is it may try
to actually remove share from hash.

> 
> >  
> >  
> >    /*
> > diff --git a/sql/table_cache.h b/sql/table_cache.h
> > --- a/sql/table_cache.h
> > +++ b/sql/table_cache.h
> > @@ -16,6 +16,171 @@
> >     Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301  USA */
> >  
> >  
> > +extern PSI_mutex_key key_TABLE_SHARE_LOCK_table_share;
> > +extern PSI_cond_key key_TABLE_SHARE_COND_release;
> > +
> > +class TDC_element
> > +{
> > +public:
> > +  uchar m_key[NAME_LEN + 1 + NAME_LEN + 1];
> > +  uint m_key_length;
> > +  ulong version;
> > +  bool flushed;
> > +  TABLE_SHARE *share;
> > +
> > +  typedef I_P_List <TABLE, TABLE_share> TABLE_list;
> > +  typedef I_P_List <TABLE, All_share_tables> All_share_tables_list;
> > +  /**
> > +    Protects ref_count, m_flush_tickets, all_tables, free_tables, flushed,
> > +    all_tables_refs.
> > +  */
> > +  mysql_mutex_t LOCK_table_share;
> > +  mysql_cond_t COND_release;
> > +  TDC_element *next, **prev;            /* Link to unused shares */
> > +  uint ref_count;                       /* How many TABLE objects uses this */
> > +  uint all_tables_refs;                 /* Number of refs to all_tables */
> > +  /**
> > +    List of tickets representing threads waiting for the share to be flushed.
> > +  */
> > +  Wait_for_flush_list m_flush_tickets;
> > +  /*
> > +    Doubly-linked (back-linked) lists of used and unused TABLE objects
> > +    for this share.
> > +  */
> > +  All_share_tables_list all_tables;
> > +  TABLE_list free_tables;
> > +
> > +  TDC_element() {}
> > +
> > +  TDC_element(const char *key, uint key_length) : m_key_length(key_length)
> > +  {
> > +    memcpy(m_key, key, key_length);
> > +  }
> > +
> > +
> > +  void dbug_release_assertions()
> 
> Somewhat confusing name. If I hadn't see the code below
> I'd thought this function releases some assertions (how? why?)
> may be assert_correctness_at_release() would be less ambiguous?
Renamed to assert_clean_share().

> 
> > +  {
> > +    DBUG_ASSERT(share == 0);
> > +    DBUG_ASSERT(ref_count == 0);
> > +    DBUG_ASSERT(m_flush_tickets.is_empty());
> > +    DBUG_ASSERT(all_tables.is_empty());
> > +    DBUG_ASSERT(free_tables.is_empty());
> > +    DBUG_ASSERT(all_tables_refs == 0);
> > +    DBUG_ASSERT(next == 0);
> > +    DBUG_ASSERT(prev == 0);
> > +  }
> > +
> > +
> > +  /**
> > +    Acquire TABLE object from table cache.
> > +
> > +    @pre share must be protected against removal.
> > +
> > +    Acquired object cannot be evicted or acquired again.
> > +
> > +    While locked:
> > +    - pop object from TABLE_SHARE::tdc.free_tables
> > +
> > +    While unlocked:
> > +    - mark object used by thd
> 
> "While locked" / "While unlocked" - what does that mean?
Leftovers from old code. These comments were useful while we did a whole bunch
of stuff under LOCK_open. Removed.

> 
> > +
> > +    @return TABLE object, or NULL if no unused objects.
> > +  */
> > +
> > +  TABLE *acquire_table(THD *thd)
> > +  {
> > +    TABLE *table;
> > +
> > +    mysql_mutex_lock(&LOCK_table_share);
> > +    table= free_tables.pop_front();
> > +    if (table)
> > +    {
> > +      DBUG_ASSERT(!table->in_use);
> 
> lots of asserts - good!
> 
> > +      table->in_use= thd;
> > +      /* The ex-unused table must be fully functional. */
> > +      DBUG_ASSERT(table->db_stat && table->file);
> > +      /* The children must be detached from the table. */
> > +      DBUG_ASSERT(!table->file->extra(HA_EXTRA_IS_ATTACHED_CHILDREN));
> 
> I don't even think of asking why it's HA_EXTRA_IS_ATTACHED_CHILDREN and not
> HA_EXTRA_HAS_ATTACHED_CHILDREN or, at least, HA_EXTRA_CHILDREN_ARE_ATTACHED.
> Ah, whatever... Horrors of the English language hidden in old MySQL code :)
Well, these asserts were inheritted from pretty old code indeed.

> 
> > +    }
> > +    mysql_mutex_unlock(&LOCK_table_share);
> > +    return table;
> > +  }
> > +
> > +
> > +  /**
> > +    Get last element of free_tables.
> > +  */
> > +
> > +  TABLE *free_tables_back()
> 
> Eh, I've thought for a few seconds what could it mean to "free tables back"
> until I read the function comment. You see, "free" is so often used as a verb
> that one absolutely doesn't see it that here it means "free_tables' back"
> even though it's also a common naming pattern (xxx_back for the list xxx).
> Rename the method, may be?
This way I'll have to rename free_tables, because "free" comes from it's name.
Or you have better option on your mind?

> 
> > +  {
> > +    TABLE_list::Iterator it(share->tdc->free_tables);
> > +    TABLE *entry, *last= 0;
> > +     while ((entry= it++))
> > +       last= entry;
> > +    return last;
> > +  }
> > +
> > +
> > +  /**
> > +    Wait for MDL deadlock detector to complete traversing tdc.all_tables.
> > +
> > +    Must be called before updating TABLE_SHARE::tdc.all_tables.
> > +  */
> > +
> > +  void wait_for_mdl_deadlock_detector()
> > +  {
> > +    while (all_tables_refs)
> > +      mysql_cond_wait(&COND_release, &LOCK_table_share);
> > +  }
> > +
> > +
> > +  /**
> > +    Prepeare table share for use with table definition cache.
> > +  */
> > +
> > +  static void lf_alloc_constructor(uchar *arg)
> > +  {
> > +    TDC_element *element= (TDC_element*) (arg + LF_HASH_OVERHEAD);
> > +    DBUG_ENTER("tdc_init_share");
> 
> intentionally? or forgot to update DBUG_ENTER?
Updated.

> 
> > +    mysql_mutex_init(key_TABLE_SHARE_LOCK_table_share,
> > +                     &element->LOCK_table_share, MY_MUTEX_INIT_FAST);
> > +    mysql_cond_init(key_TABLE_SHARE_COND_release, &element->COND_release, 0);
> > +    element->m_flush_tickets.empty();
> > +    element->all_tables.empty();
> > +    element->free_tables.empty();
> > +    element->all_tables_refs= 0;
> > +    element->share= 0;
> > +    element->ref_count= 0;
> > +    element->next= 0;
> > +    element->prev= 0;
> > +    DBUG_VOID_RETURN;
> > +  }
> > +
> > +
> > +  /**
> > +    Release table definition cache specific resources of table share.
> > +  */
> > +
> > +  static void lf_alloc_destructor(uchar *arg)
> > +  {
> > +    TDC_element *element= (TDC_element*) (arg + LF_HASH_OVERHEAD);
> > +    DBUG_ENTER("tdc_deinit_share");
> 
> intentionally? or forgot to update DBUG_ENTER?
Updated.

...skip...
> > -    if (!(*start_list = (OPEN_TABLE_LIST *)
> > -          sql_alloc(sizeof(**start_list)+share->table_cache_key.length)))
> > -    {
> > -      open_list=0;                              // Out of memory
> > -      break;
> > -    }
> > +  if (!(*arg->start_list= (OPEN_TABLE_LIST *) sql_alloc(
> 
> when you know the thd, prefer thd->alloc() over sql_alloc()
> (which is, basically, current_thd->alloc())
Updated.

...skip...
> > diff --git a/sql/table_cache.cc b/sql/table_cache.cc
> > --- a/sql/table_cache.cc
> > +++ b/sql/table_cache.cc
> > @@ -72,32 +76,23 @@
> >  /**
> >    Protects unused shares list.
> >  
> > -  TABLE_SHARE::tdc.prev
> > -  TABLE_SHARE::tdc.next
> > -  oldest_unused_share
> > -  end_of_unused_share
> > +  TDC_element::prev
> > +  TDC_element::next
> > +  unused_shares
> >  */
> >  
> >  static mysql_mutex_t LOCK_unused_shares;
> > -static mysql_rwlock_t LOCK_tdc; /**< Protects tdc_hash. */
> >  my_atomic_rwlock_t LOCK_tdc_atomics; /**< Protects tdc_version. */
> >  
> >  #ifdef HAVE_PSI_INTERFACE
> > -static PSI_mutex_key key_LOCK_unused_shares, key_TABLE_SHARE_LOCK_table_share;
> > +PSI_mutex_key key_LOCK_unused_shares, key_TABLE_SHARE_LOCK_table_share;
> 
> really? not static?
These need to be visible by TDC_element::lf_alloc_constructor. Their static
status is to be restored later.

...skip...
> > @@ -265,85 +269,41 @@ void tc_add_table(THD *thd, TABLE *table)
> >  
> >    if (need_purge)
> >    {
> > -    TABLE_SHARE *purge_share= 0;
> > -    TABLE_SHARE *share;
> > -    TABLE *entry;
> > -    ulonglong UNINIT_VAR(purge_time);
> > -    TDC_iterator tdc_it;
> > +    tc_add_table_arg argument;
> > +    argument.purge_time= ULONGLONG_MAX;
> > +    tdc_iterate(thd, (my_hash_walk_action) tc_add_table_callback, &argument);
> >  
> > -    tdc_it.init();
> > -    while ((share= tdc_it.next()))
> > +    if (argument.purge_time != ULONGLONG_MAX)
> >      {
> > -      mysql_mutex_lock(&share->tdc.LOCK_table_share);
> > -      if ((entry= tc_free_tables_back(share)) &&
> > -          (!purge_share || entry->tc_time < purge_time))
> > +      TDC_element *element= (TDC_element*) lf_hash_search(&tdc_hash,
> > +                                                          thd->tdc_hash_pins,
> > +                                                          argument.key,
> > +                                                          argument.key_length);
> > +      if (element)
> >        {
> > -          purge_share= share;
> > -          purge_time= entry->tc_time;
> > -      }
> > -      mysql_mutex_unlock(&share->tdc.LOCK_table_share);
> > -    }
> > +        TABLE *entry;
> > +        mysql_mutex_lock(&element->LOCK_table_share);
> > +        lf_hash_search_unpin(thd->tdc_hash_pins);
> > +        element->wait_for_mdl_deadlock_detector();
> 
> you wait under a mutex? hmm...
Yes, wait_for_mdl_deadlock_detector()/mysql_cond_wait() will release the mutex.

...skip...
> > -static int tdc_delete_share_from_hash(TABLE_SHARE *share)
> > +static void tdc_delete_share_from_hash(TDC_element *element)
> >  {
> > +  THD *thd= current_thd;
> > +  LF_PINS *pins;
> > +  TABLE_SHARE *share;
> >    DBUG_ENTER("tdc_delete_share_from_hash");
> > -  mysql_rwlock_wrlock(&LOCK_tdc);
> > -  mysql_mutex_lock(&share->tdc.LOCK_table_share);
> > -  if (--share->tdc.ref_count)
> > -  {
> > -    mysql_cond_broadcast(&share->tdc.COND_release);
> > -    mysql_mutex_unlock(&share->tdc.LOCK_table_share);
> > -    mysql_rwlock_unlock(&LOCK_tdc);
> > -    DBUG_RETURN(1);
> > -  }
> > -  my_hash_delete(&tdc_hash, (uchar*) share);
> > +
> > +  share= element->share;
> > +  DBUG_ASSERT(share);
> > +  element->share= 0;
> >    /* Notify PFS early, while still locked. */
> 
> "while still locked" what?
Not a valid comment anymore. It meant: unbind pfs before another thread
creates new share with the same name.

> 
> >    PSI_CALL_release_table_share(share->m_psi);
> >    share->m_psi= 0;
> > -  mysql_rwlock_unlock(&LOCK_tdc);
> >  
> > -  if (share->tdc.m_flush_tickets.is_empty())
> > +  if (!element->m_flush_tickets.is_empty())
> >    {
> > -    /* No threads are waiting for this share to be flushed, destroy it. */
> > -    mysql_mutex_unlock(&share->tdc.LOCK_table_share);
> > -    free_table_share(share);
> > -  }
> > -  else
> > -  {
> > -    Wait_for_flush_list::Iterator it(share->tdc.m_flush_tickets);
> > +    Wait_for_flush_list::Iterator it(element->m_flush_tickets);
> >      Wait_for_flush *ticket;
> >      while ((ticket= it++))
> >        (void) ticket->get_ctx()->m_wait.set_status(MDL_wait::GRANTED);
> > -    /*
> > -      If there are threads waiting for this share to be flushed,
> > -      the last one to receive the notification will destroy the
> > -      share. At this point the share is removed from the table
> > -      definition cache, so is OK to proceed here without waiting
> > -      for this thread to do the work.
> > -    */
> > -    mysql_mutex_unlock(&share->tdc.LOCK_table_share);
> > +
> > +    do
> > +    {
> > +      mysql_cond_wait(&element->COND_release, &element->LOCK_table_share);
> > +    } while (!element->m_flush_tickets.is_empty());
> >    }
> > -  DBUG_RETURN(0);
> > +
> > +  mysql_mutex_unlock(&element->LOCK_table_share);
> 
> Ok, so the caller must lock LOCK_table_share?
> please put mysql_mutex_assert_owner() in the beginning of the function
Done.

> 
> > +
> > +  if (thd)
> 
> When thd is NULL here? On shutdown?
On shutdown, SIGHUP handler, COM_REFRESH. Not aware of anything else.

...skip...
> > -TABLE_SHARE *tdc_lock_share(const char *db, const char *table_name)
> > +TDC_element *tdc_lock_share(THD *thd, const char *db, const char *table_name)
> >  {
> > +  TDC_element *element;
> >    char key[MAX_DBKEY_LENGTH];
> > -  uint key_length;
> >  
> >    DBUG_ENTER("tdc_lock_share");
> > -  key_length= tdc_create_key(key, db, table_name);
> > +  if (fix_thd_pins(thd))
> > +    DBUG_RETURN((TDC_element*) MY_ERRPTR);
> 
> I suppose you can do it in THD:init, if you'd like. A THD almost always
> will need to access table definition cache, so it'll need these pins.
> 
> Then there will be no need to return MY_ERRPTR here and handle it in the
> caller
THD::init is void itself. :(

...skip...

> > @@ -702,60 +616,60 @@ TABLE_SHARE *tdc_acquire_share(THD *thd, const char *db, const char *table_name,
> >                                 TABLE **out_table)
> >  {
> >    TABLE_SHARE *share;
> > +  TDC_element *element;
> >    bool was_unused;
> >    DBUG_ENTER("tdc_acquire_share");
> >  
> > -  mysql_rwlock_rdlock(&LOCK_tdc);
> > -  share= (TABLE_SHARE*) my_hash_search_using_hash_value(&tdc_hash, hash_value,
> > -                                                        (uchar*) key,
> > -                                                        key_length);
> > -  if (!share)
> > +  if (fix_thd_pins(thd))
> > +    DBUG_RETURN(0);
> > +
> > +retry:
> > +  while (!(element= (TDC_element*) lf_hash_search_using_hash_value(&tdc_hash,
> 
> Was lf_hash_search_using_hash_value added in a separate patch?
Yes, I'll include it into updated patch.

> 
> > +                    thd->tdc_hash_pins, hash_value, (uchar*) key, key_length)))
> >    {
> > -    TABLE_SHARE *new_share;
> > -    mysql_rwlock_unlock(&LOCK_tdc);
> > +    TDC_element tmp(key, key_length);
> > +    int res= lf_hash_insert(&tdc_hash, thd->tdc_hash_pins, (uchar*) &tmp);
> >  
> > -    if (!(new_share= alloc_table_share(db, table_name, key, key_length)))
> > +    if (res == -1)
> >        DBUG_RETURN(0);
> > -    new_share->error= OPEN_FRM_OPEN_ERROR;
> > +    else if (res == 1)
> > +      continue;
> >  
> > -    mysql_rwlock_wrlock(&LOCK_tdc);
> > -    share= (TABLE_SHARE*) my_hash_search_using_hash_value(&tdc_hash, hash_value,
> > -                                                          (uchar*) key,
> > -                                                          key_length);
> > -    if (!share)
> > -    {
> > -      bool need_purge;
> > +    element= (TDC_element*) lf_hash_search_using_hash_value(&tdc_hash,
> > +             thd->tdc_hash_pins, hash_value, (uchar*) key, key_length);
> > +    lf_hash_search_unpin(thd->tdc_hash_pins);
> 
> How can you be sure that nobody frees your element after you unpin it?
Because element->share is 0, that is nobody is allowed to find it. It is
guaranteed not to be in unused_shares list too. tdc_delete_share_from_hash()
asserts non-NULL share.

> 
> > +    DBUG_ASSERT(element);
> > +    element->dbug_release_assertions();
> >  
> > -      share= new_share;
> > -      mysql_mutex_lock(&share->tdc.LOCK_table_share);
> > -      if (my_hash_insert(&tdc_hash, (uchar*) share))
> > +    if (!(share= alloc_table_share(db, table_name, key, key_length)))
> >        {
> > -        mysql_mutex_unlock(&share->tdc.LOCK_table_share);
> > -        mysql_rwlock_unlock(&LOCK_tdc);
> > -        free_table_share(share);
> > +      lf_hash_delete(&tdc_hash, thd->tdc_hash_pins, key, key_length);
> >          DBUG_RETURN(0);
> >        }
> > -      need_purge= tdc_hash.records > tdc_size;
> > -      mysql_rwlock_unlock(&LOCK_tdc);
> >  
> >        /* note that tdc_acquire_share() *always* uses discovery */
> >        open_table_def(thd, share, flags | GTS_USE_DISCOVERY);
> > -      share->tdc.ref_count++;
> > -      mysql_mutex_unlock(&share->tdc.LOCK_table_share);
> >  
> >        if (share->error)
> >        {
> > -        tdc_delete_share_from_hash(share);
> > +      free_table_share(share);
> > +      lf_hash_delete(&tdc_hash, thd->tdc_hash_pins, key, key_length);
> >          DBUG_RETURN(0);
> >        }
> > -      else if (need_purge)
> > +
> > +    mysql_mutex_lock(&element->LOCK_table_share);
> > +    element->share= share;
> > +    share->tdc= element;
> > +    element->ref_count++;
> > +    element->version= tdc_refresh_version();
> > +    element->flushed= false;
> > +    mysql_mutex_unlock(&element->LOCK_table_share);
> > +
> >          tdc_purge(false);
> >        if (out_table)
> >          *out_table= 0;
> >        share->m_psi= PSI_CALL_get_table_share(false, share);
> >        goto end;
> > -    }
> > -    free_table_share(new_share);
> >    }
> >  
> >    /* cannot force discovery of a cached share */
> > @@ -763,18 +677,25 @@ TABLE_SHARE *tdc_acquire_share(THD *thd, const char *db, const char *table_name,
> >  
> >    if (out_table && (flags & GTS_TABLE))
> >    {
> > -    if ((*out_table= tc_acquire_table(thd, share)))
> > +    if ((*out_table= element->acquire_table(thd)))
> >      {
> > -      mysql_rwlock_unlock(&LOCK_tdc);
> > +      lf_hash_search_unpin(thd->tdc_hash_pins);
> 
> what this lf_hash_search_unpin corresponds to?
Element found, but it may be to-be-deleted element or not-yet-open element.
In both cases free_tables must be empty. This is also guarded by the following
assert: DBUG_RETURN(element->share).

This is hot-path, that's why I kept separate handling of it.

> 
> >        DBUG_ASSERT(!(flags & GTS_NOLOCK));
> > -      DBUG_ASSERT(!share->error);
> > -      DBUG_ASSERT(!share->is_view);
> > -      DBUG_RETURN(share);
> > +      DBUG_ASSERT(element->share);
> > +      DBUG_ASSERT(!element->share->error);
> > +      DBUG_ASSERT(!element->share->is_view);
> > +      DBUG_RETURN(element->share);
> >      }
> >    }
> >  
> > -  mysql_mutex_lock(&share->tdc.LOCK_table_share);
> > -  mysql_rwlock_unlock(&LOCK_tdc);
> > +  mysql_mutex_lock(&element->LOCK_table_share);
> > +  if (!(share= element->share))
> > +  {
> > +    mysql_mutex_unlock(&element->LOCK_table_share);
> > +    lf_hash_search_unpin(thd->tdc_hash_pins);
> 
> and here
Same here, but now we do fair handling of to-be-deleted or not-yet-open.

> 
> > +    goto retry;
> > +  }
> > +  lf_hash_search_unpin(thd->tdc_hash_pins);
> 
> may be that first lf_hash_search_unpin was wrong?
> the one that goes directly after lf_hash_search.
The one that goes directly after lf_hash_search is totally different story.
That lf_hash_search was for newly inserted element. This lf_hash_search is
normal hash lookup.

...skip...

> > +
> > +  if (!element->ref_count)
> > +  {
> > +    if (element->prev)
> > +    {
> > +      unused_shares.remove(element);
> > +      /* Concurrent thread may start using share again, reset prev and next. */
> 
> What concurrent thread? element->ref_count is 0, you removed
> the the share from the unused_shares list. There can be no concurrent
> threads, as far as I understand. And indeed, you do remove the share
> from the hash (which does free_table_share) immediately below,
> which also proves that there can be no concurrent threads.
Now it is impossible indeed. Removed comment, but kept next= 0 and prev= 0.

...skip...

Thanks,
Sergey