maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

On Dec 28, Sergey Vojtovich wrote:
> > > +    }
> > > +    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?

Not at the moment, sorry. I tried, but couldn't come up with anything
reasonable :( ok, let it stay the way it is.

> > >  #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.

And if you make them static they won't be visible from
TDC_element::lf_alloc_constructor?

> > > @@ -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).

I mean, what lf_hash_search invocation does that unpin corresponds to.
I didn't see any lf_hash_search that wouldn't have a matching unpin.
So this one (and other below) look "homeless"

Regards,
Sergei