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

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

@@ -38,6 +38,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(), ...
@@ -1226,11 +1227,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.
@@ -1657,11 +1656,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>();
@@ -1734,11 +1729,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;
@@ -1767,12 +1758,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;
@@ -2295,12 +2282,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>();
@@ -2344,7 +2326,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>();
@@ -2823,7 +2805,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

@@ -36,8 +36,9 @@
 #include "m_string.h"
 
 #include "my_sys.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
@@ -226,11 +227,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>();