maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Jan!

On Dec 20, Jan Lindström wrote:
> Hi Sergei,
> 
> > Not exactly. In 10.2 THD::~THD() only has
> >
> >   mysql_mutex_lock(&LOCK_thd_data);
> >   mysql_mutex_unlock(&LOCK_thd_data);
> >
> > it's not what Jan did. So I thought it'd be safer to leave it in place.
> > But 10.5 has
> >
> >   if (WSREP_NNULL(this)) mysql_mutex_lock(&LOCK_thd_data);
> >   mysql_mutex_lock(&LOCK_thd_kill);
> >   mysql_mutex_unlock(&LOCK_thd_kill);
> >   if (WSREP_NNULL(this)) mysql_mutex_unlock(&LOCK_thd_data);
> >
> > which is *exactly* what Jan did, so yes, when merging this chunk should
> > go away (and it's quite likely that you'll be doing this merge, so
> > please don't forget). Because of Jan's change in THD::~THD() there will
> > be a merge conflict in the destructor, so it cannot slip unnoticed and
> > auto-merged.
> >
> 
> I do not follow merging this chunk should go away here.
... 
> So do you mean that I should take LOCK_thd_data even when we do not have
> wsrep_rgi for wsrep as if I correctly understand
> wsrep_rgi is there only for applier not all THDs with WSREP(thd).

No, I mean that 10.5 has the the same code in THD::~THD() that you have
added to free_connection() in 10.2.

So when your patch will be merged to 10.5, this old code should be
removed from THD::~THD(), there's no need to lock same mutexes twice.

Here's your code:
https://github.com/MariaDB/server/blob/cede9020fa/sql/sql_class.cc#L1617-L1642

so this old code (in the same file, a bit down) can be removed:
https://github.com/MariaDB/server/blob/cede9020fa/sql/sql_class.cc#L1725-L1728

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx