Skip to content

Commit

Permalink
issue-902 : Debug assertion 'max_val == std::numeric_limits<ulonglong… (
Browse files Browse the repository at this point in the history 
facebook#909)

Summary:
…>::max()' with auto inc manually set > than field limit

- Added extra check to see if current auto_inc value is greater than the fields
  max_value, and if so, return ullong_max instead of trying to find a new value.

- Updated autoinc_vars test with a simple sequence that would have hit the debug
  assertion prior to the fix but now illustrates proper (well, similar to
  innodb) behavior.
Pull Request resolved: facebook#909

Differential Revision: D13243938

Pulled By: lth

fbshipit-source-id: 3798f87
  • Loading branch information
@inikep
George O. Lorch III authored and inikep committed Sep 16, 2020
1 parent 0005ece commit be54239
Show file tree
Hide file tree
Showing 3 changed files with 108 additions and 63 deletions.
21 changes: 21 additions & 0 deletions mysql-test/suite/rocksdb/r/autoinc_vars.result
View file
Expand Up @@ -159,3 +159,24 @@ INSERT INTO t1 (a) VALUES (1);
UPDATE t1 SET pk = 3;
ALTER TABLE t1 AUTO_INCREMENT 2;
DROP TABLE t1;
#----------------------------------
# Issue #902 Debug assert in autoincrement with small field type
#----------------------------------
SET auto_increment_increment=100, auto_increment_offset=10;
CREATE TABLE t1(i INT AUTO_INCREMENT PRIMARY KEY) ENGINE=ROCKSDB AUTO_INCREMENT=18446744073709551615;
INSERT INTO t1 VALUES (NULL);
ERROR HY000: Failed to read auto-increment value from storage engine
SELECT * FROM t1;
i
ALTER TABLE t1 AUTO_INCREMENT=1;
INSERT INTO t1 VALUES (NULL);
SELECT * FROM t1;
i
10
ALTER TABLE t1 AUTO_INCREMENT=18446744073709551615;
INSERT INTO t1 VALUES (NULL);
ERROR HY000: Failed to read auto-increment value from storage engine
SELECT * FROM t1;
i
10
DROP TABLE t1;
20 changes: 20 additions & 0 deletions mysql-test/suite/rocksdb/t/autoinc_vars.test
View file
Expand Up @@ -124,3 +124,23 @@ INSERT INTO t1 (a) VALUES (1);
UPDATE t1 SET pk = 3;
ALTER TABLE t1 AUTO_INCREMENT 2;
DROP TABLE t1;

--echo #----------------------------------
--echo # Issue #902 Debug assert in autoincrement with small field type
--echo #----------------------------------

