Hello,
Jon Olav Hauglid a écrit, Le 10.11.2010 16:05:
> #At file:///export/home/x/mysql-5.5-runtime-bug50619/ based on
> revid:jon.hauglid@stripped
>
> 3179 Jon Olav Hauglid 2010-11-10
> Bug #50619 assert in handler::update_auto_increment
>
> This assert could happen during subsequent inserts if -1 had
> already been inserted into an auto increment column.
"Grunt".
We spend time testing and fixing cases which the manual prohibits:
http://dev.mysql.com/doc/refman/5.5/en/create-table.html
"An AUTO_INCREMENT column works properly only if it contains only
positive values. Inserting a negative number is regarded as inserting a
very large positive number. This is done to avoid precision problems
when numbers “wrap” over from positive to negative and also to ensure
that you do not accidentally get an AUTO_INCREMENT column that contains 0."
> The root cause of the problem is that auto increment values are
> internally stored unsigned while auto increment column types can
> be signed values. When an explict value is set for the auto
> increment column, the internal auto increment value is reset to
> the explicit value + 1. However, if the explicit value is -1,
> converting this value to unsigned and adding 1 will yield 0 as
> a result (due to overflow). Since 0 would be outside the reserved
> auto increment interval for the insert (auto increment values
> should start at 1), the assert would be triggered.
>
> This patch fixes the problem by checking if the explicit value
> is negative. If this is the case, auto increment values are not
> reset.
>
> Test case added to auto_increment.test.
> === modified file 'mysql-test/r/auto_increment.result'
> --- a/mysql-test/r/auto_increment.result 2010-08-18 09:35:41 +0000
> +++ b/mysql-test/r/auto_increment.result 2010-11-10 15:05:22 +0000
> @@ -476,3 +476,16 @@ SELECT a FROM t2;
> a
> 2
> DROP TABLE t1, t2;
> +#
> +# Bug#50619 assert in handler::update_auto_increment
> +#
> +DROP TABLE IF EXISTS t1;
> +CREATE TABLE t1 (pk INT AUTO_INCREMENT, PRIMARY KEY (pk));
> +INSERT INTO t1 VALUES (-1);
> +CREATE TRIGGER tr1 BEFORE DELETE ON t1 FOR EACH ROW SET @aux = 1 ;
This trigger is needed to provoke the crash, I checked. Do you know why?
I cannot explain it to myself, it only sets a variable...?
> +REPLACE INTO t1 (pk) VALUES (NULL), (-1);
> +SELECT * FROM t1;
> +pk
> +-1
> +1
> +DROP TABLE t1;
>
> === modified file 'mysql-test/t/auto_increment.test'
> --- a/mysql-test/t/auto_increment.test 2010-08-18 09:35:41 +0000
> +++ b/mysql-test/t/auto_increment.test 2010-11-10 15:05:22 +0000
> @@ -342,3 +342,20 @@ SELECT a FROM t2;
>
> DROP TABLE t1, t2;
>
> +
> +--echo #
> +--echo # Bug#50619 assert in handler::update_auto_increment
> +--echo #
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1;
> +--enable_warnings
This section above is not needed, each test portion normally cleans up
after itself (there is "DROP TABLE t1,t2" above).
> === modified file 'sql/handler.cc'
> --- a/sql/handler.cc 2010-10-18 11:27:52 +0000
> +++ b/sql/handler.cc 2010-11-10 15:05:22 +0000
> @@ -2204,14 +2204,14 @@ compute_next_insert_id(ulonglong nr,stru
> }
>
>
> -void handler::adjust_next_insert_id_after_explicit_value(ulonglong nr)
> +void handler::adjust_next_insert_id_after_explicit_value(longlong nr)
> {
> /*
> If we have set THD::next_insert_id previously and plan to insert an
> explicitely-specified value larger than this, we need to increase
> THD::next_insert_id to be greater than the explicit value.
> */
> - if ((next_insert_id > 0) && (nr >= next_insert_id))
> + if ((next_insert_id > 0) && (nr >=0) && ((ulonglong)nr >=
> next_insert_id))
I think the coding style asks for a space between >= and 0.
Could you please add a comment explaining why this nr>=0 (reusing
something from the commit comment)?
What if the auto_increment column is BIGINT UNSIGNED? Such column has a
range of [0, 18446744073709551615] (the entire range of ulonglong).
So if I explicitely insert
18446744073709551615-3 ,
next_insert_id must be bumped to
18446744073709551615-3+1;
but with the code change, 18446744073709551615-3 is cast to longlong
when passed to the function above, so becomes -4, so
nr>=0 is false, and we don't bump next_insert_id.
I didn't test though.
> set_next_insert_id(compute_next_insert_id(nr,
> &table->in_use->variables));
> }
handler::adjust_next_insert_id_after_explicit_value() is used also in
sql_insert.cc, I wonder whether this also has the (nr==-4) problem I
mentioned above.
> @@ -2348,7 +2348,7 @@ int handler::update_auto_increment()
> */
> DBUG_ASSERT(next_insert_id >= auto_inc_interval_for_cur_row.minimum());
>
> - if ((nr= table->next_number_field->val_int()) != 0 ||
> + if ((table->next_number_field->val_int() != 0) ||
> (table->auto_increment_field_not_null &&
> thd->variables.sql_mode & MODE_NO_AUTO_VALUE_ON_ZERO))
> {
> @@ -2358,7 +2358,8 @@ int handler::update_auto_increment()
> the last NULL needs to insert 3764, not the value of the first NULL plus
> 1).
> */
> - adjust_next_insert_id_after_explicit_value(nr);
> + adjust_next_insert_id_after_explicit_value(
> + table->next_number_field->val_int());
So we have two calls to table->next_number_field->val_int() (one in the
if() and one in the block above which we enter if the condition is
true); the original code had a single call and cached the result in
"nr". I prefer to keep the caching.
> insert_id_for_cur_row= 0; // didn't generate anything
> DBUG_RETURN(0);
> }
