List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:December 1 2010 1:17pm
Subject:Re: review of BUG#39828
View as plain text  
Hello,

Sergey Glukhov a écrit, Le 01.12.2010 12:27:
> On 11/29/2010 05:46 PM, Guilhem Bichot wrote:
>>>  3525 Sergey Glukhov    2010-11-26
>>>       Bug#39828 : Autoinc wraps around when offset and increment > 1
>>>       Auto increment value wraps when performing a bulk insert with
>>>       auto_increment_increment and auto_increment_offset greater than
>>>       one.
>>>       The fix:
>>>       If overflow is happened then set auto_inc_overflow to TRUE, 
>>>       return MAX_ULONGLONG value and check if overflow happens
>>>       before storing value into the field in update_auto_increment().
>>
>>

>> Here is also a different proposal which looks very simple. I observed 
>> that 2^64-1 cannot be autogenerated, because of this code in 
>> update_auto_increment():
>>       get_auto_increment(variables->auto_increment_offset,
>>                          variables->auto_increment_increment,
>>                          nb_desired_values, &nr,
>> &nb_reserved_values);
>>       if (nr == ~(ulonglong) 0)
>>         DBUG_RETURN(HA_ERR_AUTOINC_READ_FAILED);  // Mark failure
>> See: if engine returns 2^64-1 it's treated as an error.
>> So 2^64-1 can generally not be a correct value in next_insert_id. So 
>> we could decide that we will use next_insert_id==2^64-1 as an error 
>> marker, a marker of overflow.
>> That would mean:
>> - we delete handler::auto_inc_overflow
>> - if compute_next_insert_id() overflows, it returns 2^64-1 (as in your 
>> patch)
>> - we add this kind of test:
>>       if (unlikely(nr == ~(ulonglong) 0))
>>         DBUG_RETURN(HA_ERR_AUTOINC_RANGE);
>> in place of
>>       if (auto_inc_overflow)
>>         DBUG_RETURN(HA_ERR_AUTOINC_ERANGE);
>>
>> I tested this idea on auto_increment.test and innodb-autoinc.test, it 
>> seems to work well. It also has a nice side effect: some of the 
>> queries of innodb-autoinc.test, which used to receive 
>> HA_ERR_AUTOINC_READ_FAILED ("Failed to read auto-increment value from 
>> storage engine") now receive HA_ERR_AUTOINC_ERANGE ("Out of range 
>> value for column") which I find better (it's the error found in 
>> auto_increment.test). The engine is not really guilty after all.
> My first attempt to fix the problem was the same :)

Thanks a lot for considering the idea!

> The only problem I see is mysterious behavior of MAX_ULONGLONG value:
> 
> We can store MAX_ULONGLONG value explicitly:
> 
> +CREATE TABLE t1 (c1 BIGINT UNSIGNED AUTO_INCREMENT, PRIMARY KEY(c1)) 
> engine=MyISAM;
> +INSERT INTO t1 VALUES (18446744073709551615);
> +SELECT * FROM t1;
> +c1
> +18446744073709551615

I have tested this, and my idea does not break this. My idea doesn't 
touch the success/failure status of the branch of 
update_auto_increment() which handles explicitely specified values:
   if ((nr= table->next_number_field->val_int()) != 0 ||
       (table->auto_increment_field_not_null &&
       thd->variables.sql_mode & MODE_NO_AUTO_VALUE_ON_ZERO))
   {
     /*
       Update next_insert_id if we had already generated a value in this
       statement (case of INSERT VALUES(null),(3763),(null):
       the last NULL needs to insert 3764, not the value of the first 
NULL plus
       1).
     */
     adjust_next_insert_id_after_explicit_value(nr);
     insert_id_for_cur_row= 0; // didn't generate anything
     DBUG_RETURN(0);
   }

