Track number of tables being used and autocommit when it hits 0. (fac…

…ebook#625) (facebook#625)

Summary:
There is a bug in myrocks::ha_rocksdb::external_lock where releasing a
lock on the table triggers a commit of the transaction. The query plan
for index_merge also acquires ::external_lock on a table, in addition to
the ::external lock acquired during the update query. However, the
index_merge's lock is released early (when the SQL_SELECT object is
destroyed), which would trigger a commit of the Rdb_transaction before
sql_update calls prepare.

This would result in a crash when sql_update calls rocksdb_prepare.  The
transaction is incorrectly committed and the prepare segfaults on a null
pointer.

The solution is to follow the InnoDB one, reference the number of
external lock calls through n_tables_in_use.
Closes facebook#625

Differential Revision: D5084152 (facebook@a50fa91)

Pulled By: hermanlee

fbshipit-source-id: 130468969ec

mysql-test/suite/rocksdb/r/index_merge_rocksdb.result

@@ -0,0 +1,25 @@
+CREATE TABLE t1
+(
+/* fields/keys for row retrieval tests */
+key1 INT,
+key2 INT,
+key3 INT,
+key4 INT,
+/* make rows much bigger then keys */
+filler1 CHAR(200),
+KEY(key1),
+KEY(key2)
+) ENGINE=ROCKSDB;
+CREATE TABLE t0 AS SELECT * FROM t1;
+# Printing of many insert into t0 values (....) disabled.
+# Printing of many insert into t1 select .... from t0 disabled.
+# Printing of many insert into t1 (...) values (....) disabled.
+SELECT COUNT(*) FROM t1;
+COUNT(*)
+7201
+SET GLOBAL rocksdb_force_flush_memtable_now = 1;
+EXPLAIN UPDATE t1 SET filler1='to be deleted' WHERE key1=100 AND key2=100;
+id	select_type	table	type	possible_keys	key	key_len	ref	rows	Extra
+1	SIMPLE	t1	index_merge	key1,key2	key1,key2	5,5	NULL	#	Using intersect(key1,key2); Using where
+UPDATE t1 SET filler1='to be deleted' WHERE key1=100 and key2=100;
+DROP TABLE t0, t1;

mysql-test/suite/rocksdb/t/index_merge_rocksdb-master.opt

@@ -0,0 +1 @@
+--rocksdb_strict_collation_check=off --binlog_format=row --log-bin

mysql-test/suite/rocksdb/t/index_merge_rocksdb.test

@@ -0,0 +1,85 @@
+--source include/have_rocksdb.inc
+
+#
+# index_merge_rocksdb test copied over from index_merge_ror.inc
+#
+# Triggers issue # https://github.com/facebook/mysql-5.6/issues/604
+
+CREATE TABLE t1
+(
+  /* fields/keys for row retrieval tests */
+  key1 INT,
+  key2 INT,
+  key3 INT,
+  key4 INT,
+
+  /* make rows much bigger then keys */
+  filler1 CHAR(200),
+
+  KEY(key1),
+  KEY(key2)
+) ENGINE=ROCKSDB;
+
+# fill table
+CREATE TABLE t0 AS SELECT * FROM t1;
+--disable_query_log
+--echo # Printing of many insert into t0 values (....) disabled.
+let $cnt=100;
+while ($cnt)
+{
+  eval INSERT INTO t0 VALUES (0, 0, 0, 0, 'data1');
+  dec $cnt;
+}
+
+--echo # Printing of many insert into t1 select .... from t0 disabled.
+let $1=4;
+while ($1)
+{
+  let $2=4;
+  while ($2)
+  {
+    let $3=4;
+    while ($3)
+    {
+      eval INSERT INTO t1 SELECT key1, key2, key3, key4, filler1 FROM t0;
+      dec $3;
+    }
+    dec $2;
+  }
+  dec $1;
+}
+
+--echo # Printing of many insert into t1 (...) values (....) disabled.
+# Row retrieval tests
+# -1 is used for values 'out of any range we are using'
+# insert enough rows for index intersection to be used for (key1,key2)
+INSERT INTO t1 (key1, key2, key3, key4, filler1) VALUES (100, 100, 100, 100,'key1-key2-key3-key4');
+let $cnt=400;
+while ($cnt)
+{
+  eval INSERT INTO t1 (key1, key2, key3, key4, filler1) VALUES (100, -1, 100, -1,'key1-key3');
+  dec $cnt;
+}
+let $cnt=400;
+while ($cnt)
+{
+  eval INSERT INTO t1 (key1, key2, key3, key4, filler1) VALUES (-1, 100, -1, 100,'key2-key4');
+  dec $cnt;
+}
+--enable_query_log
+
+SELECT COUNT(*) FROM t1;
+
+-- disable_query_log
+-- disable_result_log
+ANALYZE TABLE t1;
+-- enable_result_log
+-- enable_query_log
+
+SET GLOBAL rocksdb_force_flush_memtable_now = 1;
+
+--replace_column 9 #
+EXPLAIN UPDATE t1 SET filler1='to be deleted' WHERE key1=100 AND key2=100;
+UPDATE t1 SET filler1='to be deleted' WHERE key1=100 and key2=100;
+
+DROP TABLE t0, t1;

storage/rocksdb/ha_rocksdb.cc

@@ -1581,6 +1581,12 @@ class Rdb_transaction {
   int64_t m_snapshot_timestamp = 0;
   bool m_ddl_transaction;
 
+  /*
+    Tracks the number of tables in use through external_lock.
+    This should not be reset during start_tx().
+  */
+  int64_t m_n_mysql_tables_in_use = 0;
+
   /*
     for distinction between rdb_transaction_impl and rdb_writebatch_impl
     when using walk tx list
@@ -8900,7 +8906,9 @@ int ha_rocksdb::external_lock(THD *const thd, int lock_type) {
     Rdb_transaction *const tx = get_or_create_tx(thd);
 
     tx->io_perf_end_and_record(&m_io_perf);
-    if (!my_core::thd_test_options(thd, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) {
+    tx->m_n_mysql_tables_in_use--;
+    if (tx->m_n_mysql_tables_in_use == 0 &&
+        !my_core::thd_test_options(thd, OPTION_NOT_AUTOCOMMIT | OPTION_BEGIN)) {
       /*
         Do like InnoDB: when we get here, it's time to commit a
         single-statement transaction.
@@ -8962,6 +8970,7 @@ int ha_rocksdb::external_lock(THD *const thd, int lock_type) {
         tx->m_ddl_transaction = true;
       }
     }
+    tx->m_n_mysql_tables_in_use++;
     rocksdb_register_tx(rocksdb_hton, thd, tx);
     tx->io_perf_start(&m_io_perf);
   }