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
