Update AUTO_INCREMENT in the UPDATE statement #868

minggr · 2018-08-09T07:55:50Z

Currently RocksDB engine doesn't update AUTO_INCREMENT in the UPDATE statement.
For example,

CREATE TABLE t1 (pk INT AUTO_INCREMENT, a INT, PRIMARY KEY(pk)) ENGINE=RocksDB;
INSERT INTO t1 (a) VALUES (1);
UPDATE t1 SET pk = 3; ==> AUTO_INCREMENT should be updated to 4.

Without this fix, it hits the Assertion `dd_val >= last_val' failed in
myrocks::ha_rocksdb::load_auto_incr_value_from_index.

Please see detail at:
https://jira.mariadb.org/browse/MDEV-16703

facebook-github-bot · 2018-08-09T07:56:14Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

facebook-github-bot · 2018-08-09T07:58:42Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

lth

Can we add an mtr test as well?

lth · 2018-08-13T23:13:57Z

storage/rocksdb/ha_rocksdb.cc

+    return;
+  }
+
+  for (i = 0; i < table->s->fields; i++) {


Do we have to do the for loop? Doesn't table->s->next_number_index contain the field we want?

Not always, for example,

CREATE TABLE t1 (a INT, pk INT AUTO_INCREMENT, PRIMARY KEY(pk)) ENGINE=RocksDB;
INSERT INTO t1 (a) VALUES (20);

(gdb) p table->s->next_number_index
$18 = 0
(gdb) p table->field[0]->val_uint()
$19 = 20

Actually, I got confused on what table->s->next_number_index meant. It is actually telling us which index the autoincrement is one (not the "index" of the field array). In this case, it's 0 because it is on primary key.

Looking at ha_rocksdb::load_auto_incr_value_from_index, we just assume that the first column of the index is the autoincrement field. Looks like something similar is assumed in InnoDB (row_search_autoinc_read_column is always called with col_no == 0).

How about the updated patch?

lth · 2018-08-13T23:28:46Z

storage/rocksdb/ha_rocksdb.cc

+     A value for an AUTO_INCREMENT column
+     was specified in the UPDATE statement.
+    */
+    update_auto_incr_val_from_field();


We already call update_auto_incr_val_from_field from update_pk, but it's not working as expected because it's using the 'old' row instead of the new one. I think a better fix is to unify these paths.

CREATE TABLE t1 (pk INT AUTO_INCREMENT, a INT, PRIMARY KEY(pk)) ENGINE=RocksDB;
INSERT INTO t1 (a) VALUES (1);
UPDATE t1 SET pk = 3;

table->next_number_field is null so update_auto_incr_val_from_field() is not called in update_pk()

(gdb) list 9408 } else { 9409 bytes_written = row_info.old_pk_slice.size(); 9410 } 9411 } 9412 9413 if (table->next_number_field) { 9414 update_auto_incr_val_from_field(); 9415 } 9416 9417 int rc = HA_EXIT_SUCCESS; (gdb) p table->next_number_field $2 = (Field *) 0x0

Can we merge these two paths, since I think they're trying to do the same thing?

Also, table->next_number_field isn't nullptr for me, but it is using the old row, which is why this bug reproduces for me.

(gdb) list 9395 bytes_written = row_info.old_pk_slice.size(); 9396 } 9397 } 9398 9399 if (table->next_number_field) { 9400 update_auto_incr_val_from_field(); 9401 } 9402 9403 int rc = HA_EXIT_SUCCESS; 9404 rocksdb::Slice value_slice; (gdb) p table->next_number_field $2 = (Field *) 0x7f2a9c825150

That's weird. Can you set a memory watchpoint to show the call stack that sets table->next_number_field?

Breakpoint 13, mysql_update (thd=0x2c8f850, table_list=0x7fffdc005710, fields=..., values=..., conds=0x0, order_num=0, order=0x0, limit=18446744073709551615, handle_duplicates=DUP_ERROR, ignore=false, found_return=0x7ffff7e8d4a0, updated_return=0x7ffff7e8d498) at /workspace/mysql/sql/sql_update.cc:266 266 if (!table_list->updatable) (gdb) list 261 } 262 263 THD_STAGE_INFO(thd, stage_init); 264 table= table_list->table; 265 266 if (!table_list->updatable) 267 { 268 my_error(ER_NON_UPDATABLE_TABLE, MYF(0), table_list->alias, "UPDATE"); 269 DBUG_RETURN(1); 270 } (gdb) p table $53 = (TABLE *) 0x7fffdc021180 (gdb) watch ((TABLE *) 0x7fffdc021180)->next_number_field Hardware watchpoint 14: ((TABLE *) 0x7fffdc021180)->next_number_field

It's not clear to me how to merge these two paths. Can we simply do below?

--- a/storage/rocksdb/ha_rocksdb.cc +++ b/storage/rocksdb/ha_rocksdb.cc @@ -9410,7 +9410,7 @@ int ha_rocksdb::update_pk(const Rdb_key_def &kd, } } - if (table->next_number_field) { + if (table->found_next_number_field) { update_auto_incr_val_from_field(); }

Checking found_next_number_field will probably be good enough for the fix. You might need to change update_auto_incr_val_from_field to look at new row.

For some reason, I'm seeing nullptr now for next_number_field, so I'm not sure how I got the previous results in the first place...

Nice. I'll update the patch and also add a mtr test.

Patch updated.
In update_auto_incr_val_from_field(),
field = table->key_info[table->s->next_number_index].key_part[0].field;
field->val_int() is already new AUTO_INCREMENT value.

lth · 2018-08-15T01:07:37Z

Can we add an mtr test as well?

minggr · 2018-08-15T03:26:38Z

Yes, I'll add a mtr test.

minggr · 2018-08-15T07:05:44Z

What are the correct parameters to run mtr test?
I tried as below,

./mtr --mysqld="--default-tmp-storage-engine=MyISAM --rocksdb --skip-innodb --default-storage-engine=RocksDB"  --suite=rocksdb

But error log shows:

2018-08-15 15:02:32 76594 [Note] Plugin 'ROCKSDB' is disabled.
2018-08-15 15:02:32 76594 [Note] Plugin 'FEDERATED' is disabled.
2018-08-15 15:02:32 76594 [Note] InnoDB: Using atomics to ref count buffer pool pages
2018-08-15 15:02:32 7f34b2cd2880 InnoDB: !!!!!!!! UNIV_DEBUG switched on !!!!!!!!!
2018-08-15 15:02:32 7f34b2cd2880 InnoDB: !!!!!!!! UNIV_SYNC_DEBUG switched on !!!!!!!!!
2018-08-15 15:02:32 76594 [Note] InnoDB: The InnoDB memory heap is disabled
2018-08-15 15:02:32 76594 [Note] InnoDB: Mutexes and rw_locks use GCC atomic builtins
2018-08-15 15:02:32 76594 [Note] InnoDB: Memory barrier is not used
2018-08-15 15:02:32 76594 [Note] InnoDB: Compressed tables use zlib 1.2.3
2018-08-15 15:02:32 76594 [Note] InnoDB: Using Linux native AIO

spetrunia · 2018-08-15T14:45:44Z

For me running tests like this works:

./mysql-test-run --mysqld=--default-storage-engine=rocksdb \
 --mysqld=--skip-innodb --mysqld=--default-tmp-storage-engine=MyISAM \
 --mysqld=--rocksdb  --suite=rocksdb

that is, mysqld arguments go one by one

minggr · 2018-08-15T14:58:43Z

Yes, that works. Thanks.

https://jira.mariadb.org/browse/MDEV-16703 Currently RocksDB engine doesn't update AUTO_INCREMENT in the UPDATE statement. For example, CREATE TABLE t1 (pk INT AUTO_INCREMENT, a INT, PRIMARY KEY(pk)) ENGINE=RocksDB; INSERT INTO t1 (a) VALUES (1); UPDATE t1 SET pk = 3; ==> AUTO_INCREMENT should be updated to 4.

facebook-github-bot

lth has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

lth

Thanks for fixing this!

Summary: Currently RocksDB engine doesn't update AUTO_INCREMENT in the UPDATE statement. For example, CREATE TABLE t1 (pk INT AUTO_INCREMENT, a INT, PRIMARY KEY(pk)) ENGINE=RocksDB; INSERT INTO t1 (a) VALUES (1); UPDATE t1 SET pk = 3; ==> AUTO_INCREMENT should be updated to 4. Without this fix, it hits the Assertion `dd_val >= last_val' failed in myrocks::ha_rocksdb::load_auto_incr_value_from_index. Please see detail at: https://jira.mariadb.org/browse/MDEV-16703 Pull Request resolved: facebook#868 GitHub Author: Ming Lin <[email protected]> Test Plan: Imported from GitHub, without a `Test Plan:` line. Reviewers: svcscm Reviewed By: svcscm Subscribers: [email protected] Differential Revision: https://phabricator.intern.facebook.com/D9360242 Signature: 9360242:1534435395:01c49a6b0cf6cf30991bfa7f6d26c8a622111292

