PS-9666: insert with unique_checks=OFF into table with TTL crashing MyRocks

inikep · inikep · commit ecc87980002a · 2025-05-28T14:57:09.000+02:00
1. Port `convert_get_status()` from facebook/mysql-5.6@45751b91c8 2. Port a fix for `rdb_should_hide_ttl_rec()` from facebook/mysql-5.6@1fb186d93d1c16f
diff --git a/mysql-test/suite/rocksdb/r/percona_bugs.result b/mysql-test/suite/rocksdb/r/percona_bugs.result
@@ -25,3 +25,15 @@ PARTITION p1 VALUES IN (33, 44, 7, 10, 68, 78, 72, 2, 24, 73, 50, 56, 83,
 PARTITION p2 VALUES IN (90, 11, 87, 25, 97, 93, 47, 41, 92, 37, 67, 20, 43,
 42, 53, 62, 13));
 DROP TABLE t;
+#
+# PS-9666: insert with unique_checks=OFF into table with TTL crashing MyRocks
+# https://perconadev.atlassian.net/browse/PS-9666
+#
+CREATE TABLE t1 (
+`id` varbinary(32) NOT NULL,
+`ttl_ts` bigint unsigned NOT NULL,
+PRIMARY KEY (`id`)
+) ENGINE=ROCKSDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci COMMENT='ttl_duration=31536000;ttl_col=ttl_ts;';
+SET unique_checks=OFF;
+INSERT INTO t1 (`id`, `ttl_ts`) VALUES ('helloworld', 1738737225);
+DROP TABLE t1;
diff --git a/mysql-test/suite/rocksdb/t/percona_bugs.test b/mysql-test/suite/rocksdb/t/percona_bugs.test
@@ -52,3 +52,22 @@ TRUNCATE TABLE t;
 --enable_result_log
 
 DROP TABLE t;
