maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

My fault, sorry. d32089d

On Thu, 30 Jun 2022 at 11:38, Sergei Golubchik <serg@xxxxxxxxxxx> 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@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
>


-- 
Yours truly,
Nikita Malyavin