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-L16... 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-L17... Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org