maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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

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@xxxxxxxxxxx> 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@xxxxxxxxxxx
> >
> 
> 
> -- 
> Yours truly,
> Nikita Malyavin