List:Commits« Previous MessageNext Message »
From:Mattias Jonsson Date:August 17 2009 8:18am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (v.narayanan:3070)
Bug#45823
View as plain text  
Hi VN,

As we discussed on IRC, I also sent the comments for the records


See embedded comments below.



Here is the IRC log too:
[09:29]  <VN> hi mattiasj
[09:29]  <mattiasj> Hi VN
[09:30]  <VN> mattiasj, I submitted a patch for Bug#45823, I wanted to 
explain the behaviour of auto inc that exceeds boundary conditions to you
[09:31]  <mattiasj> I'm looking at your patch and have a couple of 
comments, do you want to discuss them here or by a review mail?
[09:31]  <VN> mattiasj, please send an email
[09:31]  <mattiasj> VN, sure, please explain the boundary conditions to 
me :)
[09:31]  <VN> mattiasj, OK, here are some points
[09:32]  <VN> 1) The current behaviour of partition matches that of 
innodb and myisam, the behaviour is explained in the following points
[09:32]  <VN> 2) Take the example of a TINYINT
[09:32]  <VN>     2.1) The range is -128 - +127
[09:33]  <VN>     2.2) If you insert +129, the value gets set to the 
maximum value of 127 and a warning saying that a value exceeding the 
maximum has been inserted is thrown
[09:34]  <VN>     2.3) This is the same behaviour as myisam and innodb
[09:34]  <VN>     2.4) The same happens with negative values also, so if 
you insert -129 it will get set to -128 and a similar warning is thrown
[09:34]  <VN>     2.5) There is no rounding off that takes place
[09:35]  <VN> 3) Since the behaviour now matches that of the other 
engines and since there is no rounding off of positive values to 
negative and vice-versa,
[09:35]  <VN> I decided to not throw an error
[09:36]  <VN> 4) Also an error actually never gets thrown because the 
inserted value never exceeds maximum on either side
[09:37]  <VN> mattiasj, The above are the points that had for justifying 
my changes regarding boundary conditions
[09:37]  <mattiasj> VN, It would be good to have these behavior tests in 
mysql-test/suite/parts/partition_auto_increment.inc
[09:37]  <mattiasj> VN, OK, great testing!
[09:37]  <VN> mattiasj, I have added all cases to t/partition.test
[09:38]  <VN> mattiasj, If you want me to move them I am OK with it
[09:38]  <VN> mattiasj, I have covered all the cases I was worried about 
in the tests
[09:39]  * thava (~talagu@localhost) has joined #engines
[09:39]  <mattiasj> VN, I would suggest to have some small verification 
tests in t/partition.test and the more extensive tests in 
parts/inc/partition_auto_increment.inc to be able do verify against all 
engines.
[09:39]  <mattiasj> VN, have you tested UPDATE too?
[09:40]  <VN> mattiasj, sure, I will follow your suggestions here, I am 
willing to add more test cases too, no issues there
[09:40]  <VN> mattiasj, No I have not tested update, I covered just 
insert and delete
[09:40]  <mattiasj> VN, I would guess that the bug still exists if 
UPDATE t SET auto_inc = -1 ?
[09:41]  <VN> mattiasj, right, it will need to go through the same code path
[09:42]  <VN> mattiasj, but I am confident that my fix should have 
handled that case too, I will verify this
[09:42]  <VN> mattiasj, There were a few more questions I had with 
respect to the patch
[09:42]  <mattiasj> VN, and the last thing was to use the defines from 
include/my_global instead of just copying the innodb source for the get 
max field value... :) (and that all the points I had for my review so 
far, but I will send a mail just for the record...)
[09:42]  <mattiasj> VN, sure
[09:44]  <mattiasj> VN I think that the compare with max col value must 
be in ha_partition::update_row too, (or better to move it into 
set_auto_increment_if_higher...)
[09:45]  <VN> 1) we should ideally be having a virtual function named 
get_max_value in the super class Field and override it in the integer 
subclasses
[09:46]  <VN> This is however too much complexity for too less 
requirement, I thought a static utility method is good enough
[09:46]  <VN> I however wanted to bring your attention to this
[09:48]  <VN> 2) The method returns 0 as the size on default
[09:48]  <VN> I thought this is OK, because size 0 would mean that the 
datatype is not usable and should automatically be treated as error
[09:48]  <mattiasj> 1) how should BLOBS be handled? (I'm not really the 
person to answer where to put this functionality, SerG or some other 
more senior developer may be better...)
[09:49]  <VN> mattiasj, right, that is why I thought having this as a 
static utility method is OK
[09:49]  <VN> mattiasj, The utility method specifically states that it 
is used for finding the upper limit of the MySQL integer types
[09:50]  <VN> mattiasj, we can pass BLOB as a non-integer type and 
return 0 in this utility method
[09:50]  <mattiasj> VN, right
[09:52]  <VN> mattiasj, This is a change in the storage engine API, 
meaning a change that all the storage engines can see (not really affected)
[09:53]  <VN> mattiasj, do we need clearance from arch (Serg) for this 
change?
[09:53]  <VN> mattiasj, Those were the 3 points I wanted to being to 
your notice
[09:53]  <VN> mattiasj, I agree about your observation of changes to 
update_row, this is oversight by me and needs to be handled
[09:54]  <VN> mattiasj, other than this i will do whatever changes you 
suggest.
[09:56]  <mattiasj> VN, Maybe svoj can answer if the 
Field::get_int_col_max_value is ok to push. I will send my review 
comment soon (the same as I mentioned above, just want to test the patch 
with a few statements...)