Upstream commit ID : fb-mysql-5.6.35/f8ea6852be5484ed15ffbac844b3719673f0f057 PS-5217 : Merge fb-prod201803 Summary: Currently RocksDB engine doesn't update AUTO_INCREMENT in the UPDATE statement. For example, CREATE TABLE t1 (pk INT AUTO_INCREMENT, a INT, PRIMARY KEY(pk)) ENGINE=RocksDB; INSERT INTO t1 (a) VALUES (1); UPDATE t1 SET pk = 3; ==> AUTO_INCREMENT should be updated to 4. Without this fix, it hits the Assertion `dd_val >= last_val' failed in myrocks::ha_rocksdb::load_auto_incr_value_from_index. Please see detail at: https://jira.mariadb.org/browse/MDEV-16703 Pull Request resolved: facebook/mysql-5.6#868 Differential Revision: D9360242 Pulled By: lth fbshipit-source-id: 70ee31cb90f

Summary: Currently RocksDB engine doesn't update AUTO_INCREMENT in the UPDATE statement. For example, CREATE TABLE t1 (pk INT AUTO_INCREMENT, a INT, PRIMARY KEY(pk)) ENGINE=RocksDB; INSERT INTO t1 (a) VALUES (1); UPDATE t1 SET pk = 3; ==> AUTO_INCREMENT should be updated to 4. Without this fix, it hits the Assertion `dd_val >= last_val' failed in myrocks::ha_rocksdb::load_auto_incr_value_from_index. Please see detail at: https://jira.mariadb.org/browse/MDEV-16703 Pull Request resolved: #868 Differential Revision: D9360242 Pulled By: lth fbshipit-source-id: 664dd17

