maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey!

On Jun 02, Aleksey Midenkov wrote:
> > > > > > >
> > > > > > > -  if (!(sql_lock= (MYSQL_LOCK*)
> > > > > > > -     my_malloc(key_memory_MYSQL_LOCK, sizeof(*sql_lock) +
> > > > > > > -               sizeof(THR_LOCK_DATA*)*((a->lock_count+b->lock_count)*2) +
> > > > > > > -               sizeof(TABLE*)*(a->table_count+b->table_count),MYF(MY_WME))))
> > > > > > > -    DBUG_RETURN(0);                          // Fatal error
> > > > > > > +  const size_t lock_size= sizeof(*sql_lock) +
> > > > > > > +    sizeof(THR_LOCK_DATA *) * ((a->lock_count + b->lock_count) * 2) +
> > > > > > > +    sizeof(TABLE *) * (a->table_count + b->table_count);
> > > > > > > +  if (thd)
> > > > > > > +  {
> > > > > > > +    sql_lock= (MYSQL_LOCK *) thd->alloc(lock_size);
> > > > > > > +    if (!sql_lock)
> > > > > > > +      DBUG_RETURN(0);
> > > > > > > +    sql_lock->flags= GET_LOCK_ON_THD;
> > > > > >
> > > Sorry, I meant the moment when MDEV-23639 calls mysql_lock_merge().
> > > Yes, that is the part of the bug fix.
> >
> > What did it fix and how?
> >
> That's not about changing the method, that's about merging locks. When
> I merged my locks with thd there were already thd-allocated locks.

And? How did allocating locks on THD fix anything?

> > > > > > > +  /*
> > > > > > > +      NOTE: The semantics of vers_set_hist_part() is double:
> > > > > >
> > > > > > twofold
> > > > >
> > > Do you really believe in that butter butterish? :) I mean do we
> > > need to discuss all sorts of butter? That level of white noise
> > > should be ignored.
> >
> > I'm just trying to avoid incorrect usage of a language.
> 
> I wish that threshold of incorrectness would be more relaxed.

Sure. If any authoritative source will show that both words are correct
in this context, I'll readily admit that I'm wrong and will remember it
for the future. I didn't comment on a whim, it wasn't a matter of taste,
it was as far as I know an actual incorrect usage of words, an error.

> > > > > > > +      table_list->vers_skip_create= false;
> > > > > > > +      ot_ctx->vers_create_count= 0;
> > > > > > > +      action= Open_table_context::OT_REOPEN_TABLES;
> > > > > > > +      table_arg= NULL;
> > > > > > > +    }
> > > > > >
> > > > > > I'm afraid I don't understand. All this business with
> > > > > > vers_skip_create and vers_skip_auto_create, it wasn't in the
> > > > > > previous version of the patch. So, I believe it was a fix
> > > > > > for one of the MDEV bugs reported and fixed meanwhile.
> > > > >
> > > > > No, that was the multi-threaded case which worked good for me,
> > > > > but suddenly I discovered it fails on some buildbot.
> > > >
> > > > Could you elaborate on what the problem was? Two threads trying
> > > > to add the partition in parallel? I'd expect MDL_EXCLUSIVE to
> > > > prevent that.
> > >
> > > MDL_EXCLUSIVE prevents parallel execution of
> > > repair_from_failed_open(), but not sequential. So it can add
> > > several partitions instead of 1, one after another.
> >
> > What's the sequence of events? One thread decides to add a
> > partition, takes an MDL_EXCLUSIVE, the other thread decides to add a
> > partition, waits for MDL_EXCLUSIVE, the first one finishes adding a
> > partition, releases the lock, the second grabs it and adds a second
> > partition?
> 
> Right.

Okay. Then, why a thread didn't check the number of partitions after
acquiring MDL_EXCLUSIVE? Like with mutexes, if there's a mutex that
protects a shared variable, you first acquire the mutex, then read the
variable's value. Not the other way around.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx