From 978e48c96cf22b1f17444dd54b6b64b28752881e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Thu, 4 Mar 2021 17:15:38 +0200 Subject: [PATCH 1/3] MDEV-25051 Race condition between persistent statistics and RENAME TABLE or TRUNCATE innobase_rename_table(): Invoke dict_stats_wait_bg_to_stop_using_table() to ensure that dict_stats_update() cannot be accessing the table name that we will be modifying. If we are executing RENAME rather than TRUNCATE, reset the flag at the end so that persistent statistics can be calculated again. The race condition was encountered with ASAN and rr. Sorry, there is no test case, like there is for nothing related to dict_stats_wait_bg_to_stop_using_table(). The entire code is an ugly work-around for the failure of dict_stats_process_entry_from_recalc_pool() to acquire MDL. Note: It appears that an ALTER TABLE that is not rebuilding the table will fail to reset the flag that blocks the processing of statistics. --- storage/innobase/handler/ha_innodb.cc | 32 +++++++++++++++------------ 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index c67bf6bd607..11d8ec2b81e 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -13518,17 +13518,10 @@ innobase_drop_database( @param[in,out] trx InnoDB data dictionary transaction @param[in] from old table name @param[in] to new table name -@param[in] commit whether to commit trx -@param[in] use_fk whether to parse and enforce FOREIGN KEY constraints +@param[in] commit whether to commit trx (and to enforce FOREIGN KEY) @return DB_SUCCESS or error code */ -inline -dberr_t -innobase_rename_table( - trx_t* trx, - const char* from, - const char* to, - bool commit, - bool use_fk) +inline dberr_t innobase_rename_table(trx_t *trx, const char *from, + const char *to, bool commit) { dberr_t error; char norm_to[FN_REFLEN]; @@ -13561,6 +13554,9 @@ innobase_rename_table( Convert lock_wait_timeout unit from second to 250 milliseconds */ long int lock_wait_timeout = thd_lock_wait_timeout(trx->mysql_thd) * 4; if (table != NULL) { + if (commit) { + dict_stats_wait_bg_to_stop_using_table(table, trx); + } for (dict_index_t* index = dict_table_get_first_index(table); index != NULL; index = dict_table_get_next_index(index)) { @@ -13574,7 +13570,9 @@ innobase_rename_table( } } } - dict_table_close(table, TRUE, FALSE); + if (!commit) { + dict_table_close(table, TRUE, FALSE); + } } /* FTS sync is in progress. We shall timeout this operation */ @@ -13589,7 +13587,7 @@ innobase_rename_table( ut_a(trx->will_lock > 0); error = row_rename_table_for_mysql(norm_from, norm_to, trx, commit, - use_fk); + commit); if (error != DB_SUCCESS) { if (error == DB_TABLE_NOT_FOUND @@ -13641,6 +13639,10 @@ innobase_rename_table( func_exit: if (commit) { + if (table) { + table->stats_bg_flag &= ~BG_STAT_SHOULD_QUIT; + dict_table_close(table, TRUE, FALSE); + } row_mysql_unlock_data_dictionary(trx); } @@ -13726,9 +13728,11 @@ int ha_innobase::truncate() ++trx->will_lock; trx_set_dict_operation(trx, TRX_DICT_OP_TABLE); row_mysql_lock_data_dictionary(trx); + dict_stats_wait_bg_to_stop_using_table(ib_table, trx); + int err = convert_error_code_to_mysql( innobase_rename_table(trx, ib_table->name.m_name, temp_name, - false, false), + false), ib_table->flags, m_user_thd); if (err) { trx_rollback_for_mysql(trx); @@ -13811,7 +13815,7 @@ ha_innobase::rename_table( ++trx->will_lock; trx_set_dict_operation(trx, TRX_DICT_OP_INDEX); - dberr_t error = innobase_rename_table(trx, from, to, true, true); + dberr_t error = innobase_rename_table(trx, from, to, true); DEBUG_SYNC(thd, "after_innobase_rename_table"); From 7759991a06d54630214f19eaa0ec39bd21bf09df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Thu, 4 Mar 2021 18:11:25 +0200 Subject: [PATCH 2/3] fixup 58b56f14a096285a0e51b2233fc35398f1b01f5a: Remove dead code row_prebuilt_t::m_no_prefetch: Remove (it was always false). row_prebuilt_t::m_read_virtual_key: Remove (it was always false). Only ha_innopart ever set these fields. --- storage/innobase/include/row0mysql.h | 8 +------- storage/innobase/row/row0mysql.cc | 5 +---- storage/innobase/row/row0sel.cc | 26 ++++---------------------- 3 files changed, 6 insertions(+), 33 deletions(-) diff --git a/storage/innobase/include/row0mysql.h b/storage/innobase/include/row0mysql.h index d5cdd4a3f17..8738f991368 100644 --- a/storage/innobase/include/row0mysql.h +++ b/storage/innobase/include/row0mysql.h @@ -1,7 +1,7 @@ /***************************************************************************** Copyright (c) 2000, 2017, Oracle and/or its affiliates. All Rights Reserved. -Copyright (c) 2017, 2019, MariaDB Corporation. +Copyright (c) 2017, 2021, MariaDB Corporation. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software @@ -784,12 +784,6 @@ struct row_prebuilt_t { search key values from MySQL format to InnoDB format.*/ uint srch_key_val_len; /*!< Size of search key */ - /** Disable prefetch. */ - bool m_no_prefetch; - - /** Return materialized key for secondary index scan */ - bool m_read_virtual_key; - /** The MySQL table object */ TABLE* m_mysql_table; }; diff --git a/storage/innobase/row/row0mysql.cc b/storage/innobase/row/row0mysql.cc index 312c35c58a3..c2f9186d408 100644 --- a/storage/innobase/row/row0mysql.cc +++ b/storage/innobase/row/row0mysql.cc @@ -1,7 +1,7 @@ /***************************************************************************** Copyright (c) 2000, 2018, Oracle and/or its affiliates. All Rights Reserved. -Copyright (c) 2015, 2020, MariaDB Corporation. +Copyright (c) 2015, 2021, MariaDB Corporation. This program is free software; you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software @@ -958,9 +958,6 @@ row_create_prebuilt( prebuilt->fts_doc_id_in_read_set = 0; prebuilt->blob_heap = NULL; - prebuilt->m_no_prefetch = false; - prebuilt->m_read_virtual_key = false; - DBUG_RETURN(prebuilt); } diff --git a/storage/innobase/row/row0sel.cc b/storage/innobase/row/row0sel.cc index 0451a240ffc..864f7bd54ab 100644 --- a/storage/innobase/row/row0sel.cc +++ b/storage/innobase/row/row0sel.cc @@ -2,7 +2,7 @@ Copyright (c) 1997, 2017, Oracle and/or its affiliates. All Rights Reserved. Copyright (c) 2008, Google Inc. -Copyright (c) 2015, 2020, MariaDB Corporation. +Copyright (c) 2015, 2021, MariaDB Corporation. Portions of this file contain modifications contributed and copyrighted by Google, Inc. Those modifications are gratefully acknowledged and are described @@ -3168,8 +3168,7 @@ static bool row_sel_store_mysql_rec( search or virtual key read is not requested. */ if (!rec_clust || !prebuilt->index->has_virtual() - || (!prebuilt->read_just_key - && !prebuilt->m_read_virtual_key)) { + || !prebuilt->read_just_key) { /* Initialize the NULL bit. */ mysql_rec[templ->mysql_null_byte_offset] |= (byte) templ->mysql_null_bit_mask; @@ -3185,23 +3184,8 @@ static bool row_sel_store_mysql_rec( const dfield_t* dfield = dtuple_get_nth_v_field( vrow, col->v_pos); - /* If this is a partitioned table, it might request - InnoDB to fill out virtual column data for serach - index key values while other non key columns are also - getting selected. The non-key virtual columns may - not be materialized and we should skip them. */ if (dfield_get_type(dfield)->mtype == DATA_MISSING) { -#ifdef UNIV_DEBUG - ulint prefix; -#endif /* UNIV_DEBUG */ - ut_ad(prebuilt->m_read_virtual_key); - - /* If it is part of index key the data should - have been materialized. */ - ut_ad(dict_index_get_nth_col_or_prefix_pos( - prebuilt->index, col->v_pos, false, - true, &prefix) == ULINT_UNDEFINED); - + ut_ad("no ha_innopart in MariaDB" == 0); continue; } @@ -4377,8 +4361,7 @@ row_search_mvcc( index key, if this is covered index scan or virtual key read is requested. */ bool need_vrow = dict_index_has_virtual(prebuilt->index) - && (prebuilt->read_just_key - || prebuilt->m_read_virtual_key); + && prebuilt->read_just_key; /* Reset the new record lock info if srv_locks_unsafe_for_binlog is set or session is using a READ COMMITED isolation level. Then @@ -5524,7 +5507,6 @@ use_covering_index: if ((match_mode == ROW_SEL_EXACT || prebuilt->n_rows_fetched >= MYSQL_FETCH_CACHE_THRESHOLD) && prebuilt->select_lock_type == LOCK_NONE - && !prebuilt->m_no_prefetch && !prebuilt->templ_contains_blob && !prebuilt->clust_index_was_generated && !prebuilt->used_in_HANDLER From 545cba13eb4e013363a126754c040c335874c386 Mon Sep 17 00:00:00 2001 From: Vladislav Vaintroub Date: Thu, 4 Mar 2021 23:09:22 +0100 Subject: [PATCH 3/3] MDEV-22929 fixup. Print "completed OK!" if page corruption and --log-innodb-page-corruption Since we do not stop at corrupted page error, there is no reason to log a backup error. --- extra/mariabackup/xtrabackup.cc | 7 +++---- .../mariabackup/log_page_corruption.result | 7 ++++--- .../suite/mariabackup/log_page_corruption.test | 18 +++++++++++------- 3 files changed, 18 insertions(+), 14 deletions(-) diff --git a/extra/mariabackup/xtrabackup.cc b/extra/mariabackup/xtrabackup.cc index 53301fd8e75..ffb09c73edb 100644 --- a/extra/mariabackup/xtrabackup.cc +++ b/extra/mariabackup/xtrabackup.cc @@ -4718,13 +4718,12 @@ fail_before_log_copying_thread_start: log_file_op = NULL; pthread_mutex_destroy(&backup_mutex); pthread_cond_destroy(&scanned_lsn_cond); - if (opt_log_innodb_page_corruption && !corrupted_pages.empty()) { + if (!corrupted_pages.empty()) { + ut_ad(opt_log_innodb_page_corruption); msg("Error: corrupted innodb pages are found and logged to " MB_CORRUPTED_PAGES_FILE " file"); - return false; } - else - return(true); + return(true); } diff --git a/mysql-test/suite/mariabackup/log_page_corruption.result b/mysql-test/suite/mariabackup/log_page_corruption.result index 13e373b2f70..01ad4422c6a 100644 --- a/mysql-test/suite/mariabackup/log_page_corruption.result +++ b/mysql-test/suite/mariabackup/log_page_corruption.result @@ -22,11 +22,12 @@ INSERT INTO t5_corrupted_to_rename VALUES (3), (4), (5), (6), (7), (8), (9); INSERT INTO t6_corrupted_to_drop VALUES (3), (4), (5), (6), (7), (8), (9); INSERT INTO t7_corrupted_to_alter VALUES (3), (4), (5), (6), (7), (8), (9); # Corrupt tables -# Backup must fail due to page corruption +# Backup must fail due to page corruption FOUND 1 /Database page corruption detected.*/ in backup.log # "innodb_corrupted_pages" file must not exist -# Backup must fail, but "innodb_corrupted_pages" file must be created due to --log-innodb-page-corruption option +# Backup must not fail, but "innodb_corrupted_pages" file must be created due to --log-innodb-page-corruption option FOUND 1 /Database page corruption detected.*/ in backup.log +FOUND 1 /completed OK!/ in backup.log --- "innodb_corrupted_pages" file content: --- test/t1_corrupted 6 8 9 @@ -42,7 +43,7 @@ test/t7_corrupted_to_alter INSERT INTO t1_inc_corrupted VALUES (3), (4), (5), (6), (7), (8), (9); INSERT INTO t2_inc_corrupted VALUES (3), (4), (5), (6), (7), (8), (9); INSERT INTO t3_inc VALUES (3), (4), (5), (6), (7), (8), (9); -# Backup must fail, but "innodb_corrupted_pages" file must be created due to --log-innodb-page-corruption option +# Backup must not fail, but "innodb_corrupted_pages" file must be created due to --log-innodb-page-corruption option --- "innodb_corrupted_pages" file content: --- test/t1_corrupted 6 8 9 diff --git a/mysql-test/suite/mariabackup/log_page_corruption.test b/mysql-test/suite/mariabackup/log_page_corruption.test index e9419687288..0151afb96b4 100644 --- a/mysql-test/suite/mariabackup/log_page_corruption.test +++ b/mysql-test/suite/mariabackup/log_page_corruption.test @@ -59,7 +59,7 @@ EOF --let corrupted_pages_file_filt = $MYSQLTEST_VARDIR/tmp/innodb_corrupted_pages_filt --let perl_result_file=$MYSQLTEST_VARDIR/tmp/perl_result ---echo # Backup must fail due to page corruption +--echo # Backup must fail due to page corruption --disable_result_log --error 1 exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --target-dir=$targetdir > $backuplog; @@ -80,15 +80,19 @@ exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --target-dir= --let after_copy_test_t7_corrupted_to_alter=ALTER TABLE test.t7_corrupted_to_alter ADD COLUMN (d INT) --let add_corrupted_page_for_test_t7_corrupted_to_alter=3 ---echo # Backup must fail, but "innodb_corrupted_pages" file must be created due to --log-innodb-page-corruption option +--echo # Backup must not fail, but "innodb_corrupted_pages" file must be created due to --log-innodb-page-corruption option --disable_result_log ---error 1 --exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --log-innodb-page-corruption --target-dir=$targetdir --dbug=+d,mariabackup_events,mariabackup_inject_code > $backuplog --enable_result_log --let SEARCH_PATTERN=Database page corruption detected.* --let SEARCH_FILE=$backuplog --source include/search_pattern_in_file.inc + +--let SEARCH_PATTERN=completed OK! +--let SEARCH_FILE=$backuplog +--source include/search_pattern_in_file.inc + --echo --- "innodb_corrupted_pages" file content: --- perl; do "$ENV{MTR_SUITE_DIR}/include/corrupt-page.pl"; @@ -145,9 +149,8 @@ EOF --let after_copy_test_t7_inc_corrupted_to_alter=ALTER TABLE test.t7_inc_corrupted_to_alter ADD COLUMN (d INT) --let add_corrupted_page_for_test_t7_inc_corrupted_to_alter=3 ---echo # Backup must fail, but "innodb_corrupted_pages" file must be created due to --log-innodb-page-corruption option +--echo # Backup must not fail, but "innodb_corrupted_pages" file must be created due to --log-innodb-page-corruption option --disable_result_log ---error 1 --exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --log-innodb-page-corruption --target-dir=$incdir --incremental-basedir=$targetdir --dbug=+d,mariabackup_events,mariabackup_inject_code > $backuplog --disable_result_log @@ -161,6 +164,9 @@ EOF --let SEARCH_PATTERN=Database page corruption detected.* --let SEARCH_FILE=$backuplog --source include/search_pattern_in_file.inc +--let SEARCH_PATTERN=completed OK! +--source include/search_pattern_in_file.inc + --let corrupted_pages_file = $incdir/innodb_corrupted_pages --echo --- "innodb_corrupted_pages" file content: --- perl; @@ -260,7 +266,6 @@ EOF --echo # Full backup with --log-innodb-page-corruption --disable_result_log ---error 1 --exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --log-innodb-page-corruption --target-dir=$targetdir --enable_result_log --let corrupted_pages_file = $targetdir/innodb_corrupted_pages @@ -288,7 +293,6 @@ EOF --echo # Incremental backup --log-innodb-page-corruption --disable_result_log ---error 1 --exec $XTRABACKUP --defaults-file=$MYSQLTEST_VARDIR/my.cnf --backup --log-innodb-page-corruption --target-dir=$incdir --incremental-basedir=$targetdir --dbug=+d,mariabackup_events,mariabackup_inject_code > $backuplog --disable_result_log --let corrupted_pages_file = $incdir/innodb_corrupted_pages