MyRocks executes index_merge query plan incorrectly.

[hermanlee]

Test case:

create table t1 (key1 int, key2 int, key3 int, key (key1), key (key2), key(key3)) engine=rocksdb;
insert into t1 values (1, 100, 100), (1, 200, 200), (1, 300, 300);
set global rocksdb_force_flush_memtable_now = 1;
explain select * from t1 where key1 = 1;
explain select * from t1 where key1 = 1 or key2 = 1;
select * from t1 where key1 = 1;
select * from t1 where key1 = 1 or key2 = 1;

Output: Note the two selects should return the same rows.

mysql> create table t1 (key1 int, key2 int, key3 int, key (key1), key (key2), key(key3)) engine=rocksdb;
Query OK, 0 rows affected (1.07 sec)

mysql> insert into t1 values (1, 100, 100), (1, 200, 200), (1, 300, 300);
Query OK, 3 rows affected (0.07 sec)
Records: 3  Duplicates: 0  Warnings: 0

mysql> set global rocksdb_force_flush_memtable_now = 1;
Query OK, 0 rows affected (0.42 sec)

mysql> explain select * from t1 where key1 = 1;
+----+-------------+-------+------+---------------+------+---------+-------+------+-------+
| id | select_type | table | type | possible_keys | key  | key_len | ref   | rows | Extra |
+----+-------------+-------+------+---------------+------+---------+-------+------+-------+
|  1 | SIMPLE      | t1    | ref  | key1          | key1 | 5       | const |    1 | NULL  |
+----+-------------+-------+------+---------------+------+---------+-------+------+-------+
1 row in set (0.00 sec)

mysql> explain select * from t1 where key1 = 1 or key2 = 1;
+----+-------------+-------+-------------+---------------+-----------+---------+------+------+-------------------------------------+
| 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 |    2 | Using union(key1,key2); Using where |
+----+-------------+-------+-------------+---------------+-----------+---------+------+------+-------------------------------------+
1 row in set (0.01 sec)

mysql> select * from t1 where key1 = 1;
+------+------+------+
| key1 | key2 | key3 |
+------+------+------+
|    1 |  100 |  100 |
|    1 |  200 |  200 |
|    1 |  300 |  300 |
+------+------+------+
3 rows in set (0.00 sec)

mysql> select * from t1 where key1 = 1 or key2 = 1;
+------+------+------+
| key1 | key2 | key3 |
+------+------+------+
|    1 |  100 |  100 |
+------+------+------+
1 row in set (0.01 sec)

[yoshinorim]

There is a bug in void ha_rocksdb::setup_read_decoders().

    if (m_lock_rows == RDB_LOCK_WRITE || m_verify_row_debug_checksums ||
        bitmap_is_set(table->read_set, table->field[i]->field_index)) {

bitmap_is_set() returned false with index merge, which made the decoder not added and could not decode keys properly. In the above case, "set session rocksdb_verify_row_debug_checksums=1;" makes the index merge return correct rows..

I confirmed table->read_set bitmap (table->read_set->bitmap[0]) was 7 with index lookup, and 0 with index merge. With index merge, the bitmap was set to 0 in QUICK_RANGE_SELECT::init_ror_merged_scan() (at head->column_bitmaps_set(&column_bitmap, &column_bitmap()).

[yoshinorim]

I confirmed the following change made the index merge return correct rows.

$ git diff
diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc
index 6686f69..a7d817f 100644
--- a/storage/rocksdb/ha_rocksdb.cc
+++ b/storage/rocksdb/ha_rocksdb.cc
@@ -4427,7 +4428,9 @@ void ha_rocksdb::setup_read_decoders() {
       continue;
     }

+    // bitmap is cleared on index merge but it needs to decode columns
     if (m_lock_rows == RDB_LOCK_WRITE || m_verify_row_debug_checksums ||
+        bitmap_is_clear_all(table->read_set) ||
         bitmap_is_set(table->read_set, table->field[i]->field_index)) {
       // We will need to decode this field
       m_decoders_vect.push_back({&m_encoder_arr[i], true, skip_size});

[spetrunia]

More Analysis:

QUICK_RANGE_SELECT::init_ror_merged_scan first calls ha_index_init:

  #0  myrocks::ha_rocksdb::setup_read_decoders
  #1  0x000000000149f18e in myrocks::ha_rocksdb::index_init
  #2  0x0000000000c2da60 in handler::ha_index_init
  #3  0x0000000000fdffe2 in QUICK_RANGE_SELECT::reset
  #4  0x0000000000fca745 in QUICK_RANGE_SELECT::init_ror_merged_scan
  #5  0x0000000000fcb1ea in QUICK_ROR_UNION_SELECT::reset

When the above call is made, QUICK_RANGE_SELECT::column_bitmap has all bits cleared.

The code in init_ror_merged_scan() slightly below sets column_bitmap to only include columns that are part of the used index:

  if (!head->no_keyread)
    head->mark_columns_used_by_index(index);
  head->prepare_for_position();
  head->file= org_file;
  bitmap_copy(&column_bitmap, head->read_set);

(it is also interesting is that it is set to include all columns in the index, even those that are not read by the query)

[spetrunia]

@yoshinorim, as far as I can see your fix will not break anything. Perhaps, it will cause extra columns to be read for queries SELECT COUNT(*) FROM table (without the WHERE), but it's a very degenerate case.

The fix doesn't look nice from an architecture POV, though - there is no reason why SQL layer would call index_init() with table->read_set having all bits cleared (InnoDB apparently doesn't care, but it's still wrong). Making the storage engine reflect the quirks of the SQL layer doesn't look nice

[yoshinorim]

Thanks for review @spetrunia . I'm going to commit the diff to fix the S1, and will consider long term fix and mitigating other perf issues.

[spetrunia]

A possible alternative fix

index d6acf12..e009c02 100644
--- a/sql/opt_range.cc
+++ b/sql/opt_range.cc
@@ -1714,6 +1714,7 @@ end:
   if (!head->no_keyread)
     head->mark_columns_used_by_index(index);
   head->prepare_for_position();
+  head->file->column_bitmaps_signal(); // psergey-new
   head->file= org_file;
   bitmap_copy(&column_bitmap, head->read_set);
 
diff --git a/storage/rocksdb/ha_rocksdb.h b/storage/rocksdb/ha_rocksdb.h
index 8744243..a0fbc24 100644
--- a/storage/rocksdb/ha_rocksdb.h
+++ b/storage/rocksdb/ha_rocksdb.h
@@ -630,6 +630,7 @@ class ha_rocksdb : public my_core::handler {
   /* Setup field_decoders based on type of scan and table->read_set */
   void setup_read_decoders();
 
+  virtual void column_bitmaps_signal() override { setup_read_decoders(); }
   /*
     Number of bytes in on-disk (storage) record format that are used for
     storing SQL NULL flags.

Doesn't cause extra testcase failures for me, but I would still use yours to fix the S1 issue.

[yoshinorim]

Fix committed. I'm keeping this task opened for future refactoring.