revision-id: 5736482c572b443dd8e624b342b5ef7a7c21f26c parent(s): bd344d9f4ec384020f2133eb13f55272818340eb committer: Sergei Petrunia branch nick: mysql-5.6-rocksdb-spetrunia timestamp: 2018-06-11 19:02:54 +0300 message: Issue #834: Server crash in print_keydup_error / key_unpack or unexpected ER_DUP_KEY The issue is caused by a combination of two factors: 1. Rdb_ddl_manager::rename() loses the value of m_hidden_pk_val. new object used to get 0, which means "not loaded from the db yet". 2. ha_rocksdb::load_hidden_pk_value() uses current transaction (and its snapshot) when loading hidden PK value from disk. This may cause it to load an out-of-date value. This patch fixes #1, which is sufficient to fix the issue. --- .../suite/rocksdb/r/allow_no_primary_key.result | 31 ++++++++++++++++++++++ .../suite/rocksdb/t/allow_no_primary_key.test | 28 +++++++++++++++++++ storage/rocksdb/ha_rocksdb.cc | 23 ++++++++++------ storage/rocksdb/rdb_datadic.cc | 3 +++ 4 files changed, 77 insertions(+), 8 deletions(-) diff --git a/mysql-test/suite/rocksdb/r/allow_no_primary_key.result b/mysql-test/suite/rocksdb/r/allow_no_primary_key.result index d86792a..45b0ca7 100644 --- a/mysql-test/suite/rocksdb/r/allow_no_primary_key.result +++ b/mysql-test/suite/rocksdb/r/allow_no_primary_key.result @@ -262,3 +262,34 @@ SELECT * FROM t1; a b 36 foo DROP TABLE t1; +# +# Issue #834/MDEV-15304 ALTER TABLE table_with_hidden_pk causes Can't +# write; duplicate key in table error and/or crash +# +CREATE TABLE t1 (a INT, KEY(a)) ENGINE=RocksDB; +INSERT INTO t1 VALUES (1),(1+1); +create table t2 (a int); +insert into t2 values (10),(20),(30); +BEGIN; +select * from t2; +a +10 +20 +30 +connect con1,localhost,root,,; +connection con1; +alter table t1 force; +connection default; +select * from t1; +a +connection con1; +insert into t1 values (100); +select * from t1; +a +1 +2 +100 +disconnect con1; +connection default; +rollback; +drop table t1,t2; diff --git a/mysql-test/suite/rocksdb/t/allow_no_primary_key.test b/mysql-test/suite/rocksdb/t/allow_no_primary_key.test index 2a064dc..5f2a37f 100644 --- a/mysql-test/suite/rocksdb/t/allow_no_primary_key.test +++ b/mysql-test/suite/rocksdb/t/allow_no_primary_key.test @@ -96,3 +96,31 @@ DELETE FROM t1 WHERE a = 35 AND b = 'foo'; --sorted_result SELECT * FROM t1; DROP TABLE t1; + +--echo # +--echo # Issue #834/MDEV-15304 ALTER TABLE table_with_hidden_pk causes Can't +--echo # write; duplicate key in table error and/or crash +--echo # +CREATE TABLE t1 (a INT, KEY(a)) ENGINE=RocksDB; +INSERT INTO t1 VALUES (1),(1+1); +create table t2 (a int); +insert into t2 values (10),(20),(30); + +BEGIN; +select * from t2; + +connect (con1,localhost,root,,); +connection con1; +alter table t1 force; + +connection default; +select * from t1; + +connection con1; +insert into t1 values (100); +select * from t1; + +disconnect con1; +connection default; +rollback; +drop table t1,t2; diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc index 5f13fee..a884570 100644 --- a/storage/rocksdb/ha_rocksdb.cc +++ b/storage/rocksdb/ha_rocksdb.cc @@ -5216,8 +5216,17 @@ int ha_rocksdb::load_hidden_pk_value() { active_index = m_tbl_def->m_key_count - 1; const uint8 save_table_status = table->status; - Rdb_transaction *const tx = get_or_create_tx(table->in_use); - const bool is_new_snapshot = !tx->has_snapshot(); + /* + We should read the latest committed value in the database. + That is, if we have an open transaction with a snapshot, we should not use + it as we may get old data. Start a new transaction to read the latest + value. + */ + Rdb_transaction *const temp_tx = new Rdb_transaction_impl(table->in_use); + temp_tx->start_tx(); + Rdb_transaction *&tx = get_tx_from_thd(table->in_use); + Rdb_transaction *save_tx= tx; + tx= temp_tx; longlong hidden_pk_id = 1; // Do a lookup. @@ -5227,9 +5236,8 @@ int ha_rocksdb::load_hidden_pk_value() { */ auto err = read_hidden_pk_id_from_rowkey(&hidden_pk_id); if (err) { - if (is_new_snapshot) { - tx->release_snapshot(); - } + delete tx; + tx= save_tx; return err; } @@ -5241,9 +5249,8 @@ int ha_rocksdb::load_hidden_pk_value() { !m_tbl_def->m_hidden_pk_val.compare_exchange_weak(old, hidden_pk_id)) { } - if (is_new_snapshot) { - tx->release_snapshot(); - } + delete tx; + tx= save_tx; table->status = save_table_status; active_index = save_active_index; diff --git a/storage/rocksdb/rdb_datadic.cc b/storage/rocksdb/rdb_datadic.cc index 2c1c97b..04c8a7c 100644 --- a/storage/rocksdb/rdb_datadic.cc +++ b/storage/rocksdb/rdb_datadic.cc @@ -4241,6 +4241,9 @@ bool Rdb_ddl_manager::rename(const std::string &from, const std::string &to, rec->m_auto_incr_val.load(std::memory_order_relaxed); new_rec->m_key_descr_arr = rec->m_key_descr_arr; + new_rec->m_hidden_pk_val = + rec->m_hidden_pk_val.load(std::memory_order_relaxed); + // so that it's not free'd when deleting the old rec rec->m_key_descr_arr = nullptr;