← Back to team overview

maria-developers team mailing list archive

Re: 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT

 

Sergei,

On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> 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?

LTM_PRELOCKED had locks on thd, I keep merged locks on thd. Isn't that
enough for you? Also freeing was impossible for locks on thd.

>
> > > > > > > > +  /*
> > > > > > > > +      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.

Go ahead and play that game again.

>
> > > > > > > > +      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.

Number of partitions is not a shared variable. part_info is kept in
TABLE instance. To get new value it must reacquire share, reparse
part_sql string. Then comparing with what? After releasing
MDL_SHARED_WRITE everything is gone: TABLE, TABLE_SHARE. You must
store somewhere old value, presumably in Open_table_context.

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


-- 
All the best,

Aleksey Midenkov
@midenok


Follow ups

References