SET auto_increment_increment=100, auto_increment_offset=10;
CREATE TABLE t1(i INT AUTO_INCREMENT PRIMARY KEY) ENGINE=ROCKSDB AUTO_INCREMENT=18446744073709551615;
# ha_rocksdb::get_auto_increment would assert here
--error ER_AUTOINC_READ_FAILED
INSERT INTO t1 VALUES (NULL);
SELECT * FROM t1;
ALTER TABLE t1 AUTO_INCREMENT=1;
INSERT INTO t1 VALUES (NULL);
SELECT * FROM t1;
ALTER TABLE t1 AUTO_INCREMENT=18446744073709551615;
# ha_rocksdb::get_auto_increment would assert here
--error ER_AUTOINC_READ_FAILED
INSERT INTO t1 VALUES (NULL);
SELECT * FROM t1;
DROP TABLE t1;
130 changes: 67 additions & 63 deletions storage/rocksdb/ha_rocksdb.cc
View file
Expand Up @@ -11471,73 +11471,77 @@ void ha_rocksdb::get_auto_increment(ulonglong off, ulonglong inc,
// The next value can be more complicated if either 'inc' or 'off' is not 1
ulonglong last_val = auto_incr;

// Loop until we can correctly update the atomic value
do {
DBUG_ASSERT(last_val > 0);
// Calculate the next value in the auto increment series: offset
// + N * increment where N is 0, 1, 2, ...
//
// For further information please visit:
// http://dev.mysql.com/doc/refman/5.7/en/replication-options-master.html
//
// The following is confusing so here is an explanation:
// To get the next number in the sequence above you subtract out the
// offset, calculate the next sequence (N * increment) and then add the
// offset back in.
//
// The additions are rearranged to avoid overflow. The following is
// equivalent to (last_val - 1 + inc - off) / inc. This uses the fact
// that (a+b)/c = a/c + b/c + (a%c + b%c)/c. To show why:
//
// (a+b)/c
// = (a - a%c + a%c + b - b%c + b%c) / c
// = (a - a%c) / c + (b - b%c) / c + (a%c + b%c) / c
// = a/c + b/c + (a%c + b%c) / c
//
// Now, substitute a = last_val - 1, b = inc - off, c = inc to get the
// following statement.
ulonglong n =
(last_val - 1) / inc + ((last_val - 1) % inc + inc - off) / inc;

// Check if n * inc + off will overflow. This can only happen if we have
// an UNSIGNED BIGINT field.
if (n > (std::numeric_limits<ulonglong>::max() - off) / inc) {
DBUG_ASSERT(max_val == std::numeric_limits<ulonglong>::max());
// The 'last_val' value is already equal to or larger than the largest
// value in the sequence. Continuing would wrap around (technically
// the behavior would be undefined). What should we do?
// We could:
// 1) set the new value to the last possible number in our sequence
// as described above. The problem with this is that this
// number could be smaller than a value in an existing row.
// 2) set the new value to the largest possible number. This number
// may not be in our sequence, but it is guaranteed to be equal
// to or larger than any other value already inserted.
if (last_val > max_val) {
new_val = std::numeric_limits<ulonglong>::max();
} else {
// Loop until we can correctly update the atomic value
do {
DBUG_ASSERT(last_val > 0);
// Calculate the next value in the auto increment series: offset
// + N * increment where N is 0, 1, 2, ...
//
// For now I'm going to take option 2.
// For further information please visit:
// http://dev.mysql.com/doc/refman/5.7/en/replication-options-master.html
//
// Returning ULLONG_MAX from get_auto_increment will cause the SQL
// layer to fail with ER_AUTOINC_READ_FAILED. This means that due to
// the SE API for get_auto_increment, inserts will fail with
// ER_AUTOINC_READ_FAILED if the column is UNSIGNED BIGINT, but
// inserts will fail with ER_DUP_ENTRY for other types (or no failure
// if the column is in a non-unique SK).
new_val = std::numeric_limits<ulonglong>::max();
auto_incr = new_val; // Store the largest value into auto_incr
break;
}
// The following is confusing so here is an explanation:
// To get the next number in the sequence above you subtract out the
// offset, calculate the next sequence (N * increment) and then add the
// offset back in.
//
// The additions are rearranged to avoid overflow. The following is
// equivalent to (last_val - 1 + inc - off) / inc. This uses the fact
// that (a+b)/c = a/c + b/c + (a%c + b%c)/c. To show why:
//
// (a+b)/c
// = (a - a%c + a%c + b - b%c + b%c) / c
// = (a - a%c) / c + (b - b%c) / c + (a%c + b%c) / c
// = a/c + b/c + (a%c + b%c) / c
//
// Now, substitute a = last_val - 1, b = inc - off, c = inc to get the
// following statement.
ulonglong n =
(last_val - 1) / inc + ((last_val - 1) % inc + inc - off) / inc;

// Check if n * inc + off will overflow. This can only happen if we have
// an UNSIGNED BIGINT field.
if (n > (std::numeric_limits<ulonglong>::max() - off) / inc) {
DBUG_ASSERT(max_val == std::numeric_limits<ulonglong>::max());
// The 'last_val' value is already equal to or larger than the largest
// value in the sequence. Continuing would wrap around (technically
// the behavior would be undefined). What should we do?
// We could:
// 1) set the new value to the last possible number in our sequence
// as described above. The problem with this is that this
// number could be smaller than a value in an existing row.
// 2) set the new value to the largest possible number. This number
// may not be in our sequence, but it is guaranteed to be equal
// to or larger than any other value already inserted.
//
// For now I'm going to take option 2.
//
// Returning ULLONG_MAX from get_auto_increment will cause the SQL
// layer to fail with ER_AUTOINC_READ_FAILED. This means that due to
// the SE API for get_auto_increment, inserts will fail with
// ER_AUTOINC_READ_FAILED if the column is UNSIGNED BIGINT, but
// inserts will fail with ER_DUP_ENTRY for other types (or no failure
// if the column is in a non-unique SK).
new_val = std::numeric_limits<ulonglong>::max();
auto_incr = new_val; // Store the largest value into auto_incr
break;
}

new_val = n * inc + off;
new_val = n * inc + off;

// Attempt to store the new value (plus 1 since m_auto_incr_val contains
// the next available value) into the atomic value. If the current
// value no longer matches what we have in 'last_val' this will fail and
// we will repeat the loop (`last_val` will automatically get updated
// with the current value).
//
// See above explanation for inc == 1 for why we use std::min.
} while (!auto_incr.compare_exchange_weak(last_val,
std::min(new_val + 1, max_val)));
// Attempt to store the new value (plus 1 since m_auto_incr_val contains
// the next available value) into the atomic value. If the current
// value no longer matches what we have in 'last_val' this will fail and
// we will repeat the loop (`last_val` will automatically get updated
// with the current value).
//
// See above explanation for inc == 1 for why we use std::min.
} while (!auto_incr.compare_exchange_weak(
last_val, std::min(new_val + 1, max_val)));
}
}

*first_value = new_val;
Expand Down

0 comments on commit be54239

Please sign in to comment.