List:Commits« Previous MessageNext Message »
From:Mattias Jonsson Date:April 25 2008 1:42pm
Subject:Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479
View as plain text  
Hi,

I believe that the table_share is shared by all instances of a table, 
and that must mean that the partition storage engine shares this 
structure with the used partitions storage engines.

I will let it be as we discussed, and I will add a comment in WL#4305 
about it.

Thank you for being so thorough reviewer!

Mattias


Guilhem Bichot wrote:
> Hello,
> 
> On Fri, Apr 25, 2008 at 01:41:13PM +0200, Mattias Jonsson wrote:
>> Hi,
>>
>> First, I just realized that the use of ha_data will eventually collide
>> for partitioning, when another storage engines start to use it. So in
>> the future, I believe that partitioning should have an own
>> ha_partition_data (HA_PARTITION_DATA*) in TABLE_SHARE, since it is not
>> mutual exclusive using partitioning and an other storage engine using
>> the new ha_data.
> 
> Wait, I'm confused. You mean that the top-level partition table, and
> the tables below it, share a single TABLE_SHARE?
> You can push anyway before clearing this up. I'm away next week of
> vacation, so I don't want to delay your push just for this question.
> 
>> Should I do the change now to be auto_increment specific or partition
>> specific or let it be handled in WL#4305 ?
> 
> As you like.
> 
>> Other than that, thanks for you comments, my replies are marked with MJ.
> 
> And mine with GB2.
>> Guilhem Bichot wrote:
>>> Hello Mattias,
>>>
>>> My comments prefixed with GB.
> 
> 
>>>>  sql/ha_partition.cc@stripped, 2008-04-01 23:04:38+02:00, mattiasj@witty. 
>>>>  +227 -103
>>>>    Bug#33479: Failures using auto_increment and partitioning
>>>>    
>>>>    Changed ha_partition::get_auto_increment from
> file->get_auto_increment
>>>>    to file->info(HA_AUTO_STATUS), since it is works better with
> InnoDB
>>> GB Please add why it works better.
>>>
>> There seems to be a bug in ha_innobase::get_auto_increment. It does not
>> always update the *first_value variable, if called second time as a
>> second partition:
>>
>> If you change the original code (ha_partition::get_auto_incremement,
>> ha_partition.cc ~ row 5637 from 'first_value_part= *first_value' to
>> 'first_value_part= 0' and then DBUG_ASSERT(first_value_part != 0) after
>> the file->get_auto_increment call, it will fail on this test:
>>
>> CREATE TABLE t1 (c1 INT AUTO_INCREMENT PRIMARY KEY)
>> ENGINE=InnoDB
>> PARTITION BY HASH (c1)
>> PARTITIONS 2;
>> INSERT INTO t1 VALUES (NULL);
>> INSERT INTO t1 VALUES (NULL);
> 
> GB2 Looks weird. Please write about this in a code comment somewhere.
> 
>>>> diff -Nrup a/mysql-test/extra/rpl_tests/rpl_auto_increment.test 
>>>> b/mysql-test/extra/rpl_tests/rpl_auto_increment.test
>>>> --- a/mysql-test/extra/rpl_tests/rpl_auto_increment.test	2007-06-06 
>>>> 19:48:51 +02:00
>>>> +++ b/mysql-test/extra/rpl_tests/rpl_auto_increment.test	2008-04-01 
>>>> 23:04:38 +02:00
>>>> @@ -12,7 +12,7 @@
>>>> -- source include/not_ndb_default.inc
>>>> -- source include/master-slave.inc
>>>>
>>>> -eval create table t1 (a int not null auto_increment,b int, primary key 
>>>> (a)) engine=$engine_type2 auto_increment=3;
>>>> +eval create table t1 (a int not null auto_increment,b int, primary key 
>>>> (a)) auto_increment=3 engine=$engine_type2;
>>>> insert into t1 values (NULL,1),(NULL,2),(NULL,3);
>>>> select * from t1;
>>>>
>>>> @@ -76,7 +76,7 @@ insert into t1 values (NULL),(5),(NULL),
>>>> insert into t1 values (500),(NULL),(502),(NULL),(NULL);
>>>> select * from t1;
>>>> set @@insert_id=600;
>>>> ---error ER_DUP_ENTRY
>>>> +--error ER_DUP_ENTRY, ER_DUP_KEY
>>> GB do you know why two errors can happen now and not one?
>>> As for tests and results, I didn't re-check them since one of your
>>> previous commits. So I don't know if since then, you changed anything
>>> to them or the results changed.
>>>
>> MJ: a non partitioned table returns ER_DUP_KEY and a partitioned table
>> returns ER_DUP_ENTRY, but I have changed this in the testcase (and will
>> not include the updated rpl_auto_increment test since I have not used it
>> in this patch.
> 
> GB2 It is not normal that "a non partitioned table returns ER_DUP_KEY
> and a partitioned table returns ER_DUP_ENTRY", it makes life harder
> for somebody writing an application which needs to detect duplicate
> key errors to handle them in a certain way. I suggest filing a bug
> report or fixing it.
> 
>>>> +    {
>>>> +      HA_DATA_PARTITION *ha_data= (HA_DATA_PARTITION*) 
>>>> table_share->ha_data;
>>>> +      lock_auto_increment();
>>>> +      if (next_insert_id < ha_data->next_auto_inc_val &&
>>>> +          auto_inc_interval_for_cur_row.maximum() >=
>>>> +          ha_data->next_auto_inc_val)
>>> GB you may want to cache ha_data->next_auto_inc_val in a local
>>> variable to not dereference two times.
>>>
>> MJ: Done, but this is on the fine line of over optimization :)
> 
> GB2 Yes, was just a suggestion.
> 
>>>> +        ha_data->next_auto_inc_val= next_insert_id;
>>>> +      DBUG_PRINT("info", ("ha_data->next_auto_inc_val: %lu",
>>>> +                          (ulong) ha_data->next_auto_inc_val));
>>>> +
>>>> +      /* Unlock the multi row statement lock taken in get_auto_increment
> 
>>>> */
>>>> +      if (auto_increment_multi_row_stmt_lock)
>>>> +      {
>>>> +        auto_increment_multi_row_stmt_lock= FALSE;
>>>> +        DBUG_PRINT("info", ("unlocking 
>>>> auto_increment_multi_row_stmt_lock"));
>>> GB If there wasn't this DBUG_PRINT(), I'd suggest to remove the if() and
>>> unconditionally do
>>> auto_increment_multi_row_stmt_lock= FALSE;
>>> because:
>>> "read + test + jump"
>>> is probably more costly than "write".
>>>
>> MJ: Point taken, but I would go for having the DBUG_PRINT, since it can
>> be good to have when debugging.
> 
> GB2 ok
>  
> I'm ok with everything you wrote, ok to push.
> Thanks for the marathon fixing that bug.
> 

-- 
Mattias Jonsson, Software Engineer
MySQL AB, www.mysql.com

Are you MySQL certified?  www.mysql.com/certification
Thread
bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479mattiasj1 Apr
  • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Guilhem Bichot17 Apr
    • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Mattias Jonsson25 Apr
      • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Guilhem Bichot25 Apr
        • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Mattias Jonsson25 Apr
          • Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479Sergei Golubchik25 Apr