issue-907 : Debug assertion "last_val > 0" when auto_inc_inc > 1 and … #908
Conversation
There was a problem hiding this comment.
@lth has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| CREATE TABLE t1(c1 BIGINT UNSIGNED AUTO_INCREMENT, KEY (c1)) ENGINE=ROCKSDB AUTO_INCREMENT=18446744073709551604; | ||
| --eval SET @@session.auto_increment_increment=$increment, @@session.auto_increment_offset=$offset | ||
| --disable_query_log | ||
| --error 0, ER_AUTOINC_READ_FAILED, ER_WARN_DATA_OUT_OF_RANGE |
There was a problem hiding this comment.
Would be slightly nicer if this were in a while loop too.
There was a problem hiding this comment.
Yes, but it seems that mtr doesn't support nested while loops, or at least I couldn't get it to work :-\
| 18446744073709551610 | ||
| 18446744073709551611 | ||
| 18446744073709551612 | ||
| 18446744073709551613 |
There was a problem hiding this comment.
At first glance, it looks like this sequence failed before reaching 18446744073709551614, but it's really just because we didn't do enough inserts to hit the overflow case.
Can we fix this case?
There was a problem hiding this comment.
Yup, you are correct, I didn't execute enough inserts for the increment=1 cases to overflow.
| } while (!auto_incr.compare_exchange_weak(last_val, | ||
| std::min(new_val + 1, max_val))); | ||
| } while (!auto_incr.compare_exchange_weak( | ||
| last_val, std::min(new_val, max_val - 1) + 1)); |
There was a problem hiding this comment.
Can we extend the comment above to explain why we do this -1/+1 business? The original std::min expression was probably copied from the inc == 1 case.
…ullong_max underflow - Fixed a minor logic problem in ha_rocksdb::get_auto_increment when the auto_increment_increment is > 1 and we run into the condition where the next auto increment value is ullong_max+1, which is 0. When this occurs, the next call into ha_rocksdb::get_auto_increment will debug assert on 'last_val > 0'. - Added an autoincrement overflow test that runs through a 10*10 matrix of auto_increment_increment and auto_increment_offset values that all bump up against the ullong_max overflow. Without the logic fix in place, a vast majority of the combinations within the matrix would hit the debug assertion.
george-lorch
force-pushed
the
issue-907
branch
from
9928652
to
71121e7
Compare
|
@georgelorchpercona has updated the pull request. Re-import the pull request |
| CREATE TABLE t1(c1 BIGINT UNSIGNED AUTO_INCREMENT, KEY (c1)) ENGINE=ROCKSDB AUTO_INCREMENT=18446744073709551604; | ||
| SET @@session.auto_increment_increment=10, @@session.auto_increment_offset=1; | ||
| SELECT * FROM t1; | ||
| c1 |
There was a problem hiding this comment.
These "empty" selects were also a bit worrying so I looked into them. It looks like we actually return ER_WARN_DATA_OUT_OF_RANGE in this case, because compute_next_insert_id overflows. Maybe we can add a comment in the test explaining when we get that error?
…ullong_max underflow
Fixed a minor logic problem in ha_rocksdb::get_auto_increment when the
auto_increment_increment is > 1 and we run into the condition where
the next auto increment value is ullong_max+1, which is 0. When this occurs,
the next call into ha_rocksdb::get_auto_increment will debug assert on
'last_val > 0'.
Added an autoincrement overflow test that runs through a 10*10 matrix of
auto_increment_increment and auto_increment_offset values that all bump up
against the ullong_max overflow. Without the logic fix in place, a vast
majority of the combinations within the matrix would hit the debug assertion.