+
+
+
+--echo #
+--echo # PS-9666: insert with unique_checks=OFF into table with TTL crashing MyRocks
+--echo # https://perconadev.atlassian.net/browse/PS-9666
+--echo #
+
+CREATE TABLE t1 (
+    `id` varbinary(32) NOT NULL,
+    `ttl_ts` bigint unsigned NOT NULL,
+    PRIMARY KEY (`id`)
+) ENGINE=ROCKSDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci COMMENT='ttl_duration=31536000;ttl_col=ttl_ts;';
+
+SET unique_checks=OFF;
+
+INSERT INTO t1 (`id`, `ttl_ts`) VALUES ('helloworld', 1738737225);
+
+DROP TABLE t1;
diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc
@@ -7763,7 +7763,7 @@ static int rdb_dbug_set_ttl_read_filter_ts();
   snapshots when filtering keys.
 */
 bool rdb_should_hide_ttl_rec(const Rdb_key_def &kd,
-                             const rocksdb::Slice &ttl_rec_val,
+                             const rocksdb::Slice *const ttl_rec_val,
                              Rdb_transaction *tx) {
   assert(kd.has_ttl());
   assert(kd.m_ttl_rec_offset != UINT_MAX);
@@ -7791,7 +7791,13 @@ bool rdb_should_hide_ttl_rec(const Rdb_key_def &kd,
     return false;
   }
 
-  Rdb_string_reader reader(&ttl_rec_val);
+  // Case: No value supplied, this happens when the key is not found, so we'll
+  // just return that it should be filtered
+  if (!ttl_rec_val) {
+    return true;
+  }
+
+  Rdb_string_reader reader(ttl_rec_val);
 
   /*
     Find where the 8-byte ttl is for each record in this index.
@@ -7803,7 +7809,7 @@ bool rdb_should_hide_ttl_rec(const Rdb_key_def &kd,
       8 byte ttl field in front. Don't filter the record out, and log an error.
     */
     std::string buf;
-    buf = rdb_hexdump(ttl_rec_val.data(), ttl_rec_val.size(),
+    buf = rdb_hexdump(ttl_rec_val->data(), ttl_rec_val->size(),
                       RDB_MAX_HEXDUMP_LEN);
     const GL_INDEX_ID gl_index_id = kd.get_gl_index_id();
     LogPluginErrMsg(
@@ -17288,10 +17294,10 @@ void rdb_tx_release_lock(Rdb_transaction *tx, const Rdb_key_def &kd,
   tx->release_lock(kd, std::string(key.data(), key.size()), force);
 }
 
-int rdb_tx_set_status_error(Rdb_transaction *tx, const rocksdb::Status &s,
+int rdb_tx_set_status_error(Rdb_transaction &tx, const rocksdb::Status &s,
                             const Rdb_key_def &kd,
                             const Rdb_tbl_def *const tbl_def) {
-  return tx->set_status_error(tx->get_thd(), s, kd, tbl_def);
+  return tx.set_status_error(tx.get_thd(), s, kd, tbl_def);
 }
 
 static bool parse_fault_injection_file_type(const std::string &type_str,
diff --git a/storage/rocksdb/ha_rocksdb.h b/storage/rocksdb/ha_rocksdb.h
@@ -1179,11 +1179,11 @@ inline void rocksdb_smart_prev(bool seek_backward,
 bool is_valid_iterator(rocksdb::Iterator *scan_it);
 
 bool rdb_should_hide_ttl_rec(const Rdb_key_def &kd,
-                             const rocksdb::Slice &ttl_rec_val,
+                             const rocksdb::Slice *const ttl_rec_val,
                              Rdb_transaction *tx);
 
 bool rdb_tx_started(Rdb_transaction *tx);
-int rdb_tx_set_status_error(Rdb_transaction *tx, const rocksdb::Status &s,
+int rdb_tx_set_status_error(Rdb_transaction &tx, const rocksdb::Status &s,
                             const Rdb_key_def &kd,
                             const Rdb_tbl_def *const tbl_def);
 
diff --git a/storage/rocksdb/rdb_iterator.cc b/storage/rocksdb/rdb_iterator.cc
@@ -295,7 +295,7 @@ int Rdb_iterator_base::next_with_direction(bool move_forward, bool skip_next) {
     }
 
     // Record is not visible due to TTL, move to next record.
-    if (m_pkd.has_ttl() && rdb_should_hide_ttl_rec(m_kd, value, tx)) {
+    if (m_pkd.has_ttl() && rdb_should_hide_ttl_rec(m_kd, &value, tx)) {
       continue;
     }
 
@@ -357,10 +357,29 @@ int Rdb_iterator_base::seek(enum ha_rkey_function find_flag,
   return rc;
 }
 
+int Rdb_iterator_base::convert_get_status(myrocks::Rdb_transaction &tx,
+                                          const rocksdb::Status &s,
+                                          rocksdb::PinnableSlice *value,
+                                          bool skip_ttl_check) const {
+  int rc = HA_EXIT_SUCCESS;
+  if (!s.IsNotFound() && !s.ok()) {
+    return rdb_tx_set_status_error(tx, s, m_kd, m_tbl_def);
+  }
+
+  const bool hide_ttl_rec =
+      !skip_ttl_check && m_kd.has_ttl() &&
+      rdb_should_hide_ttl_rec(m_kd, s.IsNotFound() ? nullptr : value, &tx);
+
+  if (hide_ttl_rec || s.IsNotFound()) {
+    return HA_ERR_KEY_NOT_FOUND;
+  }
+
+  return rc;
+}
+
 int Rdb_iterator_base::get(const rocksdb::Slice *key,
                            rocksdb::PinnableSlice *value, Rdb_lock_type type,
                            bool skip_ttl_check, bool skip_wait) {
-  int rc = HA_EXIT_SUCCESS;
   Rdb_transaction *const tx = get_tx_from_thd(m_thd);
   rocksdb::Status s;
   if (type == RDB_LOCK_NONE) {
@@ -374,20 +393,7 @@ int Rdb_iterator_base::get(const rocksdb::Slice *key,
       "rocksdb_return_status_corrupted",
                   { s = rocksdb::Status::Corruption(); });
 
-  if (!s.IsNotFound() && !s.ok()) {
-    return rdb_tx_set_status_error(tx, s, m_kd, m_tbl_def);
-  }
-
-  if (s.IsNotFound()) {
-    return HA_ERR_KEY_NOT_FOUND;
-  }
-
-  if (!skip_ttl_check && m_kd.has_ttl() &&
-      rdb_should_hide_ttl_rec(m_kd, *value, tx)) {
-    return HA_ERR_KEY_NOT_FOUND;
-  }
-
-  return rc;
+  return convert_get_status(*tx, s, value, skip_ttl_check);
 }
 
 Rdb_iterator_partial::Rdb_iterator_partial(THD *thd, const Rdb_key_def &kd,
@@ -657,7 +663,7 @@ int Rdb_iterator_partial::materialize_prefix() {
     return HA_EXIT_SUCCESS;
   } else if (!s.IsNotFound()) {
     thd_proc_info(m_thd, old_proc_info);
-    return rdb_tx_set_status_error(tx, s, m_kd, m_tbl_def);
+    return rdb_tx_set_status_error(*tx, s, m_kd, m_tbl_def);
   }
 
   rocksdb::WriteOptions options;
@@ -669,7 +675,7 @@ int Rdb_iterator_partial::materialize_prefix() {
   // Write sentinel key with empty value.
   s = wb->Put(m_kd.get_cf(), cur_prefix_key, rocksdb::Slice());
   if (!s.ok()) {
-    rc = rdb_tx_set_status_error(tx, s, m_kd, m_tbl_def);
+    rc = rdb_tx_set_status_error(*tx, s, m_kd, m_tbl_def);
     rdb_tx_release_lock(tx, m_kd, cur_prefix_key, true /* force */);
     thd_proc_info(m_thd, old_proc_info);
     return rc;
@@ -711,7 +717,7 @@ int Rdb_iterator_partial::materialize_prefix() {
                 rocksdb::Slice((const char *)m_sk_tails.ptr(),
                                m_sk_tails.get_current_pos()));
     if (!s.ok()) {
-      rc = rdb_tx_set_status_error(tx, s, m_kd, m_tbl_def);
+      rc = rdb_tx_set_status_error(*tx, s, m_kd, m_tbl_def);
       goto exit;
     }
 
@@ -724,7 +730,7 @@ int Rdb_iterator_partial::materialize_prefix() {
 
   s = rdb_get_rocksdb_db()->Write(options, optimize, wb.get());
   if (!s.ok()) {
-    rc = rdb_tx_set_status_error(tx, s, m_kd, m_tbl_def);
+    rc = rdb_tx_set_status_error(*tx, s, m_kd, m_tbl_def);
     goto exit;
   }
 
diff --git a/storage/rocksdb/rdb_iterator.h b/storage/rocksdb/rdb_iterator.h
@@ -68,6 +68,10 @@ class Rdb_iterator_base : public Rdb_iterator {
                        const int bytes_changed_by_succ,
                        const rocksdb::Slice &end_key);
   int next_with_direction(bool move_forward, bool skip_next);
+  [[nodiscard]] int convert_get_status(myrocks::Rdb_transaction &tx,
+                                       const rocksdb::Status &status,
+                                       rocksdb::PinnableSlice *value,
+                                       bool skip_ttl_check) const;
 
  public:
   Rdb_iterator_base(THD *thd, const Rdb_key_def &kd, const Rdb_key_def &pkd,
