Refactor data dictionary transaction isolation setting (facebook#1316)

Summary:
InnoDB uses READ UNCOMMITTED, which is not supported with MyRocks. Instead of
hardcoding READ UNCOMMITTED at several locations, introduce a helper function
that returns the desired DD transaction isolation level, based on the default DD
engine.

No functional changes if the default DD engine is InnoDB.

Pull Request resolved: facebook#1316

Differential Revision: D46470667

Author: laurynas-biveinis

sql/dd/cache/dictionary_client.h

@@ -592,8 +592,9 @@ class Dictionary_client {
     not known by the object registry.
 
     When the object is read from the persistent tables, the transaction
-    isolation level is READ UNCOMMITTED. This is necessary to be able to
-    read uncommitted data from an earlier stage of the same session.
+    isolation level is READ UNCOMMITTED if the DD is stored in InnoDB. This is
+    necessary to be able to read uncommitted data from an earlier stage of the
+    same session. Under MyRocks, READ COMMITTED is used.
 
     @tparam       T       Dictionary object type.
     @param        id      Object id to retrieve.
@@ -614,8 +615,9 @@ class Dictionary_client {
     dictionary client methods since it is not known by the object registry.
 
     When the object is read from the persistent tables, the transaction
-    isolation level is READ UNCOMMITTED. This is necessary to be able to
-    read uncommitted data from an earlier stage of the same session.
+    isolation level is READ UNCOMMITTED if the DD is stored in InnoDB. This is
+    necessary to be able to read uncommitted data from an earlier stage of the
+    same session. Under MyRocks, READ COMMITTED is used.
 
     @tparam       T           Dictionary object type.
     @param        id          Object id to retrieve.
@@ -1046,9 +1048,10 @@ class Dictionary_client {
 
   /**
     Fetch Object ids of all the views referencing base table/ view/ stored
-    function name specified in "schema"."name". The views are retrieved
-    using READ_UNCOMMITTED reads as the views could be changed by the same
-    statement (e.g. multi-table/-view RENAME TABLE).
+    function name specified in "schema"."name". The views are retrieved using
+    READ_UNCOMMITTED reads if InnoDB is the DD storage engine as the views could
+    be changed by the same statement (e.g. multi-table/-view RENAME TABLE).
+    Under MyRocks, READ_COMMITTED is used.
 
     @tparam       T               Type of the object (View_table/View_routine)
                                   to retrieve view names for.
@@ -1072,7 +1075,8 @@ class Dictionary_client {
     @param        parent_schema    Schema name of parent table.
     @param        parent_name      Table name of parent table.
     @param        parent_engine    Storage engine of parent table.
-    @param        uncommitted      Use READ_UNCOMMITTED isolation.
+    @param        uncommitted      Use READ_UNCOMMITTED isolation if DD SE is
+                                   InnoDB.
     @param[out]   children_schemas Schema names of child tables.
     @param[out]   children_names   Table names of child tables.
 

sql/dd/dd_utility.h

@@ -24,6 +24,8 @@
 #define DD__UTILITY_INCLUDED
 
 #include "sql/dd/string_type.h"  // dd::String_type
+#include "sql/handler.h"         // enum_tx_isolation
+#include "sql/mysqld.h"
 
 struct CHARSET_INFO;
 class THD;
@@ -64,6 +66,21 @@ size_t normalize_string(const CHARSET_INFO *cs, const String_type &src,
 */
 bool check_if_server_ddse_readonly(THD *thd, const char *schema_name = nullptr);
 
+/**
+  Get the isolation level for a data dictionary transaction. InnoDB uses READ
+  UNCOMMITTED to work correctly in the following cases:
+  - when called in the middle of an atomic DDL statement;
+  - wehn called during the server startup when the undo logs have not been
+  initialized yet.
+  @return isolation level
+*/
+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;
+}
+
 ///////////////////////////////////////////////////////////////////////////
 
 }  // namespace dd

sql/dd/impl/cache/dictionary_client.cc

@@ -39,6 +39,7 @@
 #include "mysqld_error.h"
 #include "sql/dd/cache/multi_map_base.h"
 #include "sql/dd/dd_schema.h"                           // dd::Schema_MDL_locker
+#include "sql/dd/dd_utility.h"
 #include "sql/dd/impl/bootstrap/bootstrap_ctx.h"        // bootstrap_stage
 #include "sql/dd/impl/cache/shared_dictionary_cache.h"  // get(), release(), ...
 #include "sql/dd/impl/cache/storage_adapter.h"          // store(), drop(), ...
@@ -1230,11 +1231,9 @@ bool Dictionary_client::acquire_uncached_uncommitted_impl(Object_id id,
     return false;
   }
 
-  // Read the uncached dictionary object using ISO_READ_UNCOMMITTED
-  // isolation level.
   const typename T::Cache_partition *stored_object = nullptr;
   bool error = Shared_dictionary_cache::instance()->get_uncached(
-      m_thd, key, ISO_READ_UNCOMMITTED, &stored_object);
+      m_thd, key, get_dd_isolation_level(), &stored_object);
   if (!error) {
     // Here, stored_object is a newly created instance, so we do not need to
     // clone() it, but we must delete it if dynamic cast fails.
@@ -1661,11 +1660,7 @@ static bool get_index_statistics_entries(
     THD *thd, const String_type &schema_name, const String_type &table_name,
     std::vector<String_type> &index_names,
     std::vector<String_type> &column_names) {
-  /*
-    Use READ UNCOMMITTED isolation, so this method works correctly when
-    called from the middle of atomic ALTER TABLE statement.
-  */
-  dd::Transaction_ro trx(thd, ISO_READ_UNCOMMITTED);
+  dd::Transaction_ro trx(thd, get_dd_isolation_level());
 
   // Open the DD tables holding dynamic table statistics.
   trx.otx.register_tables<dd::Table_stat>();
@@ -1738,11 +1733,7 @@ bool Dictionary_client::remove_table_dynamic_statistics(
           tables::Index_stats::create_object_key(schema_name, table_name,
                                                  *it_idxs, *it_cols));
 
-      /*
-        Use READ UNCOMMITTED isolation, so this method works correctly when
-        called from the middle of atomic ALTER TABLE statement.
-      */
-      if (Storage_adapter::get(m_thd, *key, ISO_READ_UNCOMMITTED, false,
+      if (Storage_adapter::get(m_thd, *key, get_dd_isolation_level(), false,
                                &idx_stat)) {
         assert(m_thd->is_error() || m_thd->killed);
         return true;
@@ -1771,12 +1762,8 @@ bool Dictionary_client::remove_table_dynamic_statistics(
   std::unique_ptr<Table_stat::Name_key> key(
       tables::Table_stats::create_object_key(schema_name, table_name));
 
-  /*
-    Use READ UNCOMMITTED isolation, so this method works correctly when
-    called from the middle of atomic ALTER TABLE statement.
-  */
   const Table_stat *tab_stat = nullptr;
-  if (Storage_adapter::get(m_thd, *key, ISO_READ_UNCOMMITTED, false,
+  if (Storage_adapter::get(m_thd, *key, get_dd_isolation_level(), false,
                            &tab_stat)) {
     assert(m_thd->is_error() || m_thd->killed);
     return true;
@@ -2299,12 +2286,7 @@ template <typename T>
 bool Dictionary_client::fetch_referencing_views_object_id(
     const char *schema, const char *tbl_or_sf_name,
     std::vector<Object_id> *view_ids) const {
-  /*
-    Use READ UNCOMMITTED isolation, so this method works correctly when
-    called from the middle of atomic DROP TABLE/DATABASE or
-    RENAME TABLE statements.
-  */
-  dd::Transaction_ro trx(m_thd, ISO_READ_UNCOMMITTED);
+  dd::Transaction_ro trx(m_thd, get_dd_isolation_level());
 
   // Register View_table_usage/View_routine_usage.
   trx.otx.register_tables<T>();
@@ -2348,7 +2330,7 @@ bool Dictionary_client::fetch_fk_children_uncached(
     std::vector<String_type> *children_schemas,
     std::vector<String_type> *children_names) {
   dd::Transaction_ro trx(
-      m_thd, uncommitted ? ISO_READ_UNCOMMITTED : ISO_READ_COMMITTED);
+      m_thd, uncommitted ? get_dd_isolation_level() : ISO_READ_COMMITTED);
 
   trx.otx.register_tables<Foreign_key>();
   Raw_table *foreign_keys_table = trx.otx.get_table<Foreign_key>();
@@ -2827,7 +2809,7 @@ void Dictionary_client::remove_uncommitted_objects(
         DBUG_EVALUATE_IF("skip_dd_table_access_check", false, true)) {
       const typename T::Cache_partition *stored_object = nullptr;
       if (!Shared_dictionary_cache::instance()->get_uncached(
-              m_thd, id_key, ISO_READ_UNCOMMITTED, &stored_object))
+              m_thd, id_key, get_dd_isolation_level(), &stored_object))
         assert(stored_object == nullptr);
     }
 

sql/dd/impl/tables/dd_properties.cc

@@ -34,6 +34,7 @@
 #include "mysql/udf_registration_types.h"
 #include "mysqld_error.h"
 #include "sql/auth/sql_security_ctx.h"
+#include "sql/dd/dd_utility.h"
 #include "sql/dd/dd_version.h"  // dd::DD_VERSION
 #include "sql/dd/impl/raw/raw_table.h"
 #include "sql/dd/impl/transaction_impl.h"
@@ -130,12 +131,7 @@ bool DD_properties::init_cached_properties(THD *thd) {
   // Early exit in case the properties are already initialized.
   if (!m_properties.empty()) return false;
 
-  /*
-    Start a DD transaction to get the properties. Please note that we
-    must do this read using isolation level ISO_READ_UNCOMMITTED
-    because the SE undo logs may not yet be available.
-  */
-  Transaction_ro trx(thd, ISO_READ_UNCOMMITTED);
+  Transaction_ro trx(thd, get_dd_isolation_level());
   trx.otx.add_table<DD_properties>();
 
   if (trx.otx.open_tables()) {

sql/dd/impl/types/table_impl.cc

@@ -38,8 +38,9 @@
 
 #include "my_sys.h"
 #include "mysql/strings/m_ctype.h"
-#include "mysqld_error.h"                         // ER_*
-#include "sql/current_thd.h"                      // current_thd
+#include "mysqld_error.h"     // ER_*
+#include "sql/current_thd.h"  // current_thd
+#include "sql/dd/dd_utility.h"
 #include "sql/dd/impl/bootstrap/bootstrap_ctx.h"  // dd::bootstrap::DD_bootstrap_ctx
 #include "sql/dd/impl/dictionary_impl.h"          // Dictionary_impl
 #include "sql/dd/impl/properties_impl.h"          // Properties_impl
@@ -228,11 +229,7 @@ bool Table_impl::load_foreign_key_parents(Open_dictionary_tables_ctx *otx) {
 ///////////////////////////////////////////////////////////////////////////
 
 bool Table_impl::reload_foreign_key_parents(THD *thd) {
-  /*
-     Use READ UNCOMMITTED isolation, so this method works correctly when
-     called from the middle of atomic DDL statements.
-   */
-  dd::Transaction_ro trx(thd, ISO_READ_UNCOMMITTED);
+  dd::Transaction_ro trx(thd, get_dd_isolation_level());
 
   // Register and open tables.
   trx.otx.register_tables<dd::Table>();