Hi, Jan! On Oct 15, Jan Lindström wrote:
Few questions:
(1) Is this review for a full patch or just problems on wsrep_abort_transaction ?
a full patch
(2) In case at wsrep_abort_transaction we do not have a transaction idea is that we do not anymore want to enter InnoDB i.e. innobase_kill_query, that is the reason we set MUST_ABORT to wsrep_conflict_state so that no more mutexes from InnoDB is acquired.
right, sorry. I thought I've removed too much from wsrep_abort_transaction.
(3) In wsrep_close_connections code used LOCK_thread_lock, is that enough to protect THDs on list from concurrent disconnect and delete? (I added locking for THD::LOCK_thd_data but I was not sure is it necessary)
Isn't it a separate issue? Anyway, as far as I understand, LOCK_thread_count protects the list of threads, a THD cannot be added to or deleted from the list. but a THD can be disconnected or killed and it'll wait somewhere at the very end of the destructor to remove itself from the list. So, yes, you need to take LOCK_thd_data or LOCK_thd_kill too.
(4) Do we still keep manual KILL as TOI ?
Not in my patch, no.
(5) What I do with thr_lock.c there we pass victim THD pointer to wsrep_abort_thd and it is as you pointed out mostly unprotected on that code, LOCK TABLE is TOI
It's fine. Same logic as in wsrep_innobase_kill_one_trx(). You kill a victim thd that owns thr locks. This THD cannot just disappear, it has to release those locks first. And you own the lock->mutex. So the THD isn't going anywhere until you return from thr_lock().
(6) Please note that fix will not be ready 19th of October as we again change critical path, Ramesh will run QA with pquery
sure
On Thu, Oct 14, 2021 at 9:49 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Jan!
Here's an idea of the fix:
Let's always use the KILL mutex locking order, that is
victim_thread->LOCK_thd_data -> lock_sys->mutex -> victim_trx->mutex
For this we need to fix wsrep_abort_transaction(), which is called from the server, and wsrep_innobase_kill_one_trx(), which is called from BF thread.
wsrep_abort_transaction() can be fixed by not invoking wsrep_innobase_kill_one_trx() and always using KILL code path (that is wsrep_thd_awake) and forcing rollback after the kill.
wsrep_innobase_kill_one_trx() can be fixed by not locking LOCK_thd_data at all, just don't lock it. We know that the victim waits on a lock inside InnoDB and we've locked trx mutex and lock_sys mutex. The victim cannot go away, cannot modify its data, it cannot do anything. So, LOCK_thd_data doesn't seem to be necessary at that point.
I've attached a demo patch. It compiles, but I didn't try to run it, it's only to show the idea, not a working fix (I already suspect I removed too much from wsrep_abort_transaction()). Note it's the patch for 10.2 at the commit 29bbcac0ee8^ - that is one commit before my fix.
On Oct 12, Jan Lindström wrote:
Hi Sergei,
Update on wsrep_close_connections problem. My suggestion to fix this issue is on
https://github.com/MariaDB/server/commit/99cbe03a44cc95e6f548550df51e7201ebe...
If you have a better solution, please advise.
R: Jan
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org