/Mattias

V Narayanan wrote:
> #At
> file:///home/narayanan/Work/mysql_checkouts/shared_repository_directory/mysql-5.1-bugteam-45823-02/
> based on revid:ramil@stripped
> 
>  3070 V Narayanan	2009-08-16
>       Bug#45823 Assertion failure in file row/row0mysql.c line 1386
>       
>       Inserting a negative value in the autoincrement column of a
>       partitioned innodb table was causing the value of the auto
>       increment column to wrap around into a very large positive
>       value. This was causing unexpected behaviour.
>       
>       The current patch ensures that before calculating the next
>       auto increment value, the current value is within the positive
>       maximum allowed limit.
>      @ mysql-test/r/partition.result
>         Bug#45823 Assertion failure in file row/row0mysql.c line 1386
>         
>         Contains result files for the test case.
>      @ mysql-test/t/partition.test
>         Bug#45823 Assertion failure in file row/row0mysql.c line 1386
>         
>         Add tests for inserting negative auto increment values into
>         the partition table.
>      @ sql/field.h
>         Bug#45823 Assertion failure in file row/row0mysql.c line 1386
>         
>         Adds a function that returns the upper limits of the mysql
>         integral and floating point types.
>      @ sql/ha_partition.cc
>         Bug#45823 Assertion failure in file row/row0mysql.c line 1386
>         
>         Ensure that the current value is lesser than the upper limit
>         for the type of the field before setting the next auto increment
>         value to be calculated.
> 
>     modified:
>       mysql-test/r/partition.result
>       mysql-test/t/partition.test

For multi engine tests there are also 
mysql-test/suite/parts/partition_auto_increment_<engine>.test

Please add negative testcases to 
mysql-test/suite/parts/inc/partition_auto_increment.inc

This way you would also test the assertion in bug 45823.

