maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Sergei,

On Wed, Jun 2, 2021 at 2:00 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> Hi, Aleksey!
>
> On Jun 02, Aleksey Midenkov wrote:
> > > > > > diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test
> > > > > > --- a/mysql-test/suite/versioning/t/partition.test
> > > > > > +++ b/mysql-test/suite/versioning/t/partition.test
> > > > > > @@ -1068,13 +1078,456 @@ alter table t1 add partition partitions 2;
> > > > > >  --replace_result $default_engine DEFAULT_ENGINE
> > > > > >  show create table t1;
> > > > > >  alter table t1 add partition partitions 8;
> > > > > > +drop table t1;
> > > > > > +
> > > > > > +--echo #
> > > > > > +--echo # End of 10.5 tests
> > > > > > +--echo #
> > > > > > +
> > > > > > +--echo #
> > > > > > +--echo # MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
> > > > > > +--echo #
> > > > > > +create or replace table t1 (x int) with system versioning
> > > > > > +partition by system_time limit 1 auto;
> > > > > > +--replace_result $default_engine DEFAULT_ENGINE
> > > > > > +show create table t1;
> > > > > > +
> > > > >
> > > > > in the previous version of this patch you had clarifying comments here, like
> > > > >
> > > > > --echo # INSERT, INSERT .. SELECT don't auto-create partitions
> > > > >
> > > > > or
> > > > >
> > > > > --echo # Number of partitions goes from 3 to 5
> > > > >
> > > > > I kind of miss them now, they were helpful
> > > >
> > > > Yes, they were helpful as is. I removed them like I promised in
> > > > the previous thread. Though, Sergei, you are free to add your own
> > > > comments in the form you like. I'm really sorry, but nobody won't
> > > > tell me what words to use unless I am not able to be clear and
> > > > bear the common sense.
> > >
> > > okay
>
> What about my suggested comments above? Look clear enough?

Like I said, you are free to add your own comments in the form you
like. I'd prefer not to interfere with that process.

>
> > > > > > diff --git a/sql/ha_partition.h b/sql/ha_partition.h
> > > > > > --- a/sql/ha_partition.h
> > > > > > +++ b/sql/ha_partition.h
> > > > > > @@ -1607,7 +1607,7 @@ class ha_partition :public handler
> > > > > >      for (; part_id < part_id_end; ++part_id)
> > > > > >      {
> > > > > >        handler *file= m_file[part_id];
> > > > > > -      DBUG_ASSERT(bitmap_is_set(&(m_part_info->read_partitions), part_id));
> > > > > > +      bitmap_set_bit(&(m_part_info->read_partitions), part_id);
> > > > >
> > > > > would be good to have a comment before the method that it's
> > > > > only used in vers_set_hist_part(), if (vers_info->limit).
> > > > >
> > > > > because if it's a generic method to return the number of rows in a partition
> > > > > (incl. subpartitions) then it's totally not clear why it modifies
> > > > > read_partitions bitmap.
> > > > >
> > > > > or may be you'd want to rename the method to make it look more
> > > > > specific for LIMIT partitions in vers_set_hist_part() ?
> > > >
> > > > Added a comment if it will matter to anyone. No, I prefer generic and
> > > > easy names instead of long and capacious ones. I believe the latter
> > > > ones are non-productive.
> > >
> > > I also prefer generic names, but only if the method is generic.
> > >
> > > I feel that modifying the read_partitions bitmap as a side effect
> > > makes this function rather specialized.
> >
> > Generally there are a lot of complications and exceptions everywhere
> > in the world. That is the nature of life. Or that is just a limitation
> > of us to not see the higher level of order in the observed entropy. If
> > we always mention and signify that, we'll spend too much energy in
> > vain. That is called tediousness. But I made a good enough comment for
> > the whole function, so I hope that will satisfy both of us.
>
> ok, let's try that.
>
> generally, as far as a big code base is concerned, I'd rather prefer
> tediousness over the excitement of multi-hour debugging and discovering
> an unexpected side effect of a generic function.

Did that ever helped? Not for me.

>
> > > > > > diff --git a/sql/lock.cc b/sql/lock.cc
> > > > > > --- a/sql/lock.cc
> > > > > > +++ b/sql/lock.cc
> > > > > > @@ -662,16 +662,28 @@ MYSQL_LOCK *mysql_lock_merge(MYSQL_LOCK *a,MYSQL_LOCK *b)
> > > > > >    DBUG_PRINT("enter", ("a->lock_count: %u  b->lock_count: %u",
> > > > > >                         a->lock_count, b->lock_count));
> > > > > >
> > > > > > -  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;
> > > > >
> > > This doesn't answer my question. Was it part of the MDEV-23639 bug fix?
> > > It was part of the MDEV-23639 commit.
> >
> > 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?
>
> I can understand how changing some memory allocation from MEM_ROOT to
> malloc could fix a bug (if MEM_ROOT lifetime is too short for the
> object), but changing from malloc to MEM_ROOT? This could be a good
> optimization, but how can it fix anything?

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.

>
> > > > > > +  /*
> > > > > > +      NOTE: The semantics of vers_set_hist_part() is double: even when we
> > > > >
> > > > > twofold
> > > >
> > > > Dual, double or twofold. What is the difference between the synonyms?
> > >
> > > https://diffsense.com/diff/double/twofold
> > >
> > > here it's used as an adjective. The page above explains:
> > >
> > >   Twofold: having two parts.
> > >   Examples: "a twofold nature; a twofold sense; a twofold argument"
> >
> > This site says:
> >
> > When used as adjectives, ... twofold means double.
> >
> > 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.

>
> > > > > > +      don't need auto-create, we need to update part_info->hist_part.
> > > > > > +  */
> > > ...
> > > > > > +    else
> > > > > > +    {
> > > > > > +      /*
> > > > > > +          NOTE: this may repeat multiple times until creating thread acquires
> > > > > > +          MDL_EXCLUSIVE. Since auto-creation is rare operation this is acceptable.
> > > > > > +          We could suspend this thread on cond-var but we must first exit
> > > > > > +          MDL_SHARED_WRITE first and we cannot store cond-var into TABLE_SHARE
> > > > > > +          because it is already released and there is no guarantee that it will
> > > > > > +          be same instance if we acquire it 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.
> > > > >
> > > > > What MDEV was it?
> > > > > Could you explain what was the issue and what was the fix,
> > > > > please?
> > > >
> > > > 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.

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



-- 
All the best,

Aleksey Midenkov
@midenok