maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

thanks for you comments. Answers inline.

On Fri, Mar 13, 2015 at 08:58:38PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> On Mar 12, svoj@xxxxxxxxxxx wrote:
> > revision-id: 20e5c37ccfac9d95a2db1f295f211656df1ce160
> > parent(s): 190858d996e7dc90e01fe8a5e7daec6af0012b23
> > committer: Sergey Vojtovich
> > branch nick: 10.1
> > timestamp: 2015-03-12 18:15:08 +0400
> > message:
> > 
> > MDEV-7728 - Spiral patch 032_mariadb-10.0.15.scale_registering_xid.diff
> > 
> > XID cache is now based on lock-free hash.
> > Also fixed lf_hash_destroy() to call alloc destructor.
> > 
> > Note that previous implementation had race condition when thread was accessing
> > XA owned by different thread. This new implementation doesn't fix it either.
...skip...

> > @@ -5106,120 +5109,205 @@ void mark_transaction_to_rollback(THD *thd, bool all)
> >  /***************************************************************************
> >    Handling of XA id cacheing
> >  ***************************************************************************/
> > +class XID_cache_element
> > +{
> > +  uint m_state;
> 
> shouldn't it be uint32 ?
> That's the type that my_atomic_add32/cas32 work with.
Yes, it should. Thanks!

> 
> > +  static const uint DELETED= 1 << 31;
> > +public:
> > +  XID_STATE *m_xid_state;
> 
> O-okay. I *think* your hand-written locking logic works.
> But please add a comment, explaining how it works,
> all four methods, the meaning of DELETED, etc.
> Perhaps DELETED should rather be NOT_INITIALIZED?
A very valid request. IMHO mutex is overkill here, that's why I implemented this
trivial locking. I'll add comments and think about better names.

> 
> > +  bool lock()
> > +  {
> > +    if (my_atomic_add32_explicit(&m_state, 1, MY_MEMORY_ORDER_ACQUIRE) & DELETED)
> > +    {
> > +      unlock();
> > +      return false;
> > +    }
> > +    return true;
> > +  }
> > +  void unlock()
> > +  {
> > +    my_atomic_add32_explicit(&m_state, -1, MY_MEMORY_ORDER_RELEASE);
> > +  }
> > +  void mark_deleted()
> > +  {
> > +    uint old= 0;
> > +    while (!my_atomic_cas32_weak_explicit(&m_state, &old, DELETED,
> > +                                          MY_MEMORY_ORDER_RELAXED,
> > +                                          MY_MEMORY_ORDER_RELAXED))
> 
> usually one should use LF_BACKOFF inside spin-loops.
Good point. Though LF_BACKOFF doesn't seem to do anything useful.

> 
> > +      old= 0;
> > +  }
> > +  void mark_not_deleted()
> > +  {
> > +    DBUG_ASSERT(m_state & DELETED);
> > +    my_atomic_add32_explicit(&m_state, -DELETED, MY_MEMORY_ORDER_RELAXED);
> > +  }
> > +  static void lf_hash_initializer(LF_HASH *hash __attribute__((unused)),
> > +                                  XID_cache_element *element,
> > +                                  XID_STATE *xid_state)
> > +  {
> > +    element->m_xid_state= xid_state;
> > +    xid_state->xid_cache_element= element;
> > +  }
> > +  static void lf_alloc_constructor(uchar *ptr)
> > +  {
> > +    XID_cache_element *element= (XID_cache_element*) (ptr + LF_HASH_OVERHEAD);
> > +    element->m_state= DELETED;
> > +  }
> > +  static void lf_alloc_destructor(uchar *ptr)
> > +  {
> > +    XID_cache_element *element= (XID_cache_element*) (ptr + LF_HASH_OVERHEAD);
> > +    if (element->m_state != DELETED)
> 
> How can this happen? You mark an element DELETED before lf_hash_delete()
That's for elements inserted by ha_recover(). There's no guarantee that they
will be deleted.

...skip...

> > -XID_STATE *xid_cache_search(XID *xid)
> > +
> > +XID_STATE *xid_cache_search(THD *thd, XID *xid)
> >  {
> > -  mysql_mutex_lock(&LOCK_xid_cache);
> > -  XID_STATE *res=(XID_STATE *)my_hash_search(&xid_cache, xid->key(),
> > -                                             xid->key_length());
> > -  mysql_mutex_unlock(&LOCK_xid_cache);
> > -  return res;
> > +  DBUG_ASSERT(thd->xid_hash_pins);
> > +  XID_cache_element *element=
> > +    (XID_cache_element*) lf_hash_search(&xid_cache, thd->xid_hash_pins,
> > +                                        xid->key(), xid->key_length());
> > +  if (element)
> > +  {
> > +    lf_hash_search_unpin(thd->xid_hash_pins);
> > +    return element->m_xid_state;
> 
> Normally, one should not access the element after it's unpinned, so the
> protocol is
> 
>         XID_STATE *state= element->m_xid_state;
>         lf_hash_search_unpin(thd->xid_hash_pins);
>         return state;
> 
> Perhaps your locking/deleted m_state guarantees that element is safe to
> access? But I failed to see that :(
This is noted in cset comment:
Note that previous implementation had race condition when thread was accessing
XA owned by different thread. This new implementation doesn't fix it either.

I can fix it if you like.

> > diff --git a/storage/spider/spd_table.cc b/storage/spider/spd_table.cc
> > index 36768ae..411c7ae 100644
> > --- a/storage/spider/spd_table.cc
> > +++ b/storage/spider/spd_table.cc
> > @@ -41,11 +41,13 @@
> 
> I'd prefer to have spider changes in a separate commit
Ok, I'll split them.

...skip...

Thanks,
Sergey