>       sql/field.h
>       sql/ha_partition.cc
> === modified file 'mysql-test/r/partition.result'
> --- a/mysql-test/r/partition.result	2009-08-12 10:03:05 +0000
> +++ b/mysql-test/r/partition.result	2009-08-16 10:06:08 +0000
> @@ -2041,4 +2041,132 @@ SELECT  1 FROM t1 JOIN t1 AS t2 USING (a
>  1
>  1
>  DROP TABLE t1;
> +#############################################################################
> +# Bug #45823 - Assertion failure in file row/row0mysql.c line 1386
> +# Bug #43988 - AUTO_INCREMENT errors with partitioned InnoDB tables in 5.1.31
> +##############################################################################
> +# Inserting negative autoincrement values into a partition table (partitions >=
> 4)
> +CREATE TABLE t (c1 INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +c2 INT) PARTITION BY HASH(c1) PARTITIONS 4;
> +INSERT INTO t(c2) VALUES (10);
> +INSERT INTO t(c2) VALUES (20);
> +INSERT INTO t VALUES (-1,-10);
> +INSERT INTO t(c2) VALUES (30);
> +INSERT INTO t(c2) VALUES (40);
> +SELECT * FROM t;
> +c1	c2
> +4	40
> +1	10
> +-1	-10
> +2	20
> +3	30
> +DROP TABLE t;
> +# Reading from a partition table (partitions >= 2 ) after inserting a negative
> +# value into the auto increment column
> +CREATE TABLE t (c1 INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +c2 INT) PARTITION BY HASH(c1) PARTITIONS 2;
> +INSERT INTO t VALUES (-2,-20);
> +INSERT INTO t(c2) VALUES (30);
> +SELECT * FROM t;
> +c1	c2
> +-2	-20
> +1	30
> +DROP TABLE t;
> +# Inserting negative auto increment value into a partition table (partitions >=
> 2)
> +# auto increment value > 2.
> +CREATE TABLE t (c1 INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +c2 INT) PARTITION BY HASH(c1) PARTITIONS 2;
> +INSERT INTO t VALUES (-4,-20);
> +INSERT INTO t(c2) VALUES (30);
> +INSERT INTO t(c2) VALUES (40);
> +SELECT * FROM t;
> +c1	c2
> +-4	-20
> +2	40
> +1	30
> +DROP TABLE t;
> +# Inserting -1 into autoincrement column of a partition table (partition >= 4)
> +CREATE TABLE t (c1 INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +c2 INT) PARTITION BY HASH(c1) PARTITIONS 4;
> +INSERT INTO t(c2) VALUES (10);
> +INSERT INTO t(c2) VALUES (20);
> +INSERT INTO t VALUES (-1,-10);
> +SELECT * FROM t;
> +c1	c2
> +1	10
> +-1	-10
> +2	20
> +INSERT INTO t(c2) VALUES (30);
> +SELECT * FROM t;
> +c1	c2
> +1	10
> +-1	-10
> +2	20
> +3	30
> +DROP TABLE t;
> +# Deleting from an auto increment table after inserting negative values
> +CREATE TABLE t (c1 INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +c2 INT) PARTITION BY HASH(c1) PARTITIONS 4;
> +INSERT INTO t(c2) VALUES (10);
> +INSERT INTO t(c2) VALUES (20);
> +INSERT INTO t VALUES (-1,-10);
> +INSERT INTO t(c2) VALUES (30);
> +INSERT INTO t VALUES (-3,-20);
> +INSERT INTO t(c2) VALUES (40);
> +SELECT * FROM t;
> +c1	c2
> +4	40
> +1	10
> +-1	-10
> +2	20
> +3	30
> +-3	-20
> +DELETE FROM t WHERE c1 > 1;
> +SELECT * FROM t;
> +c1	c2
> +1	10
> +-1	-10
> +-3	-20
> +DROP TABLE t;
> +# Inserting a positive value that exceeds maximum allowed value for an
> +# Auto Increment column (positive maximum)
> +CREATE TABLE t (c1 TINYINT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +c2 INT) PARTITION BY HASH(c1) PARTITIONS 4;
> +INSERT INTO t(c2) VALUES (10);
> +INSERT INTO t(c2) VALUES (20);
> +INSERT INTO t VALUES (126,30);
> +INSERT INTO t VALUES (127,40);
> +# disable warnings that indicate that value greater than limit is being inserted
> +INSERT INTO t VALUES (128,50);
> +ERROR 23000: Duplicate entry '127' for key 'PRIMARY'
> +INSERT INTO t VALUES (129,60);
> +ERROR 23000: Duplicate entry '127' for key 'PRIMARY'
> +SELECT * FROM t;
> +c1	c2
> +1	10
> +2	20
> +126	30
> +127	40
> +DROP TABLE t;
> +# Inserting a negative value that goes below minimum allowed value for an
> +# Auto Increment column (negative minimum)
> +CREATE TABLE t (c1 TINYINT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +c2 INT) PARTITION BY HASH(c1) PARTITIONS 4;
> +INSERT INTO t(c2) VALUES (10);
> +INSERT INTO t(c2) VALUES (20);
> +INSERT INTO t VALUES (-127,30);
> +INSERT INTO t VALUES (-128,40);
> +# disable warnings that indicate that value greater than limit is being inserted
> +INSERT INTO t VALUES (-129,50);
> +ERROR 23000: Duplicate entry '-128' for key 'PRIMARY'
> +INSERT INTO t VALUES (-130,60);
> +ERROR 23000: Duplicate entry '-128' for key 'PRIMARY'
> +SELECT * FROM t;
> +c1	c2
> +-128	40
> +1	10
> +2	20
> +-127	30
> +DROP TABLE t;
> +##############################################################################
>  End of 5.1 tests
> 
> === modified file 'mysql-test/t/partition.test'
> --- a/mysql-test/t/partition.test	2009-08-12 10:03:05 +0000
> +++ b/mysql-test/t/partition.test	2009-08-16 10:06:08 +0000
> @@ -2023,4 +2023,138 @@ INSERT INTO t1 VALUES (6,8,10);
>  SELECT  1 FROM t1 JOIN t1 AS t2 USING (a) FOR UPDATE;
>  
>  DROP TABLE t1;
> +
> +--echo
> #############################################################################
> +--echo # Bug #45823 - Assertion failure in file row/row0mysql.c line 1386
> +--echo # Bug #43988 - AUTO_INCREMENT errors with partitioned InnoDB tables in
> 5.1.31
> +--echo
> ##############################################################################
> +
> +--echo # Inserting negative autoincrement values into a partition table (partitions
> >= 4)
> +
> +CREATE TABLE t (c1 INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +  c2 INT) PARTITION BY HASH(c1) PARTITIONS 4;
> +
> +INSERT INTO t(c2) VALUES (10);
> +INSERT INTO t(c2) VALUES (20);
> +INSERT INTO t VALUES (-1,-10);
> +INSERT INTO t(c2) VALUES (30);
> +INSERT INTO t(c2) VALUES (40);
> +
> +SELECT * FROM t;
> +
> +DROP TABLE t;
> +
> +--echo # Reading from a partition table (partitions >= 2 ) after inserting a
> negative
> +--echo # value into the auto increment column
> +
> +
> +CREATE TABLE t (c1 INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +  c2 INT) PARTITION BY HASH(c1) PARTITIONS 2;
> +
> +INSERT INTO t VALUES (-2,-20);
> +INSERT INTO t(c2) VALUES (30);
> +
> +SELECT * FROM t;
> +
> +DROP TABLE t;
> +
> +--echo # Inserting negative auto increment value into a partition table (partitions
> >= 2)
> +--echo # auto increment value > 2.
> +
> +CREATE TABLE t (c1 INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +  c2 INT) PARTITION BY HASH(c1) PARTITIONS 2;
> +
> +INSERT INTO t VALUES (-4,-20);
> +INSERT INTO t(c2) VALUES (30);
> +INSERT INTO t(c2) VALUES (40);
> +
> +SELECT * FROM t;
> +
> +DROP TABLE t;
> +
> +--echo # Inserting -1 into autoincrement column of a partition table (partition
> >= 4)
> +
> +CREATE TABLE t (c1 INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +  c2 INT) PARTITION BY HASH(c1) PARTITIONS 4;
> +
> +INSERT INTO t(c2) VALUES (10);
> +INSERT INTO t(c2) VALUES (20);
> +INSERT INTO t VALUES (-1,-10);
> +
> +SELECT * FROM t;
> +
> +INSERT INTO t(c2) VALUES (30);
> +
> +SELECT * FROM t;
> +
> +DROP TABLE t;
> +
> +--echo # Deleting from an auto increment table after inserting negative values
> +
> +CREATE TABLE t (c1 INT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +  c2 INT) PARTITION BY HASH(c1) PARTITIONS 4;
> +
> +INSERT INTO t(c2) VALUES (10);
> +INSERT INTO t(c2) VALUES (20);
> +INSERT INTO t VALUES (-1,-10);
> +INSERT INTO t(c2) VALUES (30);
> +INSERT INTO t VALUES (-3,-20);
> +INSERT INTO t(c2) VALUES (40);
> +
> +SELECT * FROM t;
> +
> +DELETE FROM t WHERE c1 > 1;
> +
> +SELECT * FROM t;
> +
> +DROP TABLE t;
> +
> +--echo # Inserting a positive value that exceeds maximum allowed value for an
> +--echo # Auto Increment column (positive maximum)
> +
> +CREATE TABLE t (c1 TINYINT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +  c2 INT) PARTITION BY HASH(c1) PARTITIONS 4;
> +
> +INSERT INTO t(c2) VALUES (10);
> +INSERT INTO t(c2) VALUES (20);
> +INSERT INTO t VALUES (126,30);
> +INSERT INTO t VALUES (127,40);
> +
> +--echo # disable warnings that indicate that value greater than limit is being
> inserted
> +--disable_warnings
> +--error 1062
> +INSERT INTO t VALUES (128,50);
> +--error 1062
> +INSERT INTO t VALUES (129,60);
> +--enable_warnings
> +
> +SELECT * FROM t;
> +
> +DROP TABLE t;
> +
> +--echo # Inserting a negative value that goes below minimum allowed value for an
> +--echo # Auto Increment column (negative minimum)
> +
> +CREATE TABLE t (c1 TINYINT NOT NULL AUTO_INCREMENT, PRIMARY KEY(c1),
> +  c2 INT) PARTITION BY HASH(c1) PARTITIONS 4;
> +
> +INSERT INTO t(c2) VALUES (10);
> +INSERT INTO t(c2) VALUES (20);
> +INSERT INTO t VALUES (-127,30);
> +INSERT INTO t VALUES (-128,40);
> +
> +--echo # disable warnings that indicate that value greater than limit is being
> inserted
> +--disable_warnings
> +--error 1062
> +INSERT INTO t VALUES (-129,50);
> +--error 1062
> +INSERT INTO t VALUES (-130,60);
> +--enable_warnings
> +
> +SELECT * FROM t;
> +
> +DROP TABLE t;
> +
> +--echo
> ##############################################################################
> +
>  --echo End of 5.1 tests
> 
> === modified file 'sql/field.h'
> --- a/sql/field.h	2009-06-09 16:44:26 +0000
> +++ b/sql/field.h	2009-08-16 10:06:08 +0000
> @@ -53,6 +53,67 @@ public:
>    static void *operator new(size_t size) throw ()
>    { return sql_alloc(size); }
>    static void operator delete(void *ptr_arg, size_t size) { TRASH(ptr_arg, size); }
> +  /**
> +    Return the upper limit of the mysql integral and floating point
> +    type.
> +
> +    @param[in] The Field object whose maximum value needs to be returned.
> +    
> +    @return upper limit of Field object type.
> +  */
> +  static ulonglong get_int_col_max_value(const Field* field)
> +  {
> +    ulonglong  max_value= 0;
> +
> +    switch(field->key_type()) {
> +    /* TINY */
> +    case HA_KEYTYPE_BINARY:
> +            max_value= 0xFFULL;
> +            break;

Please reuse the defines from my_global.h
for all the max_value's (UINT_MAX8 here).

> +    case HA_KEYTYPE_INT8:
> +            max_value= 0x7FULL;
> +            break;
> +    /* SHORT */
> +    case HA_KEYTYPE_USHORT_INT:
> +            max_value= 0xFFFFULL;
> +            break;
> +    case HA_KEYTYPE_SHORT_INT:
> +            max_value= 0x7FFFULL;
> +            break;
> +     /* MEDIUM */
> +    case HA_KEYTYPE_UINT24:
> +            max_value= 0xFFFFFFULL;
> +            break;
> +    case HA_KEYTYPE_INT24:
> +            max_value= 0x7FFFFFULL;
> +            break;
> +    /* LONG */
> +    case HA_KEYTYPE_ULONG_INT:
> +            max_value= 0xFFFFFFFFULL;
> +            break;
> +    case HA_KEYTYPE_LONG_INT:
> +            max_value= 0x7FFFFFFFULL;
> +            break;
> +    /* BIG */
> +    case HA_KEYTYPE_ULONGLONG:
> +            max_value= 0xFFFFFFFFFFFFFFFFULL;
> +            break;
> +    case HA_KEYTYPE_LONGLONG:
> +            max_value= 0x7FFFFFFFFFFFFFFFULL;
> +            break;
> +    case HA_KEYTYPE_FLOAT:
> +            /* We use the maximum as per IEEE754-2008 standard, 2^24 */
> +            max_value= 0x1000000ULL;
> +            break;
> +    case HA_KEYTYPE_DOUBLE:
> +            /* We use the maximum as per IEEE754-2008 standard, 2^53 */
> +            max_value= 0x20000000000000ULL;
> +            break;
> +    default:
> +            max_value= 0;
> +    }
> +    return(max_value);
> +}
>  
>    uchar		*ptr;			// Position to field in record
>    uchar		*null_ptr;		// Byte where null_bit is
> 
> === modified file 'sql/ha_partition.cc'
> --- a/sql/ha_partition.cc	2009-08-06 11:31:26 +0000
> +++ b/sql/ha_partition.cc	2009-08-16 10:06:08 +0000
> @@ -3024,7 +3024,26 @@ int ha_partition::write_row(uchar * buf)
>    tmp_disable_binlog(thd); /* Do not replicate the low-level changes. */
>    error= m_file[part_id]->ha_write_row(buf);
>    if (have_auto_increment && !table->s->next_number_keypart)
> -    set_auto_increment_if_higher(table->next_number_field->val_int());
> +  {
> +    /*
> +      Stores the upper limit of the auto increment column type
> +    */
> +    ulonglong col_max_value= 0;
> +    /*
> +      Stores the current value of the auto increment column
> +    */ 
> +    ulonglong auto_inc= 0;
> +    col_max_value= Field::get_int_col_max_value(table->next_number_field);
> +    auto_inc= table->next_number_field->val_int();
> +    /*
> +      If the current value is lesser than or equal to the upper limit
> +    */
> +    if (auto_inc <= col_max_value)
> +    {
> +      set_auto_increment_if_higher(auto_inc);
> +    }
> +  }
> +    
>    reenable_binlog(thd);
>  exit:
>    table->timestamp_field_type= orig_timestamp_type;
> 
> 
> 
> ------------------------------------------------------------------------
> 
> 


