List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:November 12 2010 10:36am
Subject:Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3179) Bug#50619
View as plain text  
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);
>    }

Thread
bzr commit into mysql-5.5-runtime branch (jon.hauglid:3179) Bug#50619Jon Olav Hauglid10 Nov
  • Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3179) Bug#50619Guilhem Bichot12 Nov
    • Re: bzr commit into mysql-5.5-runtime branch (jon.hauglid:3179)Bug#50619Jon Olav Hauglid13 Dec