maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Marko!

On Dec 19, Marko Mäkelä wrote:
> I have already approved the InnoDB changes.

Yes, I've noticed :)

> On Sat, Dec 19, 2020 at 12:43 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> > Hi, Jan!
> [snip]
> > > +  mysql_mutex_lock(&LOCK_thd_kill);
> > > +  mysql_mutex_unlock(&LOCK_thd_kill);
> >
> > Note that THD destructor starts from `assert_not_linked()` - it's
> > important, because if the THD is linked in the list of threads, then
> > someone might start using it after you released LOCK_thd_kill.
> >
> > With the new logic you need to move assert_not_linked to
> > free_connection().
> >
> > otherwise ok to push
> 
> Do you mean that also the empty mutex lock/unlock pair should be
> removed from THD::~THD()? It was copied (not moved) from there in this
> patch.

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.

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