What happens when one does an update? (maybe better to move it into 
set_auto_increment_if_higher)

i.e. UPDATE t1 SET auto_inc_col = -1;

A small test on partitioned innodb, look at the very large 
AUTO_INCREMENT in the SHOW CREATE TABLE:
+SHOW CREATE TABLE t1;
+Table	Create Table
+t1	CREATE TABLE `t1` (
+  `c1` int(11) NOT NULL AUTO_INCREMENT,
+  PRIMARY KEY (`c1`)
+) ENGINE=InnoDB AUTO_INCREMENT=166 DEFAULT CHARSET=latin1
+/*!50100 PARTITION BY HASH (c1)
+PARTITIONS 2 */
+UPDATE t1 SET c1 = -15 WHERE c1 = 164;
+SHOW CREATE TABLE t1;
+Table	Create Table
+t1	CREATE TABLE `t1` (
+  `c1` int(11) NOT NULL AUTO_INCREMENT,
+  PRIMARY KEY (`c1`)
+) ENGINE=InnoDB AUTO_INCREMENT=18446744073709551602 DEFAULT CHARSET=latin1
+/*!50100 PARTITION BY HASH (c1)
+PARTITIONS 2 */


/Mattias
Thread
bzr commit into mysql-5.1-bugteam branch (v.narayanan:3070) Bug#45823V Narayanan16 Aug
  • Re: bzr commit into mysql-5.1-bugteam branch (v.narayanan:3070)Bug#45823Mattias Jonsson17 Aug