Summary: Currently RocksDB engine doesn't update AUTO_INCREMENT in the UPDATE statement. For example, CREATE TABLE t1 (pk INT AUTO_INCREMENT, a INT, PRIMARY KEY(pk)) ENGINE=RocksDB; INSERT INTO t1 (a) VALUES (1); UPDATE t1 SET pk = 3; ==> AUTO_INCREMENT should be updated to 4. Without this fix, it hits the Assertion `dd_val >= last_val' failed in myrocks::ha_rocksdb::load_auto_incr_value_from_index. Please see detail at: https://jira.mariadb.org/browse/MDEV-16703 Pull Request resolved: facebook#868 Differential Revision: D9360242 Pulled By: lth fbshipit-source-id: 664dd17

Upstream commit ID : fb-mysql-5.6.35/f8ea6852be5484ed15ffbac844b3719673f0f057 PS-5217 : Merge fb-prod201803 Summary: Currently RocksDB engine doesn't update AUTO_INCREMENT in the UPDATE statement. For example, CREATE TABLE t1 (pk INT AUTO_INCREMENT, a INT, PRIMARY KEY(pk)) ENGINE=RocksDB; INSERT INTO t1 (a) VALUES (1); UPDATE t1 SET pk = 3; ==> AUTO_INCREMENT should be updated to 4. Without this fix, it hits the Assertion `dd_val >= last_val' failed in myrocks::ha_rocksdb::load_auto_incr_value_from_index. Please see detail at: https://jira.mariadb.org/browse/MDEV-16703 Pull Request resolved: facebook/mysql-5.6#868 Differential Revision: D9360242 Pulled By: lth fbshipit-source-id: 70ee31cb90f

facebook-github-bot added the CLA Signed label Aug 9, 2018

spetrunia mentioned this pull request Aug 13, 2018

Assertion `dd_val >= last_val' failed in myrocks::ha_rocksdb::load_auto_incr_value_from_index #869

Closed

lth closed this Aug 13, 2018

lth reopened this Aug 13, 2018

lth suggested changes Aug 13, 2018

View reviewed changes

minggr force-pushed the fix-auto-inc branch from 205b34f to f4ddaa5 Compare August 15, 2018 00:11

minggr force-pushed the fix-auto-inc branch from f4ddaa5 to e446d99 Compare August 16, 2018 06:06

minggr force-pushed the fix-auto-inc branch from e446d99 to 2036e54 Compare August 16, 2018 06:09

facebook-github-bot reviewed Aug 16, 2018

View reviewed changes

lth approved these changes Aug 16, 2018

View reviewed changes

facebook-github-bot closed this in f8ea685 Aug 16, 2018

lth mentioned this pull request Sep 28, 2018

Fix issue #878: Descending scans from reverse cf return no results #879

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update AUTO_INCREMENT in the UPDATE statement #868

Update AUTO_INCREMENT in the UPDATE statement #868

minggr commented Aug 9, 2018

facebook-github-bot commented Aug 9, 2018

facebook-github-bot commented Aug 9, 2018

lth left a comment

lth Aug 13, 2018

minggr Aug 14, 2018

lth Aug 14, 2018

minggr Aug 15, 2018

lth Aug 13, 2018

minggr Aug 15, 2018

lth Aug 15, 2018

lth Aug 15, 2018

minggr Aug 15, 2018

lth Aug 15, 2018

minggr Aug 15, 2018

minggr Aug 16, 2018

lth commented Aug 15, 2018

minggr commented Aug 15, 2018

minggr commented Aug 15, 2018

spetrunia commented Aug 15, 2018 •

edited

Loading

minggr commented Aug 15, 2018

facebook-github-bot left a comment

lth left a comment

Update AUTO_INCREMENT in the UPDATE statement #868

Update AUTO_INCREMENT in the UPDATE statement #868

Conversation

minggr commented Aug 9, 2018

facebook-github-bot commented Aug 9, 2018

facebook-github-bot commented Aug 9, 2018

lth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lth commented Aug 15, 2018

minggr commented Aug 15, 2018

minggr commented Aug 15, 2018

spetrunia commented Aug 15, 2018 • edited Loading

minggr commented Aug 15, 2018

facebook-github-bot left a comment

Choose a reason for hiding this comment

lth left a comment

Choose a reason for hiding this comment

spetrunia commented Aug 15, 2018 •

edited

Loading