April 2025 - commits - lists.mariadb.org

Review of MDEV-33675 Assertion(reclength < vreclength) in setup_vcols_for_repair()
by Michael Widenius 22 Apr '25

22 Apr '25

Hi! Review of MDEV-33675 Assertion(reclength < vreclength) in setup_vcols_for_repair() Thanks for the clear explanation of the problem. Please add that this problem only occurs with tables where there is no data for the main record outside of the null bits. As slightly smaller patch is (same idea applies for the Aria patch): +++ b/storage/myisam/ha_myisam.cc @@ -367,13 +367,13 @@ int table2myisam(TABLE *table_arg, MI_KEYDEF **keydef_out, while (recpos < (uint) share->… [View More]

1 0

Review of MDEV-36213 Doubled memory usage
by Michael Widenius 16 Apr '25

16 Apr '25

Review of: MDEV-36213 Doubled memory usage (11.4.4 <-> 11.4.5) Fixing the code adding MySQL _0900_ collations as _uca1400_ aliases not to perform deep initialization of the corresponding _uca1400_ collations. ----- > b. it introduced cyclic dependency between /mysys and /strings > - /mysys/charset-def.c depended on /strings/ctype-uca.c > - /strings/ctype-uca.c depended on /mysys/charset.c Please remove the above comment as this patch is not removing the dependencies. … [View More] Cyclic dependencies between my_sys and strings is not a problem. We always had it and will probably always will. There are 10 files in strings that includes my_sys.h You should think that string manipulations and main character set should be in strings and other things mysys. The separation mainly is to make things easier to find. > MY_CHARSET_LOADER::once_alloc call, which normally points to my_once_alloc(). I think it always points to once_alloc in the server code. conf_to_src.c, which uses malloc, is only used in uca-dump. I think you can change the commit comment to say that in the server all collations are allocated through my_once_alloc(). +++ b/mysql-test/main/ctype_utf8mb4_0900_mem.result +# +# I_S queries for initialized collations should not add memory +# +2416 utf8mb4_uca1400_spanish2_ai_ci +2417 utf8mb4_uca1400_spanish2_ai_cs +2418 utf8mb4_uca1400_spanish2_as_ci +2419 utf8mb4_uca1400_spanish2_as_cs +2420 utf8mb4_uca1400_spanish2_nopad_ai_ci +2421 utf8mb4_uca1400_spanish2_nopad_ai_cs +2422 utf8mb4_uca1400_spanish2_nopad_as_ci +2423 utf8mb4_uca1400_spanish2_nopad_as_cs +<1M utf8mb4_uca1400_spanish2% +270 utf8mb4_es_trad_0900_ai_ci +293 utf8mb4_es_trad_0900_as_cs +<1M utf8mb4_%es_trad_0900% The output is confusing to read. I was expecting memory checks for all rows. Please add # Reading collections before executing selects and # checking memory before the memory checks in the output. +--echo # End of 11.4 tests diff --git a/mysys/charset-def.c b/mysys/charset-def.c index e2e5a747eb5..5c2fac91773 100644 --- a/mysys/charset-def.c +++ b/mysys/charset-def.c @@ -184,76 +184,10 @@ extern struct charset_info_st my_charset_utf8mb4_unicode_520_nopad_ci; #endif /* HAVE_UCA_COLLATIONS */ -static my_bool -my_uca1400_collation_definition_add(MY_CHARSET_LOADER *loader, - my_cs_encoding_t charset_id, - uint tailoring_id, - my_bool nopad, - my_bool secondary_level, - my_bool tertiary_level) -{ - struct charset_info_st *tmp; - uint collation_id= my_uca1400_make_builtin_collation_id(charset_id, - tailoring_id, - nopad, - secondary_level, - tertiary_level); - if (!collation_id) - return FALSE; - if (!(tmp= (struct charset_info_st*) - my_once_alloc(sizeof(CHARSET_INFO),MYF(0)))) - return TRUE; - if (my_uca1400_collation_definition_init(loader, tmp, collation_id)) - return TRUE; - add_compiled_collation(tmp); - return FALSE; -} - It would have been VERY helpful, from a review point of view, if you would first have moved functions to different files in a separate commit and then do the changes. Changes that did something simple and mechanical, that could have been reviewed quickly could also have been in different commits, like adding my_loader to several function parameters. diff --git a/sql/field.cc b/sql/field.cc index 7358b142621..19fd3528bd0 100644 --- a/sql/field.cc +++ b/sql/field.cc @@ -2719,7 +2719,7 @@ bool Field_null::is_equal(const Column_definition &new_field) const { DBUG_ASSERT(!compression_method()); return (new_field.type_handler() == type_handler() && - !compare_collations(new_field.charset, field_charset()) && + new_field.charset->eq_collation(field_charset()) && new_field.length == max_display_length()); } Why not keep the old compare_collations() function and have it defined to do new_field.charset->eq_collation(param ?) Then you would not have to change the above function and 7 other places? Less not required changes makes merges easier! I think you should consider using compare_colations() instead if eq_collation() in the new code. The performance and code generated should be identical. index 3650eca61b9..84193ba3b97 100644 --- a/strings/ctype-uca.c @@ -39565,137 +39381,4 @@ LEX_CSTRING my_ci_get_collation_name_uca(CHARSET_INFO *cs, return cs->coll_name; } diff --git a/strings/ctype-uca0900.c b/strings/ctype-uca0900.c new file mode 100644 index 00000000000..11577daed44 --- /dev/null +++ b/strings/ctype-uca0900.c @@ -0,0 +1,209 @@ +/* Copyright (c) 2025, MariaDB Should be MariaDB Corporation <cut> +static my_bool +mysql_uca0900_collation_definition_add(MY_CHARSET_LOADER *loader, + const struct mysql_0900_to_mariadb_1400_mapping *map, + uint alias_id) Fix indentation for the above. Parameters should be on same level as ( As this is a static function, you can just call the function collation_definition_add() +{ + char comment_buffer[MY_CS_COLLATION_NAME_SIZE + 15]; + char alias_buffer[MY_CS_COLLATION_NAME_SIZE + 1]; + char name1400_buffer[MY_CS_COLLATION_NAME_SIZE + 1]; + LEX_CSTRING comment= {comment_buffer, 0}; + LEX_CSTRING alias_name= {alias_buffer, 0}; + LEX_CSTRING name1400= {name1400_buffer, 0}; + LEX_CSTRING utf8mb4= {STRING_WITH_LEN("utf8mb4")}; + uint offset_in_map= alias_id - mysql_0900_collation_start; + uint id1400= mysql_0900_mapping[offset_in_map].collation_id; The above two lines are the same as: uint id1400= map->collation_id; return my_uca1400_collation_alloc_and_init(loader, alias_name, +++ b/strings/ctype-uca0900.h @@ -0,0 +1,47 @@ +#ifndef CTYPE_UCA_0900_H +#define CTYPE_UCA_0900_H +/* Copyright (c) 2025, MariaDB MariaDB Corporation <cut> +++ b/strings/ctype-uca1400.c @@ -0,0 +1,434 @@ +/* Copyright (c) 2025, MariaDB MariaDB Corporation <cut> +#include "ctype-uca1400data.h" Remove extra line +extern struct charset_info_st my_charset_utf8mb3_unicode_520_nopad_ci; +extern struct charset_info_st my_charset_utf8mb3_unicode_520_ci; +extern struct charset_info_st my_charset_utf8mb4_unicode_520_nopad_ci; +extern struct charset_info_st my_charset_utf8mb4_unicode_520_ci; +extern struct charset_info_st my_charset_ucs2_unicode_520_nopad_ci; +extern struct charset_info_st my_charset_ucs2_unicode_520_ci; +extern struct charset_info_st my_charset_utf16_unicode_520_nopad_ci; +extern struct charset_info_st my_charset_utf16_unicode_520_ci; +extern struct charset_info_st my_charset_utf32_unicode_520_nopad_ci; +extern struct charset_info_st my_charset_utf32_unicode_520_ci; +extern struct charset_info_st my_charset_utf8mb4_turkish_uca_ci; Please add all external declarations in include files. This ensures all components sees exactly the same declarations. (We have had in the past extern declarations in source file that did not match with the real one and caused some not wanted effects). Normally a c/c++ source file (not include file) should never have any extern declarations! <cut> +/* + Make an UCA-14.0.0 full collation name using its id, + then allocate and add the collation. +*/ +static +my_bool my_uca1400_collation_definition_add(MY_CHARSET_LOADER *loader, uint id) +{ + char coll_name_buffer[MY_CS_COLLATION_NAME_SIZE + 1]; + LEX_CSTRING coll_name; + LEX_CSTRING comment= {"",0}; + uca_collation_def_param_t param= my_uca1400_collation_param_by_id(id); + CHARSET_INFO *src= my_uca1400_collation_source(param.cs_id, + param.nopad_flags); + const MY_UCA1400_COLLATION_DEFINITION *def= + &my_uca1400_collation_definitions[param.tailoring_id]; + + DBUG_ASSERT(id); Please add DBUG_ASSERT(id) before first usage of the id. <cut> +my_bool my_uca1400_collation_definitions_add(MY_CHARSET_LOADER *loader) <cut> + for (tertiary_level= 0; tertiary_level < 2; tertiary_level++) + { + uint collation_id= my_uca1400_make_builtin_collation_id(charset_id, + tailoring_id, + (my_bool) nopad, + (my_bool) secondary_level, + (my_bool) tertiary_level); Fix indentation for the above. +++ b/strings/ctype.c <cut> + +LEX_CSTRING my_ci_make_comment_for_alias(char *buffer, size_t buffer_size, + const char *srcname) +{ + LEX_CSTRING res= {buffer, 0}; + DBUG_ASSERT(buffer_size > 0); + res.length= strxnmov(buffer, buffer_size - 1, "Alias for ", srcname, NullS) - + buffer; + return res; +} Above is ONLY used in ctype-uca0900.c Please move it there and make it static. diff --git a/strings/strings_def.h b/strings/strings_def.h index 2c226e890c7..b137cc4af4f 100644 --- a/strings/strings_def.h +++ b/strings/strings_def.h @@ -20,6 +20,7 @@ #undef DBUG_ASSERT_AS_PRINTF #include <my_global.h> /* Define standard vars */ #include "m_string.h" /* Exernal definitions of string functions */ +#include "m_ctype.h" /* We can't use the original DBUG_ASSERT() (which includes _db_flush()) @@ -148,6 +149,16 @@ void my_ci_set_level_flags(struct charset_info_st *cs, uint flags); uint my_casefold_multiply_1(CHARSET_INFO *cs); uint my_casefold_multiply_2(CHARSET_INFO *cs); +my_bool my_ci_eq_collation_generic(CHARSET_INFO *self, CHARSET_INFO *other); + +LEX_CSTRING my_ci_make_comment_for_alias(char *buffer, size_t buffer_size, + const char *srcname); Above is ONLY used in ctype-uca0900.c Please move it there and make it static. <cut> Regards, Monty [View Less]

1 0

Review of MDL scalability regression after backup locks
by Michael Widenius 14 Apr '25

14 Apr '25

Hi! Review of: commit 5b0b5f529f760021e97bc20f33cb777733863bf2 Author: Marko Mäkelä <marko.makela(a)mariadb.com> Date: Fri Apr 11 14:43:07 2025 +0300 MDEV-19749 MDL scalability regression after backup locks diff --git a/sql/backup.cc b/sql/backup.cc index f634a11f867..a312904dc70 100644 --- a/sql/backup.cc +++ b/sql/backup.cc @@ -48,29 +48,49 @@ static const char *stage_names[]= TYPELIB backup_stage_names= { array_elements(stage_names)-1, "", stage_names, 0 }; -static … [View More]MDL_ticket *backup_flush_ticket; +static MDL_ticket *backup_flush_ticket= 0; +std::atomic<uint> THD::backup_commit_lock_enabled; + static File volatile backup_log= -1; static int backup_log_error= 0; -static bool backup_start(THD *thd); -static bool backup_flush(THD *thd); -static bool backup_block_ddl(THD *thd); -static bool backup_block_commit(THD *thd); -static bool start_ddl_logging(); -static void stop_ddl_logging(); +static bool backup_start(THD *thd) noexcept; +static bool backup_flush(THD *thd) noexcept; +static bool backup_block_ddl(THD *thd) noexcept; +static bool backup_block_commit(THD *thd) noexcept; +static bool start_ddl_logging() noexcept; +static void stop_ddl_logging() noexcept; Please do not do the above not critical changes (adding noexcept) in this commit that has nothing to with the above code. Try to keep commits in active trees to minimal! If you want to do the above, do that in a different commit, preferably in 12.0 tree. We could do this file by file. -/** - Run next stage of backup -*/ -void backup_init() +/* Initialize backup variables */ + +void backup_init() noexcept { Remove noexcept - backup_flush_ticket= 0; + DBUG_ASSERT(backup_flush_ticket == 0); + DBUG_ASSERT(THD::backup_commit_lock_enabled == 0); Please reset all variables in init. This is good practice to as it ensures things works better with the embedded server! It also shows clearly which variables are used by backup. backup_log= -1; backup_log_error= 0; } -bool run_backup_stage(THD *thd, backup_stages stage) + +#ifndef DBUG_OFF +void backup_reset() noexcept +{ + /* + Ensure that disable_backup_commit_locks() has been called for all + calls to enable_backup_commit_locks(). + */ + DBUG_ASSERT(THD::backup_commit_lock_enabled == 0); + DBUG_ASSERT(backup_flush_ticket == 0); +} +#endif Remove DBUG_OFF and noexcept. It is good to to have a function to reset / check things at the end. The above code is also needed when compiling without DBUG but have DBUG_ASSERT enabled as printf, something we do support and can be useful for debugging things at customers. + +/** + Run next stage of backup +*/ + +bool run_backup_stage(THD *thd, backup_stages stage) noexcept Remove noexcept here and in other places for this patch. + +/** @return whether a thread is in "fast path" THD::protect_against_backup() */ Please split comments over many rows +static my_bool check_if_unblocked(THD *thd, void *) noexcept +{ + return thd->backup_commit_lock.load(std::memory_order_acquire) == + THD::IN_COMMIT_NO_LOCK; +} Why change the name of the function ? I think that check_if_thd_in_commit() is a more clear name. check_if_thd_is_excuting_commit would be even better. Remove also noexcept ! + +/* + Wait until all active commits are done and all server threads knows + that backup has started. + This will cause ha_commit_trans() to use MDL locks to enable protection + commits under FTWRL and BACKUP STAGE BLOCK_COMMIT +*/ + +bool THD::enable_backup_commit_locks() noexcept +{ + /* Wait for all THD to start taking backup MDL locks during commit */ + THD_STAGE_INFO(this, stage_backup_setup_mdl_locks); + + /* Notify THD::protect_against_backup() that we are interested in them. */ + backup_commit_lock_enabled.fetch_add(1, std::memory_order_release); + + /* + Ensure that all threads are doing commits with mdl backup locks active. + */ + while (server_threads.iterate(check_if_unblocked, static_cast<void*>(0))) + { + if (killed) + { + disable_backup_commit_locks(); + return 1; + } + my_sleep(100); + } + THD_STAGE_INFO(this, stage_backup_got_commit_lock); + return 0; +} + + +void THD::disable_backup_commit_locks() noexcept +{ + IF_DBUG(auto l=,) Please do not use auto variables ! I REALLY dislike them! + backup_commit_lock_enabled.fetch_sub(1, std::memory_order_relaxed); + DBUG_ASSERT(l); +} + + Please add back my comment: /* Mark that the THD is about to execute a commit. If backup is already running, the caller needs to take a mdl lock instead */ +ATTRIBUTE_COLD bool THD::protect_against_backup_slow() noexcept +{ + /* The slow path: First, update our state, then wait for mdl_backup. */ + DBUG_ASSERT(this == current_thd); Remove assert. not needed and possible not required. + + backup_commit_lock.store(IN_COMMIT_WITH_LOCK, std::memory_order_release); + if (likely(!mdl_context.acquire_lock + (&mdl_backup, variables.lock_wait_timeout))) Fix indentation: if (likely(!mdl_context.acquire_lock(&mdl_backup, variables.lock_wait_timeout))) + return false; + + backup_commit_lock.store(OUTSIDE_COMMIT, std::memory_order_release); + return true; +} cut> -void backup_set_alter_copy_lock(THD *thd, TABLE *table) +void backup_set_alter_copy_lock(THD *thd, TABLE *table) noexcept { - MDL_ticket *ticket= thd->mdl_backup_ticket; - - /* Ticket maybe NULL in case of LOCK TABLES or for temporary tables*/ - DBUG_ASSERT(ticket || thd->locked_tables_mode || - table->s->tmp_table != NO_TMP_TABLE); - if (ticket) + if (MDL_ticket *ticket= thd->mdl_backup_ticket) ticket->downgrade_lock(MDL_BACKUP_ALTER_COPY); + else + /* Ticket maybe NULL in case of LOCK TABLES or for temporary tables*/ + DBUG_ASSERT(thd->locked_tables_mode || + table->s->tmp_table != NO_TMP_TABLE); } Please revert code. Old one was easier to read. <cut> index 2e5c3a58ba2..bd235dcea22 100644 --- a/sql/backup.h +++ b/sql/backup.h Remove all noexcept @@ -35,13 +35,18 @@ struct backup_log_info { bool new_partitioned; }; -void backup_init(); -bool run_backup_stage(THD *thd, backup_stages stage); -bool backup_end(THD *thd); -void backup_set_alter_copy_lock(THD *thd, TABLE *altered_table); -bool backup_reset_alter_copy_lock(THD *thd); - -bool backup_lock(THD *thd, TABLE_LIST *table); -void backup_unlock(THD *thd); -void backup_log_ddl(const backup_log_info *info); +void backup_init() noexcept; +#ifndef DBUG_OFF +void backup_reset() noexcept; +#else +# define backup_reset() /* empty */ +#endif Remove the above, keep original code. (See comment later) +bool run_backup_stage(THD *thd, backup_stages stage) noexcept; +bool backup_end(THD *thd) noexcept; +void backup_set_alter_copy_lock(THD *thd, TABLE *altered_table) noexcept; +bool backup_reset_alter_copy_lock(THD *thd) noexcept; + +bool backup_lock(THD *thd, TABLE_LIST *table) noexcept; +void backup_unlock(THD *thd) noexcept; +void backup_log_ddl(const backup_log_info *info) noexcept; #endif /* BACKUP_INCLUDED */ diff --git a/sql/handler.cc b/sql/handler.cc index 0088b84a2ec..1ec60f0e811 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -1785,19 +1784,11 @@ int ha_commit_trans(THD *thd, bool all) We allow the owner of FTWRL to COMMIT; we assume that it knows what it does. */ - MDL_REQUEST_INIT(&mdl_backup, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, - MDL_EXPLICIT); - - if (!WSREP(thd)) + if (!WSREP(thd) && thd->protect_against_backup()) { - if (thd->mdl_context.acquire_lock(&mdl_backup, - thd->variables.lock_wait_timeout)) - { - my_error(ER_ERROR_DURING_COMMIT, MYF(0), 1); - ha_rollback_trans(thd, all); - DBUG_RETURN(1); - } - thd->backup_commit_lock= &mdl_backup; + my_error(ER_ERROR_DURING_COMMIT, MYF(0), 1); + ha_rollback_trans(thd, all); + DBUG_RETURN(1); } DEBUG_SYNC(thd, "ha_commit_trans_after_acquire_commit_lock"); } @@ -2047,17 +2038,14 @@ int ha_commit_trans(THD *thd, bool all) thd->rgi_slave->is_parallel_exec); } end: - if (mdl_backup.ticket) - { - /* - We do not always immediately release transactional locks - after ha_commit_trans() (see uses of ha_enable_transaction()), - thus we release the commit blocker lock as soon as it's - not needed. - */ - thd->mdl_context.release_lock(mdl_backup.ticket); - thd->backup_commit_lock= 0; - } + /* + We do not always immediately release transactional locks + after ha_commit_trans() (see uses of ha_enable_transaction()), + thus we release the commit blocker lock as soon as it's + not needed. + */ + thd->unprotect_against_backup(); + #ifdef WITH_WSREP if (wsrep_is_active(thd) && is_real_trans && !error && (rw_ha_count == 0 || all) && diff --git a/sql/lock.cc b/sql/lock.cc index ef8c2ba3c86..f32b1a57011 100644 --- a/sql/lock.cc +++ b/sql/lock.cc @@ -1125,6 +1125,7 @@ bool Global_read_lock::lock_global_read_lock(THD *thd) void Global_read_lock::unlock_global_read_lock(THD *thd) { + bool have_backup_commit_lock; DBUG_ENTER("unlock_global_read_lock"); DBUG_ASSERT(m_mdl_global_read_lock && m_state); @@ -1138,7 +1139,16 @@ void Global_read_lock::unlock_global_read_lock(THD *thd) } } + /* + The backup commit lock was only taken and hold in + make_global_read_lock_block_commit() if the current lock is + FTWRL2 + */ + have_backup_commit_lock= (m_mdl_global_read_lock->get_type() == + MDL_BACKUP_FTWRL2); thd->mdl_context.release_lock(m_mdl_global_read_lock); + if (have_backup_commit_lock) + thd->disable_backup_commit_locks(); #ifdef WITH_WSREP if (m_state == GRL_ACQUIRED_AND_BLOCKS_COMMIT && @@ -1199,10 +1209,16 @@ bool Global_read_lock::make_global_read_lock_block_commit(THD *thd) if (m_state != GRL_ACQUIRED) DBUG_RETURN(0); + if (thd->enable_backup_commit_locks()) + DBUG_RETURN(TRUE); + if (thd->mdl_context.upgrade_shared_lock(m_mdl_global_read_lock, MDL_BACKUP_FTWRL2, thd->variables.lock_wait_timeout)) + { + thd->disable_backup_commit_locks(); DBUG_RETURN(TRUE); + } m_state= GRL_ACQUIRED_AND_BLOCKS_COMMIT; diff --git a/sql/log.cc b/sql/log.cc index 2f9eab73812..b97d8af19e2 100644 --- a/sql/log.cc +++ b/sql/log.cc @@ -6951,20 +6951,12 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate) uint64 commit_id= 0; MDL_request mdl_request; DBUG_PRINT("info", ("direct is set")); - DBUG_ASSERT(!thd->backup_commit_lock); - MDL_REQUEST_INIT(&mdl_request, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, - MDL_EXPLICIT); - if (thd->mdl_context.acquire_lock(&mdl_request, - thd->variables.lock_wait_timeout)) + if (thd->protect_against_backup()) DBUG_RETURN(1); - thd->backup_commit_lock= &mdl_request; - if ((res= thd->wait_for_prior_commit())) { - if (mdl_request.ticket) - thd->mdl_context.release_lock(mdl_request.ticket); - thd->backup_commit_lock= 0; + thd->unprotect_against_backup(); DBUG_RETURN(res); } file= &log_file; @@ -6982,9 +6974,7 @@ bool MYSQL_BIN_LOG::write(Log_event *event_info, my_bool *with_annotate) commit_id= entry->val_int(&null_value); }); res= write_gtid_event(thd, true, using_trans, commit_id); - if (mdl_request.ticket) - thd->mdl_context.release_lock(mdl_request.ticket); - thd->backup_commit_lock= 0; + thd->unprotect_against_backup(); if (res) goto err; } @@ -8088,7 +8078,7 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) wait_for_commit *wfc; bool backup_lock_released= 0; int result= 0; - THD *thd= orig_entry->thd; + THD *const thd= orig_entry->thd; DBUG_ENTER("MYSQL_BIN_LOG::queue_for_group_commit"); DBUG_ASSERT(thd == current_thd); @@ -8100,7 +8090,7 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) another safe check under lock, to avoid the race where the other transaction wakes us up between the check and the wait. */ - wfc= orig_entry->thd->wait_for_commit_ptr; + wfc= thd->wait_for_commit_ptr; orig_entry->queued_by_other= false; if (wfc && wfc->waitee.load(std::memory_order_acquire)) { @@ -8129,12 +8119,14 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) yet have the MDL_BACKUP_COMMIT_LOCK) and any threads using BACKUP LOCK BLOCK_COMMIT. */ - if (thd->backup_commit_lock && thd->backup_commit_lock->ticket && - !backup_lock_released) + + if (!backup_lock_released && + thd->backup_commit_lock.load(std::memory_order_relaxed) == + THD::IN_COMMIT_WITH_LOCK) { - backup_lock_released= 1; - thd->mdl_context.release_lock(thd->backup_commit_lock->ticket); - thd->backup_commit_lock->ticket= 0; + backup_lock_released= true; + thd->mdl_context.release_lock(thd->mdl_backup.ticket); + thd->mdl_backup.ticket= nullptr; } /* @@ -8152,13 +8144,12 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) we have been woken up. */ wfc->opaque_pointer= orig_entry; - DEBUG_SYNC(orig_entry->thd, "group_commit_waiting_for_prior"); - orig_entry->thd->ENTER_COND(&wfc->COND_wait_commit, - &wfc->LOCK_wait_commit, - &stage_waiting_for_prior_transaction_to_commit, - &old_stage); + DEBUG_SYNC(thd, "group_commit_waiting_for_prior"); + thd->ENTER_COND(&wfc->COND_wait_commit, &wfc->LOCK_wait_commit, + &stage_waiting_for_prior_transaction_to_commit, + &old_stage); while ((loc_waitee= wfc->waitee.load(std::memory_order_relaxed)) && - !orig_entry->thd->check_killed(1)) + !thd->check_killed(1)) mysql_cond_wait(&wfc->COND_wait_commit, &wfc->LOCK_wait_commit); wfc->opaque_pointer= NULL; DBUG_PRINT("info", ("After waiting for prior commit, queued_by_other=%d", @@ -8189,19 +8180,19 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) */ wfc->waitee.store(NULL, std::memory_order_relaxed); - orig_entry->thd->EXIT_COND(&old_stage); + thd->EXIT_COND(&old_stage); /* Interrupted by kill. */ - DEBUG_SYNC(orig_entry->thd, "group_commit_waiting_for_prior_killed"); - wfc->wakeup_error= orig_entry->thd->killed_errno(); + DEBUG_SYNC(thd, "group_commit_waiting_for_prior_killed"); + wfc->wakeup_error= thd->killed_errno(); if (!wfc->wakeup_error) wfc->wakeup_error= ER_QUERY_INTERRUPTED; my_message(wfc->wakeup_error, - ER_THD(orig_entry->thd, wfc->wakeup_error), MYF(0)); + ER_THD(thd, wfc->wakeup_error), MYF(0)); result= -1; goto end; } } - orig_entry->thd->EXIT_COND(&old_stage); + thd->EXIT_COND(&old_stage); } else mysql_mutex_unlock(&wfc->LOCK_wait_commit); @@ -8222,8 +8213,8 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) } /* Now enqueue ourselves in the group commit queue. */ - DEBUG_SYNC(orig_entry->thd, "commit_before_enqueue"); - orig_entry->thd->clear_wakeup_ready(); + DEBUG_SYNC(thd, "commit_before_enqueue"); + thd->clear_wakeup_ready(); mysql_mutex_lock(&LOCK_prepare_ordered); orig_queue= group_commit_queue; @@ -8268,9 +8259,9 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) if (entry->cache_mngr->using_xa) { - DEBUG_SYNC(orig_entry->thd, "commit_before_prepare_ordered"); + DEBUG_SYNC(thd, "commit_before_prepare_ordered"); run_prepare_ordered(entry->thd, entry->all); - DEBUG_SYNC(orig_entry->thd, "commit_after_prepare_ordered"); + DEBUG_SYNC(thd, "commit_after_prepare_ordered"); } if (cur) @@ -8391,14 +8382,14 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) if (opt_binlog_commit_wait_count > 0 && orig_queue != NULL) mysql_cond_signal(&COND_prepare_ordered); mysql_mutex_unlock(&LOCK_prepare_ordered); - DEBUG_SYNC(orig_entry->thd, "commit_after_release_LOCK_prepare_ordered"); + DEBUG_SYNC(thd, "commit_after_release_LOCK_prepare_ordered"); DBUG_PRINT("info", ("Queued for group commit as %s", (orig_queue == NULL) ? "leader" : "participant")); end: if (backup_lock_released) - thd->mdl_context.acquire_lock(thd->backup_commit_lock, + thd->mdl_context.acquire_lock(&thd->mdl_backup, thd->variables.lock_wait_timeout); DBUG_RETURN(result); } diff --git a/sql/mysqld.cc b/sql/mysqld.cc index e7a0daedc41..c817f7e8307 100644 --- a/sql/mysqld.cc +++ b/sql/mysqld.cc @@ -742,7 +742,7 @@ mysql_mutex_t LOCK_thread_id; server may be fairly high, we need a dedicated lock. */ mysql_mutex_t LOCK_prepared_stmt_count; -mysql_mutex_t LOCK_backup_log; +mysql_mutex_t LOCK_backup_log, LOCK_backup_commit; mysql_rwlock_t LOCK_grant, LOCK_sys_init_connect, LOCK_sys_init_slave; mysql_rwlock_t LOCK_ssl_refresh; mysql_rwlock_t LOCK_all_status_vars; @@ -921,7 +921,7 @@ PSI_mutex_key key_BINLOG_LOCK_index, key_BINLOG_LOCK_xid_list, key_LOCK_crypt, key_LOCK_delayed_create, key_LOCK_delayed_insert, key_LOCK_delayed_status, key_LOCK_error_log, key_LOCK_gdl, key_LOCK_global_system_variables, - key_LOCK_manager, key_LOCK_backup_log, + key_LOCK_manager, key_LOCK_backup_log, key_LOCK_backup_commit, key_LOCK_prepared_stmt_count, key_LOCK_rpl_status, key_LOCK_server_started, key_LOCK_status, key_LOCK_temp_pool, @@ -985,6 +985,7 @@ static PSI_mutex_info all_server_mutexes[]= { &key_hash_filo_lock, "hash_filo::lock", 0}, { &key_LOCK_active_mi, "LOCK_active_mi", PSI_FLAG_GLOBAL}, { &key_LOCK_backup_log, "LOCK_backup_log", PSI_FLAG_GLOBAL}, + { &key_LOCK_backup_commit, "LOCK_backup_commit", PSI_FLAG_GLOBAL}, { &key_LOCK_temp_pool, "LOCK_temp_pool", PSI_FLAG_GLOBAL}, { &key_LOCK_thread_id, "LOCK_thread_id", PSI_FLAG_GLOBAL}, { &key_LOCK_crypt, "LOCK_crypt", PSI_FLAG_GLOBAL}, @@ -2049,6 +2050,7 @@ static void clean_up(bool print_message) logger.cleanup_end(); sys_var_end(); free_charsets(); + backup_reset(); my_free(const_cast<char*>(log_bin_basename)); my_free(const_cast<char*>(log_bin_index)); @@ -2140,6 +2142,7 @@ static void clean_up_mutexes() mysql_mutex_destroy(&LOCK_active_mi); mysql_rwlock_destroy(&LOCK_ssl_refresh); mysql_mutex_destroy(&LOCK_backup_log); + mysql_mutex_destroy(&LOCK_backup_commit); mysql_mutex_destroy(&LOCK_temp_pool); mysql_rwlock_destroy(&LOCK_sys_init_connect); mysql_rwlock_destroy(&LOCK_sys_init_slave); @@ -4502,6 +4505,7 @@ static int init_thread_environment() MY_MUTEX_INIT_SLOW); mysql_mutex_init(key_LOCK_backup_log, &LOCK_backup_log, MY_MUTEX_INIT_FAST); mysql_mutex_init(key_LOCK_temp_pool, &LOCK_temp_pool, MY_MUTEX_INIT_FAST); + mysql_mutex_init(key_LOCK_backup_commit, &LOCK_backup_commit, MY_MUTEX_INIT_FAST); #ifdef HAVE_OPENSSL #ifdef HAVE_des @@ -9391,6 +9395,8 @@ PSI_stage_info stage_waiting_for_deadlock_kill= { 0, "Waiting for parallel repli PSI_stage_info stage_starting= { 0, "starting", 0}; PSI_stage_info stage_waiting_for_flush= { 0, "Waiting for non trans tables to be flushed", 0}; PSI_stage_info stage_waiting_for_ddl= { 0, "Waiting for DDLs", 0}; +PSI_stage_info stage_backup_setup_mdl_locks= { 0, "Enabling backup commit lock", 0}; +PSI_stage_info stage_backup_got_commit_lock= { 0, "Backup commit lock enabled", 0}; #ifdef WITH_WSREP // Aditional Galera thread states diff --git a/sql/mysqld.h b/sql/mysqld.h index 7dcf14e3532..6a90e8be2b0 100644 --- a/sql/mysqld.h +++ b/sql/mysqld.h @@ -692,6 +692,9 @@ extern PSI_stage_info stage_slave_background_process_request; extern PSI_stage_info stage_slave_background_wait_request; extern PSI_stage_info stage_waiting_for_deadlock_kill; extern PSI_stage_info stage_starting; +extern PSI_stage_info stage_backup_setup_mdl_locks; +extern PSI_stage_info stage_backup_got_commit_lock; + #ifdef WITH_WSREP // Aditional Galera thread states extern PSI_stage_info stage_waiting_isolation; @@ -768,7 +771,8 @@ extern mysql_mutex_t LOCK_error_log, LOCK_delayed_insert, LOCK_short_uuid_generator, LOCK_delayed_status, LOCK_delayed_create, LOCK_crypt, LOCK_timezone, LOCK_active_mi, LOCK_manager, LOCK_user_conn, - LOCK_prepared_stmt_count, LOCK_error_messages, LOCK_backup_log; + LOCK_prepared_stmt_count, LOCK_error_messages, + LOCK_backup_log, LOCK_backup_commit; extern MYSQL_PLUGIN_IMPORT mysql_mutex_t LOCK_global_system_variables; extern mysql_rwlock_t LOCK_all_status_vars; extern mysql_mutex_t LOCK_start_thread; diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 20ba2ca7a66..88da104ddd5 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -1345,7 +1345,9 @@ void THD::init() first_successful_insert_id_in_prev_stmt_for_binlog= 0; first_successful_insert_id_in_cur_stmt= 0; current_backup_stage= BACKUP_FINISHED; - backup_commit_lock= 0; + backup_commit_lock.store(OUTSIDE_COMMIT, std::memory_order_relaxed); + MDL_REQUEST_INIT(&mdl_backup, MDL_key::BACKUP, "", "", MDL_BACKUP_COMMIT, + MDL_EXPLICIT); #ifdef WITH_WSREP wsrep_last_query_id= 0; wsrep_xid.null(); @@ -8304,7 +8306,8 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill) { PSI_stage_info old_stage; wait_for_commit *loc_waitee; - bool backup_lock_released= false; + + DBUG_ASSERT(thd == current_thd); /* Release MDL_BACKUP_COMMIT LOCK while waiting for other threads to commit @@ -8312,11 +8315,14 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill) yet have the MDL_BACKUP_COMMIT_LOCK) and any threads using BACKUP LOCK BLOCK_COMMIT. */ - if (thd->backup_commit_lock && thd->backup_commit_lock->ticket) + const bool backup_lock_released= + thd->backup_commit_lock.load(std::memory_order_relaxed) == + THD::IN_COMMIT_WITH_LOCK; + + if (backup_lock_released) { - backup_lock_released= true; - thd->mdl_context.release_lock(thd->backup_commit_lock->ticket); - thd->backup_commit_lock->ticket= 0; + thd->mdl_context.release_lock(thd->mdl_backup.ticket); + thd->mdl_backup.ticket= nullptr; } mysql_mutex_lock(&LOCK_wait_commit); @@ -8331,7 +8337,9 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill) { if (wakeup_error) my_error(ER_PRIOR_COMMIT_FAILED, MYF(0)); - goto end; + end: + thd->EXIT_COND(&old_stage); + goto func_exit; } /* Wait was interrupted by kill. We need to unregister our wait and give the @@ -8367,15 +8375,9 @@ wait_for_commit::wait_for_prior_commit2(THD *thd, bool allow_kill) use within enter_cond/exit_cond. */ DEBUG_SYNC(thd, "wait_for_prior_commit_killed"); +func_exit: if (unlikely(backup_lock_released)) - thd->mdl_context.acquire_lock(thd->backup_commit_lock, - thd->variables.lock_wait_timeout); - return wakeup_error; - -end: - thd->EXIT_COND(&old_stage); - if (unlikely(backup_lock_released)) - thd->mdl_context.acquire_lock(thd->backup_commit_lock, + thd->mdl_context.acquire_lock(&thd->mdl_backup, thd->variables.lock_wait_timeout); return wakeup_error; } diff --git a/sql/sql_class.h b/sql/sql_class.h index 98cd1e0d0c8..dde5d1ebca1 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -2736,10 +2736,49 @@ class THD: public THD_count, /* this must be first */ rpl_io_thread_info *rpl_io_info; rpl_sql_thread_info *rpl_sql_info; } system_thread_info; - /* Used for BACKUP LOCK */ - MDL_ticket *mdl_backup_ticket, *mdl_backup_lock; - /* Used to register that thread has a MDL_BACKUP_WAIT_COMMIT lock */ - MDL_request *backup_commit_lock; + /** Used in lock_table_names() */ + MDL_ticket *mdl_backup_ticket; + /** Used in BACKUP STAGE */ + MDL_ticket *mdl_backup_lock; + /** Pre-initialized MDL request for protect_against_backup() */ + MDL_request mdl_backup; + + /** the state of protect_against_backup() and unprotect_against_backup() */ + enum backup_locking_state { + OUTSIDE_COMMIT= 0, IN_COMMIT_NO_LOCK, IN_COMMIT_WITH_LOCK + }; + + /** whether the thread holds a MDL_BACKUP_WAIT_COMMIT lock */ + std::atomic<backup_locking_state> backup_commit_lock{OUTSIDE_COMMIT}; + + /** the difference of enable_backup_commit_locks() and + disable_backup_commit_locks() calls */ Please change to: /* The difference of enable_backup_commit_locks() and disable_backup_commit_locks() calls */ + static std::atomic<uint> backup_commit_lock_enabled; + + /** @return whether we failed to acquire mdl_backup when it is necessary */ + bool protect_against_backup() noexcept + { + DBUG_ASSERT(!backup_commit_lock.load(std::memory_order_relaxed)); + backup_commit_lock.store(IN_COMMIT_NO_LOCK, std::memory_order_release); + /* We must have updated our state before the load below, to guarantee that + enable_backup_commit_locks() will account for us. */ Fix comment indentation + return + unlikely(backup_commit_lock_enabled.load(std::memory_order_acquire)) && + protect_against_backup_slow(); use return (...) and indent accordingly. + } There is an issue with your patch here. You are using IN_COMMIT_NO_LOCK even if backup MDL locks are enabled and the other thread waitin for no threads with IN_COMMIT_NO_LOCK. On a busy server, it is in theory possible that THD::enable_backup_commit_locks() will never return as there is always a thread between setting IN_COMMIT_NO_LOCK and changing it to IN_COMMIT_NO_LOCK. A safer strategy would be to check first for backup_commit_lock_enabled.load(std::memory_order_acquire) and if yes set it to IN_COMMIT_LOCK and request MDL lock. There is no problem if a thread would use MDL locks accidently while unprotect_against_backup() is running. Add double empty lines between functions to make this part easier to read. + void unprotect_against_backup() noexcept + { + backup_locking_state s= backup_commit_lock.load(std::memory_order_relaxed); + backup_commit_lock.store(OUTSIDE_COMMIT, std::memory_order_release); + if (unlikely(s == IN_COMMIT_WITH_LOCK)) + unprotect_against_backup_slow(); + } add 2 empty lines here +private: + bool protect_against_backup_slow() noexcept; + void unprotect_against_backup_slow() noexcept; +public: + bool enable_backup_commit_locks() noexcept; + static void disable_backup_commit_locks() noexcept; void reset_for_next_command(bool do_clear_errors= 1); [View Less]

3 3