List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:April 25 2008 1:36pm
Subject:Re: bk commit into 5.1 tree (mattiasj:1.2518) BUG#33479
View as plain text  
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.

-- 
   __  ___     ___ ____  __
  /  |/  /_ __/ __/ __ \/ /    Mr. Guilhem Bichot <guilhem@stripped>
 / /|_/ / // /\ \/ /_/ / /__   Sun Microsystems, Lead Software Engineer
/_/  /_/\_, /___/\___\_\___/   Bordeaux, France
       <___/   www.mysql.com   
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