Fix CommitTimeWriteBatch without prepare error in MyRocks DDL

Summary:
This is discovered with rqg_runtime test - it issues a DDL `alter table t1 add column a int` operation that implicit commits current transaction and starts a new transaction for the table copy operation with temp tables.

In this case, the following event happens:
1. Start of every command we reset binlog position in THD to NULL/0
2. DDL calls trans_commit_implicit to implicitly commit current on-going transaction. The transaction go through 2pc as usual, updates binlog position in THD, and writes it to RocksDB as part of CommitTimeWriteBatch during commit
3. DDL starts a new transaction for the in-memory temp table copy operation that doesn't go through binlog (which makes sense since it is purely in memory).
4. DDL commits this transaction and go to ha_commit_low directly without going through ordered_commit
5. As a result, rocksdb_prepare is skippped, since this isn't a MySQL multi-engine 2pc transaction
6. As part of rocksdb_commit, we get the binlog position from earlier transaction, and tries to update with CommitTimeWriteBatch.
7. When RocksDB commits, it notices that this isn't a RocksDB 2pc (prepare isn't called) and yet CommitTimeWriteBatch has non-0 items, and returns a failure

I can see 3 issues here:
1. We should not leak binlog position in THD when a statement implicitly commits current transaction. Usually this is OK if the next transaction is a MySQL 2pc transaction since it'll get overwritten
2. In a non MySQL 2pc transaction where binlog isn't updated and prepare haven't been called, we should never return and write to a CommitTimeWriteBatch.
3. We should not update binlog position in a non-2pc operation with leaked binlog position

This fixes 1) & 3) by clearing binlog position in implicit commit case. 2) should be always correct once 1) is fixed, and if ever there is an issue RocksDB would report an error.

Reviewed By: lloyd

Differential Revision: D19895639

Author: yizhang82

mysql-test/collections/disabled_rocksdb.def

@@ -40,6 +40,4 @@ rocksdb.com_rpc_tx : BUG#0000 detached session is missing in 8.0
 # Misc bugs
 rocksdb.persistent_cache : BUG#0000 only repros with other tests running
 rocksdb.index_merge_rocksdb2 : BUG#0000 query plan is different between 8.0 and 5.6
-rocksdb.rqg_runtime: BUG#0000 commit without prepare
-rocksdb.rqg_transactions : BUG#0000 commit without prepare
 rocksdb.add_index_inplace : BUG#0000 : ALTER TABLE ALGORITHM=INPLACE fails

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

@@ -0,0 +1,20 @@
+create table t1 (pk int primary key, a int);
+create table t2 (pk int primary key, a int);
+insert into t1 values (1, 1), (2, 2);
+start transaction;
+insert into t2 values (1, 1);
+alter table t1 add column b int;
+select * from t1;
+pk	a	b
+1	1	NULL
+2	2	NULL
+drop table t1;
+drop table t2;
+create table t1 (pk int primary key);
+insert into t1 values (1);
+start transaction;
+insert into t1 values (2), (3), (1);
+ERROR 23000: Duplicate entry '1' for key 't1.PRIMARY'
+savepoint a;
+commit;
+drop table t1;

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

@@ -2,6 +2,7 @@ call mtr.add_suppression("Did not write failed ");
 call mtr.add_suppression("Can't open and lock privilege tables");
 call mtr.add_suppression("Attempt to delete the trigger file");
 SET @ORIG_EVENT_SCHEDULER = @@EVENT_SCHEDULER;
+SET @ORIG_INNODB_FILE_PER_TABLE = @@INNODB_FILE_PER_TABLE;
 CREATE TABLE user_temp SELECT * FROM mysql.user LIMIT 0;
 INSERT user_temp SELECT * FROM mysql.user;
 CREATE TABLE tables_priv_temp SELECT * FROM mysql.tables_priv LIMIT 0;
@@ -27,4 +28,5 @@ DROP TABLE tables_priv_temp;
 DROP TABLE IF EXISTS test.executors;
 DROP DATABASE IF EXISTS testdb_N;
 DROP DATABASE IF EXISTS testdb_S;
+SET GLOBAL INNODB_FILE_PER_TABLE = @ORIG_INNODB_FILE_PER_TABLE;
 SET GLOBAL EVENT_SCHEDULER = @ORIG_EVENT_SCHEDULER;

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

@@ -0,0 +1,35 @@
+--source include/have_rocksdb.inc
+
+# Scenario #1
+# ALTER TABLE implicit commit
+create table t1 (pk int primary key, a int);
+create table t2 (pk int primary key, a int);
+insert into t1 values (1, 1), (2, 2);
+start transaction;
+insert into t2 values (1, 1);
+
+# Bug: triggers commit without prepare error in RocksDB
+alter table t1 add column b int;
+--sorted_result
+select * from t1;
+
+drop table t1;
+drop table t2;
+
+# Scenario #2
+# Incorrect write count in statement rollback in transaction
+create table t1 (pk int primary key);
+insert into t1 values (1);
+
+start transaction;
+
+--error ER_DUP_ENTRY
+insert into t1 values (2), (3), (1);
+
+savepoint a;
+
+# Bug: triggers commit without prepare error in RocksDB
+commit;
+
+drop table t1;
+

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

@@ -10,6 +10,9 @@ call mtr.add_suppression("Attempt to delete the trigger file");
 
 SET @ORIG_EVENT_SCHEDULER = @@EVENT_SCHEDULER;
 
+# Following tests may update INNODB_FILE_PER_TABLE
+SET @ORIG_INNODB_FILE_PER_TABLE = @@INNODB_FILE_PER_TABLE;
+
 # mysql.user and mysql.tables_priv are modified by the
 # tests, so they need to be restored to the original
 # state.
@@ -62,4 +65,5 @@ DROP DATABASE IF EXISTS testdb_N;
 DROP DATABASE IF EXISTS testdb_S;
 --enable_warnings
 
+SET GLOBAL INNODB_FILE_PER_TABLE = @ORIG_INNODB_FILE_PER_TABLE;
 SET GLOBAL EVENT_SCHEDULER = @ORIG_EVENT_SCHEDULER;

sql/binlog.cc

@@ -9250,6 +9250,15 @@ TC_LOG::enum_result MYSQL_BIN_LOG::commit(THD *thd, bool all) {
       (void)RUN_HOOK(transaction, after_commit, (thd, all));
     }
   } else if (!skip_commit) {
+    /*
+      We only set engine binlog position in ordered_commit path flush phase
+      and not all transactions go through them (such as table copy in DDL).
+      So in cases where a DDL statement implicitly commits earlier transaction
+      and starting a new one, the new transaction could be "leaking" the
+      engine binlog pos. In order to avoid that and accidentally overwrite
+      binlog position with previous location, we reset it here.
+    */
+    thd->set_trans_pos(nullptr, 0);
     if (trx_coordinator::commit_in_engines(thd, all))
       return RESULT_INCONSISTENT;
   }

storage/rocksdb/ha_rocksdb.cc

@@ -3614,7 +3614,7 @@ class Rdb_transaction {
         statement start) because setting a savepoint is cheap.
       */
       do_set_savepoint();
-      m_writes_at_last_savepoint = m_write_count;
+      m_write_count = m_writes_at_last_savepoint;
     }
   }