← Back to team overview

maria-developers team mailing list archive

Re: 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@xxxxxxxxxxx


Follow ups