Re: [Maria-developers] e9293e31ec6: MDEV-28930 ALTER TABLE Deadlocks with parallel TL_WRITE
Hi, Nikita, This is very good. Just one comment, see below: On Jun 29, Nikita Malyavin wrote:
revision-id: e9293e31ec6 (mariadb-10.6.1-499-ge9293e31ec6) parent(s): 6cfcb36d027 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2022-06-29 20:16:19 +0300 message:
MDEV-28930 ALTER TABLE Deadlocks with parallel TL_WRITE
ALTER ONLINE TABLE acquires table with TL_READ. Myisam normally acquires TL_WRITE for DML, which makes it hang until table is freed.
We deadlock once ALTER upgrades its MDL lock.
Solution: Unlock table earlier. We don't need to hold TL_READ once we finished copying. Relay log replication requires no data locks on `from` table.
diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc index 425de469a7f..90f6283d9df 100644 --- a/sql/log_event_server.cc +++ b/sql/log_event_server.cc @@ -5734,7 +5734,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) used in the transaction and proceed with execution of the actual event. */ - if (!thd->lock) + if (!thd->lock && rgi->tables_to_lock_count) { /* Lock_tables() reads the contents of thd->lex, so they must be
Instead of adding a second condition to the execution path for every row event in the replication, consider this: - if (!thd->lock) + if (!thd->open_tables) this will work identically for replication, I hope, but will be always false in your case, as you still have the table open. I mean, that's the idea. I run the rpl suite with it, but haven't done any further testing, so not completely sure it'll work. When making this change, don't forget to update the comment right above this if(). Ok to push into bb-10.10-MDEV-16329 after that. Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Thanks! Updated here: 86ee039cdcff. Let's see, how it'll go on tests On Wed, 29 Jun 2022 at 23:36, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
This is very good. Just one comment, see below:
On Jun 29, Nikita Malyavin wrote:
revision-id: e9293e31ec6 (mariadb-10.6.1-499-ge9293e31ec6) parent(s): 6cfcb36d027 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2022-06-29 20:16:19 +0300 message:
MDEV-28930 ALTER TABLE Deadlocks with parallel TL_WRITE
ALTER ONLINE TABLE acquires table with TL_READ. Myisam normally acquires TL_WRITE for DML, which makes it hang until table is freed.
We deadlock once ALTER upgrades its MDL lock.
Solution: Unlock table earlier. We don't need to hold TL_READ once we finished copying. Relay log replication requires no data locks on `from` table.
diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc index 425de469a7f..90f6283d9df 100644 --- a/sql/log_event_server.cc +++ b/sql/log_event_server.cc @@ -5734,7 +5734,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) used in the transaction and proceed with execution of the actual event. */ - if (!thd->lock) + if (!thd->lock && rgi->tables_to_lock_count) { /* Lock_tables() reads the contents of thd->lex, so they must be
Instead of adding a second condition to the execution path for every row event in the replication, consider this:
- if (!thd->lock) + if (!thd->open_tables)
this will work identically for replication, I hope, but will be always false in your case, as you still have the table open. I mean, that's the idea. I run the rpl suite with it, but haven't done any further testing, so not completely sure it'll work.
When making this change, don't forget to update the comment right above this if().
Ok to push into bb-10.10-MDEV-16329 after that.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- Yours truly, Nikita Malyavin
Hi, Nikita, You forgot to update the comment. As I've wrote in a review (below):
When making this change, don't forget to update the comment right above this if().
It says "If there is no locks taken, this is the first binrow event..." and should say something like "If there are no tables open" or something like that. And instead of "We should then lock all the tables", for example, "We should then open and lock all tables" Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org On Jun 30, Nikita Malyavin wrote:
Thanks! Updated here: 86ee039cdcff. Let's see, how it'll go on tests
On Wed, 29 Jun 2022 at 23:36, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
This is very good. Just one comment, see below:
On Jun 29, Nikita Malyavin wrote:
revision-id: e9293e31ec6 (mariadb-10.6.1-499-ge9293e31ec6) parent(s): 6cfcb36d027 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2022-06-29 20:16:19 +0300 message:
MDEV-28930 ALTER TABLE Deadlocks with parallel TL_WRITE
ALTER ONLINE TABLE acquires table with TL_READ. Myisam normally acquires TL_WRITE for DML, which makes it hang until table is freed.
We deadlock once ALTER upgrades its MDL lock.
Solution: Unlock table earlier. We don't need to hold TL_READ once we finished copying. Relay log replication requires no data locks on `from` table.
diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc index 425de469a7f..90f6283d9df 100644 --- a/sql/log_event_server.cc +++ b/sql/log_event_server.cc @@ -5734,7 +5734,7 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi) used in the transaction and proceed with execution of the actual event. */ - if (!thd->lock) + if (!thd->lock && rgi->tables_to_lock_count) { /* Lock_tables() reads the contents of thd->lex, so they must be
Instead of adding a second condition to the execution path for every row event in the replication, consider this:
- if (!thd->lock) + if (!thd->open_tables)
this will work identically for replication, I hope, but will be always false in your case, as you still have the table open. I mean, that's the idea. I run the rpl suite with it, but haven't done any further testing, so not completely sure it'll work.
When making this change, don't forget to update the comment right above this if().
Ok to push into bb-10.10-MDEV-16329 after that.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- Yours truly, Nikita Malyavin
My fault, sorry. d32089d On Thu, 30 Jun 2022 at 11:38, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
You forgot to update the comment. As I've wrote in a review (below):
When making this change, don't forget to update the comment right above this if().
It says "If there is no locks taken, this is the first binrow event..." and should say something like "If there are no tables open" or something like that. And instead of "We should then lock all the tables", for example, "We should then open and lock all tables"
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Thanks! Updated here: 86ee039cdcff. Let's see, how it'll go on tests
On Wed, 29 Jun 2022 at 23:36, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
This is very good. Just one comment, see below:
On Jun 29, Nikita Malyavin wrote:
revision-id: e9293e31ec6 (mariadb-10.6.1-499-ge9293e31ec6) parent(s): 6cfcb36d027 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2022-06-29 20:16:19 +0300 message:
MDEV-28930 ALTER TABLE Deadlocks with parallel TL_WRITE
ALTER ONLINE TABLE acquires table with TL_READ. Myisam normally acquires TL_WRITE for DML, which makes it hang until table is freed.
We deadlock once ALTER upgrades its MDL lock.
Solution: Unlock table earlier. We don't need to hold TL_READ once we finished copying. Relay log replication requires no data locks on `from`
diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc index 425de469a7f..90f6283d9df 100644 --- a/sql/log_event_server.cc +++ b/sql/log_event_server.cc @@ -5734,7 +5734,7 @@ int
Rows_log_event::do_apply_event(rpl_group_info *rgi)
used in the transaction and proceed with execution of the actual event. */ - if (!thd->lock) + if (!thd->lock && rgi->tables_to_lock_count) { /* Lock_tables() reads the contents of thd->lex, so they must be
Instead of adding a second condition to the execution path for every row event in the replication, consider this:
- if (!thd->lock) + if (!thd->open_tables)
this will work identically for replication, I hope, but will be always false in your case, as you still have the table open. I mean, that's
On Jun 30, Nikita Malyavin wrote: table. the
idea. I run the rpl suite with it, but haven't done any further testing, so not completely sure it'll work.
When making this change, don't forget to update the comment right above this if().
Ok to push into bb-10.10-MDEV-16329 after that.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- Yours truly, Nikita Malyavin
-- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik