From bf397ee9d5e4387b2c379e84504122f5dd99d1a6 Mon Sep 17 00:00:00 2001
From: Laurynas Biveinis <laurynas.biveinis@gmail.com>
Date: Fri, 9 Jun 2023 09:50:12 -0700
Subject: [PATCH] Refactor data dictionary transaction isolation setting
(#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: https://github.com/facebook/mysql-5.6/pull/1316
Differential Revision: D46470667
---
sql/dd/cache/dictionary_client.h | 20 +++++++++------
sql/dd/dd_utility.h | 17 +++++++++++++
sql/dd/impl/cache/dictionary_client.cc | 34 ++++++--------------------
sql/dd/impl/tables/dd_properties.cc | 8 ++----
sql/dd/impl/types/table_impl.cc | 11 +++------
5 files changed, 43 insertions(+), 47 deletions(-)
diff --git a/sql/dd/cache/dictionary_client.h b/sql/dd/cache/dictionary_client.h
index 685eba172981..e657fc2bfc78 100644
--- a/sql/dd/cache/dictionary_client.h
+++ b/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.
diff --git a/sql/dd/dd_utility.h b/sql/dd/dd_utility.h
index 651c46807daa..2820deb89265 100644
--- a/sql/dd/dd_utility.h
+++ b/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
diff --git a/sql/dd/impl/cache/dictionary_client.cc b/sql/dd/impl/cache/dictionary_client.cc
index ea2b66a74a7f..38ab7cf6c5df 100644
--- a/sql/dd/impl/cache/dictionary_client.cc
+++ b/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);
}
diff --git a/sql/dd/impl/tables/dd_properties.cc b/sql/dd/impl/tables/dd_properties.cc
index 91157787a3d7..ea621a25b6ac 100644
--- a/sql/dd/impl/tables/dd_properties.cc
+++ b/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()) {
diff --git a/sql/dd/impl/types/table_impl.cc b/sql/dd/impl/types/table_impl.cc
index 6aa1004d839a..2f146119bf3f 100644
--- a/sql/dd/impl/types/table_impl.cc
+++ b/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>();