> We can store auto-generated MAX_ULONGLONG value in some cases:
> 
> +CREATE TABLE t1 (c1 BIGINT UNSIGNED AUTO_INCREMENT, PRIMARY KEY(c1)) 
> engine=MyISAM;
> +INSERT INTO t1 VALUES (18446744073709551613);
> +SET @@SESSION.AUTO_INCREMENT_INCREMENT=2;
> +INSERT INTO t1 VALUES (NULL);
> +SELECT * FROM t1;
> +c1
> +18446744073709551613
> +18446744073709551615

right

> And we can not store auto-generated MAX_ULONGLONG value in some other 
> cases:
> 
> +CREATE TABLE t1 (c1 BIGINT UNSIGNED AUTO_INCREMENT, PRIMARY KEY(c1)) 
> engine=MyISAM;
> +INSERT INTO t1 VALUES (18446744073709551614);
> +INSERT INTO t1 VALUES (NULL);
> +ERROR HY000: Failed to read auto-increment value from storage engine
> +SELECT * FROM t1;
> +c1
> +18446744073709551614
> 
> Btw last example looks strange for me as MAX_ULONGLONG is legal value.
> But comments in our code assert that if an engine returns MAX_ULONGLONG
> then we exit with error, so lets keep it as it is.

What happens in the existing code is that in
  INSERT INTO t1 VALUES (NULL);
for a MyISAM table, handler::get_auto_increment() is called, which 
returns "1 + the max value of the index". Not honouring "offset" or 
"increment" parameters.
So with:
  INSERT INTO t1 VALUES (18446744073709551614);
  INSERT INTO t1 VALUES (NULL);
when we insert NULL get_auto_increment() returns 18446744073709551614+1, 
==~(ulonglong)0 which is the documented error value of 
get_auto_increment() in the handler API (*), and it fails.
But with
  INSERT INTO t1 VALUES (18446744073709551613);
  INSERT INTO t1 VALUES (NULL);
when we insert NULL get_auto_increment() returns 18446744073709551613+1, 
then compute_next_insert_id() changes this to 18446744073709551615 (due 
to increment=2) and inserts it.
My idea only breaks that last case. But to me this is a detail: as an 
autogenerated 18446744073709551615 is already a problem in the case of 
18446744073709551614, and we don't plan to fix this, we can as well 
decide that the case of 18446744073709551613 can be broken as well, and 
break it intentionally, gaining simpler code (no new variable in class 
"handler", no handler API change).

Current situation:
"autogenerated 18446744073709551615 is insertable if current max value 
is less than 18446744073709551614, otherwise not".
Situation with my idea:
"autogenerated 18446744073709551615 is not insertable".

I can't imagine that a customer would rely on an autogenerated 
18446744073709551615 being insertable in some cases, knowing that it's 
already not insertable in other cases; we haven't had bug reports about 
the case where the query fails; customers probably don't run into this 
particular large value often. If we prevent insertion of 
18446744073709551615 in one more case, it doesn't change much the 
situation, it even makes it clearer to explain. In the current 
situation, customer would get an error at the next row (autogenerated 
18446744073709551616) anyway...

> Using your idea we will change server behavior for two first examples,
> it will throw 'Out of range' error.

I think only the second example will be broken.

 > My idea was to fix the problem and
> keep existing behavior even if this behavior looks strange and
> auto_inc_overflow variable allows to do it.

I understand. But I would like to discuss this; I find that just using 
~(ulonglong)0 as an error marker, without a new member in "handler", 
makes code simpler, which is good in the long run. And find that what we 
lose in functionality, is neglectible.
We can see what Evgeny, the second reviewer, thinks (cc:ing him).

(*) handler::update_auto_increment() interprets this value as "error 
from engine". InnoDB and NDB use this value to signal a problem happened 
in their get_auto_increment(). So this is a well-installed convention.
Thread
review of BUG#39828Guilhem Bichot29 Nov
Re: review of BUG#39828Guilhem Bichot1 Dec
  • Re: review of BUG#39828Evgeny Potemkin2 Dec