Refactor the acquisition of DDSE handlerton (facebook#1319)

Summary: Make it support both InnoDB and MyRocks DDSEs by introducing a helper function dd::get_dd_engine that returns either based on the default_dd_storage_engine sysvar setting. To have correct diagnostics in one instance at least, introduce a get_dd_engine_name utility function too. No functional changes if default_dd_storage_engine == InnoDB. Pull Request resolved: facebook#1319 Differential Revision: D46649458
inikep · Jul 16, 2024 · 9a4ee4e · 9a4ee4e
1 parent ebfeec9
commit 9a4ee4e
Show file tree

Hide file tree

Showing 12 changed files with 69 additions and 59 deletions.
diff --git a/mysql-test/suite/rocksdb/r/early_load_rocksdb_plugin.result b/mysql-test/suite/rocksdb/r/early_load_rocksdb_plugin.result
@@ -1,18 +1,16 @@
 # shut server down
 # Server is down
 #
-# Try --initialize
-#
 # Run the server with --initialize
 #
+#
+# Error log checks
+#
+Occurrences of 'Check RocksDB:Init column families' in the input file: 0
+Occurrences of 'Check Data dictionary initializing' in the input file: 0
+#
 # Cleanup
 #
 # Restarting the server
 # restart
-#
-# Error log checks
-#
-include/assert_grep.inc [Check RocksDB:Init column families]
-include/assert_grep.inc [Check Data dictionary initializing]
-include/assert_grep.inc [disabling rocksdb_large_prefix is not allowed with MySQL data dictionary in MyRocks]
 # Remove data dir and log file.
diff --git a/mysql-test/suite/rocksdb/t/early_load_rocksdb_plugin.test b/mysql-test/suite/rocksdb/t/early_load_rocksdb_plugin.test
@@ -14,39 +14,31 @@ let extra_args=--no-defaults --basedir=$BASEDIR --debug=+d,ddse_rocksdb;
 --source include/shutdown_mysqld.inc
 --echo # Server is down
 
---echo #
---echo # Try --initialize
---echo #
-
 
+--echo #
 --echo # Run the server with --initialize
+--echo #
+--error 1
 --exec $MYSQLD $extra_args --initialize --default_dd_storage_engine=RocksDB --datadir=$DDIR --log-error-verbosity=3 > $MYSQLD_LOG 2>&1
-
 --force-rmdir $DDIR
+--echo #
+--echo # Error log checks
+--echo #
+--let $grep_file= $MYSQLD_LOG 
+--let $grep_pattern=Check RocksDB:Init column families
+--let $grep_output= print_count
+--source include/grep_pattern.inc
+
+--let $grep_file= $MYSQLD_LOG 
+--let $grep_pattern=Check Data dictionary initializing
+--let $grep_output= print_count
+--source include/grep_pattern.inc
 
 --echo #
 --echo # Cleanup
 --echo #
 --echo # Restarting the server
 --source include/start_mysqld.inc
 
---echo #
---echo # Error log checks
---echo #
---let $assert_file = $MYSQLD_LOG
---let $assert_count = 1
-
---let $assert_text = Check RocksDB:Init column families
---let $assert_select = RocksDB:Init column families
---source include/assert_grep.inc
-
---let $assert_text = Check Data dictionary initializing
---let $assert_select = Data dictionary initializing
---let $assert_only_after = Check RocksDB:Init column families
---source include/assert_grep.inc
-
---let $assert_text=disabling rocksdb_large_prefix is not allowed with MySQL data dictionary in MyRocks
---source include/assert_grep.inc
-
 --echo # Remove data dir and log file.
 --remove_file $MYSQLD_LOG
diff --git a/sql/dd/dd_table.cc b/sql/dd/dd_table.cc
@@ -46,6 +46,7 @@
 #include "sql/dd/cache/dictionary_client.h"  // dd::cache::Dictionary_client
 #include "sql/dd/collection.h"               // dd::Collection
 #include "sql/dd/dd.h"                       // dd::get_dictionary
+#include "sql/dd/dd_utility.h"
 #include "sql/dd/dictionary.h"               // dd::Dictionary
 // TODO: Avoid exposing dd/impl headers in public files.
 #include "sql/dd/impl/dictionary_impl.h"       // default_catalog_name
@@ -2374,7 +2375,7 @@ static std::unique_ptr<dd::Table> create_dd_system_table(
   }
 
   // Register the se private id with the DDSE.
-  handlerton *ddse = ha_resolve_by_legacy_type(thd, DB_TYPE_INNODB);
+  handlerton *ddse = get_dd_engine(thd);
   if (ddse->dict_register_dd_table_id == nullptr) return nullptr;
   ddse->dict_register_dd_table_id(tab_obj->se_private_id());
 

diff --git a/sql/dd/dd_utility.cc b/sql/dd/dd_utility.cc
@@ -57,12 +57,11 @@ size_t normalize_string(const CHARSET_INFO *cs, const String_type &src,
 
 bool check_if_server_ddse_readonly(THD *thd, const char *schema_name_abbrev) {
   /*
-    We must check if the DDSE is started in a way that makes the DD
-    read only. For now, we only support InnoDB as SE for the DD. The call
-    to retrieve the handlerton for the DDSE should be replaced by a more
-    generic mechanism.
+    We must check if the DDSE is started in a way that makes the DD read only.
+    The call to retrieve the handlerton for the DDSE should be replaced by a
+    more generic mechanism.
   */
-  handlerton *ddse = ha_resolve_by_legacy_type(thd, DB_TYPE_INNODB);
+  handlerton *ddse = get_dd_engine(thd);
   if (ddse->is_dict_readonly && ddse->is_dict_readonly()) {
     LogErr(WARNING_LEVEL, ER_SKIP_UPDATING_METADATA_IN_SE_RO_MODE,
            schema_name_abbrev);

diff --git a/sql/dd/dd_utility.h b/sql/dd/dd_utility.h
@@ -74,13 +74,28 @@ bool check_if_server_ddse_readonly(THD *thd, const char *schema_name = nullptr);
   initialized yet.
   @return isolation level
 */
-inline enum_tx_isolation get_dd_isolation_level() {
+[[nodiscard]] inline enum_tx_isolation get_dd_isolation_level() {
   assert(default_dd_storage_engine == DEFAULT_DD_ROCKSDB ||
          default_dd_storage_engine == DEFAULT_DD_INNODB);
   return default_dd_storage_engine == DEFAULT_DD_ROCKSDB ? ISO_READ_COMMITTED
                                                          : ISO_READ_UNCOMMITTED;
 }
 
+[[nodiscard]] inline handlerton *get_dd_engine(THD *thd) {
+  assert(default_dd_storage_engine == DEFAULT_DD_ROCKSDB ||
+         default_dd_storage_engine == DEFAULT_DD_INNODB);
+  const auto db_type = default_dd_storage_engine == DEFAULT_DD_ROCKSDB
+                           ? DB_TYPE_ROCKSDB
+                           : DB_TYPE_INNODB;
+  return ha_resolve_by_legacy_type(thd, db_type);
+}
+
+[[nodiscard]] inline const char *get_dd_engine_name() {
+  assert(default_dd_storage_engine == DEFAULT_DD_ROCKSDB ||
+         default_dd_storage_engine == DEFAULT_DD_INNODB);
+  return default_dd_storage_engine == DEFAULT_DD_ROCKSDB ? "RocksDB" : "InnoDB";
+}
+
 ///////////////////////////////////////////////////////////////////////////
 
 }  // namespace dd

diff --git a/sql/dd/impl/bootstrap/bootstrapper.cc b/sql/dd/impl/bootstrap/bootstrapper.cc
@@ -41,6 +41,7 @@
 #include "sql/auth/sql_security_ctx.h"
 #include "sql/dd/cache/dictionary_client.h"  // dd::cache::Dictionary_client
 #include "sql/dd/dd.h"                       // dd::create_object
+#include "sql/dd/dd_utility.h"
 #include "sql/dd/impl/bootstrap/bootstrap_ctx.h"        // DD_bootstrap_ctx
 #include "sql/dd/impl/cache/shared_dictionary_cache.h"  // Shared_dictionary_cache
 #include "sql/dd/impl/cache/storage_adapter.h"          // Storage_adapter
@@ -85,7 +86,7 @@ bool DDSE_dict_recover(THD *thd, dict_recovery_mode_t dict_recovery_mode,
                        uint version) {
   if (!opt_initialize)
     sysd::notify("STATUS=InnoDB crash recovery in progress\n");
-  handlerton *ddse = ha_resolve_by_legacy_type(thd, DB_TYPE_INNODB);
+  handlerton *ddse = get_dd_engine(thd);
   if (ddse->dict_recover == nullptr) return true;
 
   bool error = ddse->dict_recover(dict_recovery_mode, version);
@@ -623,14 +624,14 @@ bool populate_tables(THD *thd) {
 // Re-populate character sets and collations upon normal restart.
 bool repopulate_charsets_and_collations(THD *thd) {
   /*
-    We must check if the DDSE is started in a way that makes the DD
-    read only. For now, we only support InnoDB as SE for the DD. The call
-    to retrieve the handlerton for the DDSE should be replaced by a more
-    generic mechanism.
+    We must check if the DDSE is started in a way that makes the DD read only.
+    The call to retrieve the handlerton for the DDSE should be replaced by a
+    more generic mechanism.
   */
-  handlerton *ddse = ha_resolve_by_legacy_type(thd, DB_TYPE_INNODB);
+  handlerton *ddse = get_dd_engine(thd);
   if (ddse->is_dict_readonly && ddse->is_dict_readonly()) {
-    LogErr(WARNING_LEVEL, ER_DD_NO_WRITES_NO_REPOPULATION, "InnoDB", " ");
+    LogErr(WARNING_LEVEL, ER_DD_NO_WRITES_NO_REPOPULATION, get_dd_engine_name(),
+           " ");
     return false;
   }
 
@@ -731,7 +732,7 @@ namespace bootstrap {
   predefined tables and tablespaces.
 */
 bool DDSE_dict_init(THD *thd, dict_init_mode_t dict_init_mode, uint version) {
-  handlerton *ddse = ha_resolve_by_legacy_type(thd, DB_TYPE_INNODB);
+  handlerton *ddse = get_dd_engine(thd);
 
   /*
     The lists with element wrappers are mem root allocated. The wrapped
@@ -846,8 +847,8 @@ bool initialize_dictionary(THD *thd, bool is_dd_upgrade_57,
     return true;
 
   if (is_dd_upgrade_57) {
-    // Add status to mark creation of dictionary in InnoDB.
-    // Till this step, no new undo log is created by InnoDB.
+    // Add status to mark creation of dictionary in DDSE. In case it is InnoDB,
+    // till this step, no new undo log is created there.
     if (upgrade_57::Upgrade_status().update(
             upgrade_57::Upgrade_status::enum_stage::DICT_TABLES_CREATED))
       return true;
@@ -1689,7 +1690,7 @@ bool sync_meta_data(THD *thd) {
     return true;
 
   // Reset the DDSE local dictionary cache.
-  handlerton *ddse = ha_resolve_by_legacy_type(thd, DB_TYPE_INNODB);
+  handlerton *ddse = get_dd_engine(thd);
   if (ddse->dict_cache_reset == nullptr) return true;
 
   for (System_tables::Const_iterator it = System_tables::instance()->begin();
@@ -1959,7 +1960,7 @@ bool update_versions(THD *thd, bool is_dd_upgrade_57) {
     back in case of an abort, so this better be the last step we
     do before committing.
   */
-  handlerton *ddse = ha_resolve_by_legacy_type(thd, DB_TYPE_INNODB);
+  handlerton *ddse = get_dd_engine(thd);
   if (bootstrap::DD_bootstrap_ctx::instance().is_server_upgrade() ||
       bootstrap::DD_bootstrap_ctx::instance().is_server_patch_downgrade()) {
     if (ddse->dict_set_server_version == nullptr ||

diff --git a/sql/dd/impl/bootstrap/bootstrapper.h b/sql/dd/impl/bootstrap/bootstrapper.h
@@ -236,15 +236,15 @@ bool setup_dd_objects_and_collations(THD *thd);
 /**
   This function is used in case of crash during upgrade.
   It tries to initialize dictionary and calls DDSE_dict_recover.
-  InnoDB should do the recovery and empty undo log. Upgrade
+  The DDSE should do the recovery and, if it is InnoDB, empty undo log. Upgrade
   process will do the cleanup and exit.
 
   @param thd    Thread context.
 */
 void recover_innodb_upon_upgrade(THD *thd);
 
 /**
-  Initialize InnoDB for
+  Initialize DDSE for
   - creating new data directory : InnoDB creates system tablespace and
                                   dictionary tablespace.
   - normal server restart.      : Verifies existence of system and dictionary
@@ -253,7 +253,7 @@ void recover_innodb_upon_upgrade(THD *thd);
                                   create dictionary tablespace.
 
   @param thd             Thread context.
-  @param dict_init_mode  mode to initialize InnoDB
+  @param dict_init_mode  mode to initialize DDSE
   @param version         Dictionary version.
 
   @return       Upon failure, return true, otherwise false.

diff --git a/sql/dd/impl/dictionary_impl.cc b/sql/dd/impl/dictionary_impl.cc
@@ -41,6 +41,7 @@
 #include "sql/dd/cache/dictionary_client.h"      // dd::Dictionary_client
 #include "sql/dd/dd.h"                           // enum_dd_init_type
 #include "sql/dd/dd_schema.h"                    // dd::Schema_MDL_locker
+#include "sql/dd/dd_utility.h"
 #include "sql/dd/dd_version.h"                   // dd::DD_VERSION
 #include "sql/dd/impl/bootstrap/bootstrapper.h"  // dd::Bootstrapper
 #include "sql/dd/impl/cache/shared_dictionary_cache.h"  // Shared_dictionary_cache
@@ -694,7 +695,7 @@ bool drop_native_table(THD *thd, const char *schema_name,
 
 bool reset_tables_and_tablespaces() {
   Auto_THD thd;
-  handlerton *ddse = ha_resolve_by_legacy_type(thd.thd, DB_TYPE_INNODB);
+  handlerton *ddse = get_dd_engine(thd.thd);
 
   // Acquire transactional metadata locks and evict all cached objects.
   if (dd::cache::Shared_dictionary_cache::reset_tables_and_tablespaces(thd.thd))

diff --git a/sql/dd/impl/upgrade/dd.cc b/sql/dd/impl/upgrade/dd.cc
@@ -32,6 +32,7 @@
 #include "mysql_version.h"  // MYSQL_VERSION_ID
 #include "mysqld_error.h"
 #include "sql/dd/cache/dictionary_client.h"  // dd::cache::Dictionary_client
+#include "sql/dd/dd_utility.h"
 #include "sql/dd/impl/bootstrap/bootstrap_ctx.h"        // DD_bootstrap_ctx
 #include "sql/dd/impl/cache/shared_dictionary_cache.h"  // Shared_dictionary_cache
 #include "sql/dd/impl/system_registry.h"                // dd::System_tables
@@ -1205,7 +1206,7 @@ bool upgrade_tables(THD *thd) {
   Object_table_definition_impl::set_dd_tablespace_encrypted(false);
 
   // Reset the DDSE local dictionary cache.
-  handlerton *ddse = ha_resolve_by_legacy_type(thd, DB_TYPE_INNODB);
+  handlerton *ddse = get_dd_engine(thd);
   if (ddse->dict_cache_reset == nullptr) return true;
 
   for (System_tables::Const_iterator it =

diff --git a/sql/dd/info_schema/table_stats.cc b/sql/dd/info_schema/table_stats.cc
@@ -26,6 +26,7 @@
 #include "mysql/strings/m_ctype.h"
 #include "sql/dd/cache/dictionary_client.h"
 #include "sql/dd/dd.h"          // dd::create_object
+#include "sql/dd/dd_utility.h"
 #include "sql/dd/impl/utils.h"  // dd::my_time_t_to_ull_datetime()
 #include "sql/dd/properties.h"
 #include "sql/dd/types/index_stat.h"             // dd::Index_stat
@@ -70,7 +71,7 @@ namespace {
 inline bool can_persist_I_S_dynamic_statistics(THD *thd,
                                                const char *schema_name,
                                                const char *partition_name) {
-  handlerton *ddse = ha_resolve_by_legacy_type(thd, DB_TYPE_INNODB);
+  handlerton *ddse = dd::get_dd_engine(thd);
   if (ddse == nullptr || ddse->is_dict_readonly()) return false;
 
   return (thd->variables.information_schema_stats_expiry &&

diff --git a/sql/dd/upgrade_57/upgrade.h b/sql/dd/upgrade_57/upgrade.h
@@ -107,8 +107,8 @@ class Upgrade_status {
     STARTED,
 
     /*
-      Started InnoDB in upgrade mode, i.e., undo and redo logs are
-      upgraded and mysql.ibd is created.
+      Started DDSE in upgrade mode, i.e. in the case of InnoDB, undo and redo
+      logs are upgraded and mysql.ibd is created.
     */
     DICT_SPACE_CREATED,
 

diff --git a/sql/resourcegroups/resource_group_mgr.cc b/sql/resourcegroups/resource_group_mgr.cc
@@ -49,6 +49,7 @@
 #include "sql/current_thd.h"                 // current_thd
 #include "sql/dd/cache/dictionary_client.h"  // Dictionary_client
 #include "sql/dd/dd_resource_group.h"        // dd::create_resource_group
+#include "sql/dd/dd_utility.h"
 #include "sql/dd/string_type.h"
 #include "sql/dd/types/resource_group.h"
 #include "sql/handler.h"
@@ -120,7 +121,7 @@ Resource_group_mgr *Resource_group_mgr::instance() {
 static inline bool persist_resource_group(
     THD *thd, const resourcegroups::Resource_group &resource_group,
     bool update) {
-  handlerton *ddse = ha_resolve_by_legacy_type(thd, DB_TYPE_INNODB);
+  handlerton *ddse = dd::get_dd_engine(thd);
   if (ddse->is_dict_readonly && ddse->is_dict_readonly()) {
     LogErr(WARNING_LEVEL, ER_RESOURCE_GROUP_METADATA_UPDATE_SKIPPED);
     return false;