BUG#35180061 mysqlsh produces wrong dump in case of complex primary key

When dumping with `chunking` enabled, if PK or a unique index selected
to chunk a table contained an ENUM column with values specified in
non-alphabetic order, some rows from that table were missing in the
dump.

The chunking algorithm is using '>' and '<' operators to create ranges
defining each chunk. In case of an ENUM, these operators use
lexicographical ordering of the string literals assigned to the index
values and not the index values themselves. If values of an ENUM column
are not specified in the alphabetic order, chunks will not cover the
whole table and some rows will be missing.

As a fix, PKs and unique indexes which contain one or more ENUM columns
are ignored when selecting an index for the chunking algorithm.

Change-Id: I188351135231d8821c0bb26b6c0b9badc243acf6
(cherry picked from commit c827b268540d26ed64fe4f5d854a2bc27fe46ea2)

Author: Paweł Andruszkiewicz

modules/util/dump/instance_cache.cc

@@ -755,12 +755,15 @@ void Instance_cache_builder::fetch_table_indexes() {
   info.where = "COLUMN_NAME IS NOT NULL AND NON_UNIQUE=0";
 
   const std::string primary_index = "PRIMARY";
-  // schema -> table -> index name -> columns
+  struct Index_info {
+    std::vector<Instance_cache::Column *> columns;
+    bool unsafe = false;
+  };
+  // schema -> table -> index name -> index info
+  // indexes are ordered to ensure repeatability of the algorithm
   std::unordered_map<
       std::string,
-      std::unordered_map<
-          std::string,
-          std::map<std::string, std::map<uint64_t, Instance_cache::Column *>>>>
+      std::unordered_map<std::string, std::map<std::string, Index_info>>>
       indexes;
 
   iterate_tables(
@@ -781,18 +784,38 @@ void Instance_cache_builder::fetch_table_indexes() {
         // column will not be found if user is missing SELECT privilege and it
         // was not possible to fetch column information
         if (t->all_columns.end() != column) {
-          indexes[schema_name][table_name][row->get_string(2, "")].emplace(
-              row->get_uint(4), &(*column));  // INDEX_NAME, SEQ_IN_INDEX
+          auto &index_info = indexes[schema_name][table_name]
+                                    [row->get_string(2, {})];  // INDEX_NAME
+          const auto seq = row->get_uint(4);                   // SEQ_IN_INDEX
+
+          // SEQ_IN_INDEX is 1-based
+          if (seq > index_info.columns.size()) {
+            index_info.columns.resize(seq);
+          }
+
+          index_info.columns[seq - 1] = &(*column);
+          // BUG#35180061 - do not use indexes on ENUM columns
+          index_info.unsafe |= (mysqlshdk::db::Type::Enum == column->type);
         } else {
           assert(t->all_columns.empty());
         }
       });
 
-  for (const auto &schema : indexes) {
-    auto &s = m_cache.schemas.at(schema.first);
+  for (auto &schema : indexes) {
+    for (auto &table : schema.second) {
+      for (auto index = table.second.begin(); index != table.second.end();) {
+        if (index->second.unsafe) {
+          index = table.second.erase(index);
+        } else {
+          ++index;
+        }
+      }
 
-    for (const auto &table : schema.second) {
-      auto &t = s.tables.at(table.first);
+      if (table.second.empty()) {
+        continue;
+      }
+
+      auto &t = m_cache.schemas.at(schema.first).tables.at(table.first);
 
       auto index = table.second.find(primary_index);
       bool primary = false;
@@ -803,12 +826,11 @@ void Instance_cache_builder::fetch_table_indexes() {
         const auto mark_integer_columns = [](decltype(index) idx) {
           std::vector<bool> result;
 
-          result.reserve(idx->second.size());
+          result.reserve(idx->second.columns.size());
 
-          for (const auto &c : idx->second) {
-            result.emplace_back(
-                c.second->type == mysqlshdk::db::Type::Integer ||
-                c.second->type == mysqlshdk::db::Type::UInteger);
+          for (const auto &c : idx->second.columns) {
+            result.emplace_back(c->type == mysqlshdk::db::Type::Integer ||
+                                c->type == mysqlshdk::db::Type::UInteger);
           }
 
           return result;
@@ -829,23 +851,23 @@ void Instance_cache_builder::fetch_table_indexes() {
 
         // skip first index
         for (auto it = std::next(index); it != table.second.end(); ++it) {
-          const auto size = it->second.size();
+          const auto size = it->second.columns.size();
 
-          if (size < index->second.size()) {
+          if (size < index->second.columns.size()) {
             // prefer shorter index
             use_index(it);
-          } else if (size == index->second.size()) {
+          } else if (size == index->second.columns.size()) {
             // if indexes have equal length, prefer the one with longer run of
             // integer columns
             auto candidate = mark_integer_columns(it);
-            auto candidate_column = it->second.begin();
-            auto current_column = index->second.begin();
+            auto candidate_column = it->second.columns.begin();
+            auto current_column = index->second.columns.begin();
 
             for (std::size_t i = 0; i < size; ++i) {
               if (candidate[i] == current[i]) {
                 // both columns are of the same type, check if they are nullable
-                if (candidate_column->second->nullable ==
-                    current_column->second->nullable) {
+                if ((*candidate_column)->nullable ==
+                    (*current_column)->nullable) {
                   // both columns are NULL or NOT NULL
                   if (!current[i]) {
                     // both columns are not integers, use the current index
@@ -854,7 +876,7 @@ void Instance_cache_builder::fetch_table_indexes() {
                   // else, both column are integers, check next column
                 } else {
                   // prefer NOT NULL column
-                  if (!candidate_column->second->nullable) {
+                  if (!(*candidate_column)->nullable) {
                     // candidate is NOT NULL, use that
                     use_given_index(it, std::move(candidate));
                   }
@@ -883,8 +905,8 @@ void Instance_cache_builder::fetch_table_indexes() {
 
       t.index.set_primary(primary);
 
-      for (auto &column : index->second) {
-        t.index.add_column(column.second);
+      for (auto &column : index->second.columns) {
+        t.index.add_column(column);
       }
     }
   }

unittest/scripts/auto/py_shell/scripts/util_dump_tables_norecord.py

@@ -3126,12 +3126,59 @@ def generate_gaps():
 
 session.run_sql("DROP SCHEMA IF EXISTS !;", [ tested_schema ])
 session.run_sql("CREATE SCHEMA !", [ tested_schema ])
-session.run_sql(f"CREATE TABLE !.! (a TEXT, b TEXT, c GEOMETRY NOT NULL, d GEOMETRY NOT NULL, FULLTEXT (a), FULLTEXT (b), SPATIAL KEY (c), SPATIAL KEY (d))", [ tested_schema, tested_table ])
+session.run_sql("CREATE TABLE !.! (a TEXT, b TEXT, c GEOMETRY NOT NULL, d GEOMETRY NOT NULL, FULLTEXT (a), FULLTEXT (b), SPATIAL KEY (c), SPATIAL KEY (d))", [ tested_schema, tested_table ])
 
 TEST_DUMP_AND_LOAD(tested_schema, [ tested_table ], { "showProgress": False })
 
 session.run_sql("DROP SCHEMA !;", [ tested_schema ])
 
+#@<> BUG#35180061 - dumping a table which has a PK containing an ENUM column, whose values are not specified in an alphabetic order
+tested_schema = "tested_schema"
+tested_table = "tested_table"
+
+session.run_sql("DROP SCHEMA IF EXISTS !;", [ tested_schema ])
+session.run_sql("CREATE SCHEMA !", [ tested_schema ])
+session.run_sql("""CREATE TABLE !.! (
+    `content_type` ENUM('music','music_video','ringtone') NOT NULL,
+    `service` ENUM('individual','individual_annual','match','student','family','trial') NOT NULL,
+    `product_id` int unsigned NOT NULL,
+    `ordinal` int unsigned NOT NULL,
+    PRIMARY KEY (`content_type`,`service`),
+    KEY `product_id` (`product_id`),
+    UNIQUE KEY `ordinal` (`ordinal`)
+)""", [ tested_schema, tested_table ])
+session.run_sql("""INSERT INTO !.! VALUES
+    ('music_video', 'individual', 255, 1),
+    ('music', 'trial', 327, 2),
+    ('ringtone', 'trial', 327, 3),
+    ('music', 'family', 383, 4),
+    ('ringtone', 'family', 383, 5),
+    ('music', 'individual', 384, 6),
+    ('ringtone', 'individual', 384, 7),
+    ('music_video', 'family', 385, 8),
+    ('music_video', 'trial', 386, 9),
+    ('music', 'student', 392, 10),
+    ('ringtone', 'student', 392, 11),
+    ('music_video', 'student', 393, 12),
+    ('music', 'match', 394, 13),
+    ('ringtone', 'match', 394, 14),
+    ('music_video', 'match', 395, 15),
+    ('music', 'individual_annual', 431, 16),
+    ('ringtone', 'individual_annual', 431, 17),
+    ('music_video', 'individual_annual', 432, 18)
+""", [ tested_schema, tested_table ])
+
+# PK consists of ENUM columns, but there's a unique index, it's going to be used for chunking and only one chunk is going to be created
+TEST_DUMP_AND_LOAD(tested_schema, [ tested_table ], { "showProgress": False })
+EXPECT_STDOUT_CONTAINS("1 chunks (18 rows, 447 bytes) for 1 tables in 1 schemas were loaded")
+
+# PK consists of ENUM columns, there are no more indexes, table is not going to be chunked, chunks are created while dumping the table, two chunks are going to be created
+session.run_sql("DROP INDEX `ordinal` ON !.!", [ tested_schema, tested_table ])
+TEST_DUMP_AND_LOAD(tested_schema, [ tested_table ], { "showProgress": False })
+EXPECT_STDOUT_CONTAINS("2 chunks (18 rows, 447 bytes) for 1 tables in 1 schemas were loaded")
+
+session.run_sql("DROP SCHEMA !;", [ tested_schema ])
+
 #@<> Cleanup
 drop_all_schemas()
 session.run_sql("SET GLOBAL local_infile = false;")