From bebeee689df23f09c64500f12ce075deb6a1b76d Mon Sep 17 00:00:00 2001 From: Laurynas Biveinis Date: Mon, 29 May 2023 11:32:32 +0300 Subject: [PATCH 1/2] Move default PATH setting from CMake to get_rocksdb_files.sh In c3f1703231d7fec34de1479b8913a06cd9c4cd79, get_rocksdb_files.sh invocation got PATH setting. The public discussion is incomplete but b739eac1760c2451200246c5d7fe3233787053b8 suggests that there is a problem with CentOS 9 and make 4.3 that undefined PATH does not get a default fallback, thus a PATH value is provided for the invocation. The provided PATH value is however incompatible with building on macOS. Fix by setting the fallback PATH value in the get_rocksdb_files.sh script itself. --- storage/rocksdb/CMakeLists.txt | 2 +- storage/rocksdb/get_rocksdb_files.sh | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/storage/rocksdb/CMakeLists.txt b/storage/rocksdb/CMakeLists.txt index 2baba594f096..b9bb20611608 100644 --- a/storage/rocksdb/CMakeLists.txt +++ b/storage/rocksdb/CMakeLists.txt @@ -52,7 +52,7 @@ ELSE() # get a list of rocksdb library source files # run with env -i to avoid passing variables EXECUTE_PROCESS( - COMMAND env -i PATH="/sbin:/usr/sbin:/bin:/usr/bin" ${CMAKE_SOURCE_DIR}/storage/rocksdb/get_rocksdb_files.sh ${ROCKSDB_FOLLY} + COMMAND env -i ${CMAKE_SOURCE_DIR}/storage/rocksdb/get_rocksdb_files.sh ${ROCKSDB_FOLLY} OUTPUT_VARIABLE SCRIPT_OUTPUT WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} ) diff --git a/storage/rocksdb/get_rocksdb_files.sh b/storage/rocksdb/get_rocksdb_files.sh index 2f60343beec8..756bc9ec25bb 100755 --- a/storage/rocksdb/get_rocksdb_files.sh +++ b/storage/rocksdb/get_rocksdb_files.sh @@ -1,4 +1,8 @@ #!/bin/bash +if [ -z "$PATH" ]; then + export PATH="/sbin:/usr/sbin:/bin:/usr/bin" +fi + MKFILE=`mktemp` # create and run a simple makefile # include rocksdb make file relative to the path of this script From 3dae3efb55fbc9e824075df857e1780e733e48b0 Mon Sep 17 00:00:00 2001 From: Laurynas Biveinis Date: Mon, 5 Jun 2023 21:26:22 +0300 Subject: [PATCH 2/2] Refactor data dictionary transaction isolation setting 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. --- sql/dd/cache/dictionary_client.h | 20 +++++++++------ sql/dd/dd_utility.h | 16 ++++++++++++ sql/dd/impl/cache/dictionary_client.cc | 34 ++++++-------------------- sql/dd/impl/tables/dd_properties.cc | 8 ++---- sql/dd/impl/types/table_impl.cc | 7 ++---- 5 files changed, 40 insertions(+), 45 deletions(-) diff --git a/sql/dd/cache/dictionary_client.h b/sql/dd/cache/dictionary_client.h index 6e63b4f66331..2678218b397c 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. @@ -1043,9 +1045,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. @@ -1069,7 +1072,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 00e63205f3a7..8c6a518063dd 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" +#include "sql/mysqld.h" struct CHARSET_INFO; class THD; @@ -63,6 +65,20 @@ size_t normalize_string(const CHARSET_INFO *cs, const String_type &src, */ bool check_if_server_ddse_readonly(THD *thd, const char *schema_name); +/** + 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 13c9c005e438..a8d181040bf5 100644 --- a/sql/dd/impl/cache/dictionary_client.cc +++ b/sql/dd/impl/cache/dictionary_client.cc @@ -37,6 +37,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(), ... @@ -1223,11 +1224,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. @@ -1644,11 +1643,7 @@ static bool get_index_statistics_entries( THD *thd, const String_type &schema_name, const String_type &table_name, std::vector &index_names, std::vector &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(); @@ -1721,11 +1716,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; @@ -1754,12 +1745,8 @@ bool Dictionary_client::remove_table_dynamic_statistics( std::unique_ptr 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; @@ -2282,12 +2269,7 @@ template bool Dictionary_client::fetch_referencing_views_object_id( const char *schema, const char *tbl_or_sf_name, std::vector *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(); @@ -2331,7 +2313,7 @@ bool Dictionary_client::fetch_fk_children_uncached( std::vector *children_schemas, std::vector *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(); Raw_table *foreign_keys_table = trx.otx.get_table(); @@ -2810,7 +2792,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 62296de8f5f4..42f68842330e 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" @@ -117,12 +118,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(); if (trx.otx.open_tables()) { diff --git a/sql/dd/impl/types/table_impl.cc b/sql/dd/impl/types/table_impl.cc index 8882ed02b631..29c1b01197f3 100644 --- a/sql/dd/impl/types/table_impl.cc +++ b/sql/dd/impl/types/table_impl.cc @@ -66,6 +66,7 @@ #include "sql/dd/types/index.h" #include "sql/dd/types/partition.h" #include "sql/dd/types/weak_object.h" +#include "sql/dd/dd_utility.h" #include "sql/sql_class.h" using dd::tables::Check_constraints; @@ -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();