maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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?

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

> > > > > 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?

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

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

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