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
