From e43791d4dc8feb2c02a08ce73c0bb0e2c320018c Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Wed, 2 Oct 2019 15:23:59 +0400 Subject: [PATCH 1/3] Cleanup EITS Moved EITS allocation inside read_statistics_for_tables_if_needed(). Removed redundant is_safe argument. --- sql/sql_base.cc | 26 --------------------- sql/sql_statistics.cc | 53 +++++++++++++++++++++++++++---------------- sql/sql_statistics.h | 2 -- 3 files changed, 34 insertions(+), 47 deletions(-) diff --git a/sql/sql_base.cc b/sql/sql_base.cc index e8bdff8b48f..1c4e7600d87 100644 --- a/sql/sql_base.cc +++ b/sql/sql_base.cc @@ -4245,32 +4245,6 @@ open_and_process_table(THD *thd, TABLE_LIST *tables, uint *counter, uint flags, goto end; } - if (get_use_stat_tables_mode(thd) > NEVER && tables->table) - { - TABLE_SHARE *table_share= tables->table->s; - if (table_share && table_share->table_category == TABLE_CATEGORY_USER && - table_share->tmp_table == NO_TMP_TABLE) - { - if (table_share->stats_cb.stats_can_be_read || - !alloc_statistics_for_table_share(thd, table_share, FALSE)) - { - if (table_share->stats_cb.stats_can_be_read) - { - KEY *key_info= table_share->key_info; - KEY *key_info_end= key_info + table_share->keys; - KEY *table_key_info= tables->table->key_info; - for ( ; key_info < key_info_end; key_info++, table_key_info++) - table_key_info->read_stats= key_info->read_stats; - Field **field_ptr= table_share->field; - Field **table_field_ptr= tables->table->field; - for ( ; *field_ptr; field_ptr++, table_field_ptr++) - (*table_field_ptr)->read_stats= (*field_ptr)->read_stats; - tables->table->stats_is_read= table_share->stats_cb.stats_is_read; - } - } - } - } - process_view_routines: /* Again we may need cache all routines used by this view and add diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index 52b3811e60d..e18cec589be 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -2216,8 +2216,6 @@ inline bool statistics_for_command_is_needed(THD *thd) thd Thread handler @param table_share Table share for which the memory for statistical data is allocated - @param - is_safe TRUE <-> at any time only one thread can perform the function @note The function allocates the memory for the statistical data on a table in the @@ -2226,8 +2224,6 @@ inline bool statistics_for_command_is_needed(THD *thd) mysql.index_stats. The memory is allocated for the statistics on the table, on the tables's columns, and on the table's indexes. The memory is allocated in the table_share's mem_root. - If the parameter is_safe is TRUE then it is guaranteed that at any given time - only one thread is executed the code of the function. @retval 0 If the memory for all statistical data has been successfully allocated @@ -2246,16 +2242,10 @@ inline bool statistics_for_command_is_needed(THD *thd) Here the second and the third threads try to allocate the memory for statistical data at the same time. The precautions are taken to guarantee the correctness of the allocation. - - @note - Currently the function always is called with the parameter is_safe set - to FALSE. */ -int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *table_share, - bool is_safe) +static int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *table_share) { - Field **field_ptr; KEY *key_info, *end; TABLE_STATISTICS_CB *stats_cb= &table_share->stats_cb; @@ -2268,13 +2258,11 @@ int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *table_share, if (!statistics_for_command_is_needed(thd)) DBUG_RETURN(1); - if (!is_safe) - mysql_mutex_lock(&table_share->LOCK_share); + mysql_mutex_lock(&table_share->LOCK_share); if (stats_cb->stats_can_be_read) { - if (!is_safe) - mysql_mutex_unlock(&table_share->LOCK_share); + mysql_mutex_unlock(&table_share->LOCK_share); DBUG_RETURN(0); } @@ -2285,8 +2273,7 @@ int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *table_share, sizeof(Table_statistics)); if (!table_stats) { - if (!is_safe) - mysql_mutex_unlock(&table_share->LOCK_share); + mysql_mutex_unlock(&table_share->LOCK_share); DBUG_RETURN(1); } memset(table_stats, 0, sizeof(Table_statistics)); @@ -2358,8 +2345,7 @@ int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *table_share, if (column_stats && index_stats && idx_avg_frequency) stats_cb->stats_can_be_read= TRUE; - if (!is_safe) - mysql_mutex_unlock(&table_share->LOCK_share); + mysql_mutex_unlock(&table_share->LOCK_share); DBUG_RETURN(0); } @@ -3264,6 +3250,35 @@ int read_statistics_for_tables_if_needed(THD *thd, TABLE_LIST *tables) DBUG_ENTER("read_statistics_for_tables_if_needed"); + for (TABLE_LIST *tl= tables; tl; tl= tl->next_global) + { + if (get_use_stat_tables_mode(thd) > NEVER && tl->table) + { + TABLE_SHARE *table_share= tl->table->s; + if (table_share && table_share->table_category == TABLE_CATEGORY_USER && + table_share->tmp_table == NO_TMP_TABLE) + { + if (table_share->stats_cb.stats_can_be_read || + !alloc_statistics_for_table_share(thd, table_share)) + { + if (table_share->stats_cb.stats_can_be_read) + { + KEY *key_info= table_share->key_info; + KEY *key_info_end= key_info + table_share->keys; + KEY *table_key_info= tl->table->key_info; + for ( ; key_info < key_info_end; key_info++, table_key_info++) + table_key_info->read_stats= key_info->read_stats; + Field **field_ptr= table_share->field; + Field **table_field_ptr= tl->table->field; + for ( ; *field_ptr; field_ptr++, table_field_ptr++) + (*table_field_ptr)->read_stats= (*field_ptr)->read_stats; + tl->table->stats_is_read= table_share->stats_cb.stats_is_read; + } + } + } + } + } + DEBUG_SYNC(thd, "statistics_read_start"); if (!statistics_for_tables_is_needed(thd, tables)) diff --git a/sql/sql_statistics.h b/sql/sql_statistics.h index 16325220e84..968f77cd2ca 100644 --- a/sql/sql_statistics.h +++ b/sql/sql_statistics.h @@ -90,8 +90,6 @@ Use_stat_tables_mode get_use_stat_tables_mode(THD *thd) int read_statistics_for_tables_if_needed(THD *thd, TABLE_LIST *tables); int collect_statistics_for_table(THD *thd, TABLE *table); -int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *share, - bool is_safe); void delete_stat_values_for_table_share(TABLE_SHARE *table_share); int alloc_statistics_for_table(THD *thd, TABLE *table); int update_statistics_for_table(THD *thd, TABLE *table); From adefaeffcce7c4ae0844f72dd920603b35285d40 Mon Sep 17 00:00:00 2001 From: Sergey Vojtovich Date: Wed, 2 Oct 2019 16:04:52 +0400 Subject: [PATCH 2/3] MDEV-19536 - Server crash or ASAN heap-use-after-free in is_temporary_table / read_statistics_for_tables_if_needed Regression after 279a907, read_statistics_for_tables_if_needed() was called after open_normal_and_derived_tables() failure. Fixed by moving read_statistics_for_tables() call to a branch of get_schema_stat_record() where result of open_normal_and_derived_tables() is checked. Removed THD::force_read_stats, added read_statistics_for_tables() instead. Simplified away statistics_for_command_is_needed(). --- sql/sql_class.cc | 1 - sql/sql_class.h | 3 -- sql/sql_show.cc | 5 +-- sql/sql_statistics.cc | 83 ++++++++++++++----------------------------- sql/sql_statistics.h | 1 + 5 files changed, 29 insertions(+), 64 deletions(-) diff --git a/sql/sql_class.cc b/sql/sql_class.cc index 1a3ca54abf8..ab105c67507 100644 --- a/sql/sql_class.cc +++ b/sql/sql_class.cc @@ -986,7 +986,6 @@ THD::THD(bool is_wsrep_applier) memset(&invoker_host, 0, sizeof(invoker_host)); prepare_derived_at_open= FALSE; create_tmp_table_for_derived= FALSE; - force_read_stats= FALSE; save_prep_leaf_list= FALSE; /* Restore THR_THD */ set_current_thd(old_THR_THD); diff --git a/sql/sql_class.h b/sql/sql_class.h index 8a4d8ff06a3..6c622648ac7 100644 --- a/sql/sql_class.h +++ b/sql/sql_class.h @@ -2200,9 +2200,6 @@ public: */ bool create_tmp_table_for_derived; - /* The flag to force reading statistics from EITS tables */ - bool force_read_stats; - bool save_prep_leaf_list; /* container for handler's private per-connection data */ diff --git a/sql/sql_show.cc b/sql/sql_show.cc index 71bfc644441..c154f5da472 100644 --- a/sql/sql_show.cc +++ b/sql/sql_show.cc @@ -4272,7 +4272,6 @@ fill_schema_table_by_open(THD *thd, bool is_show_fields_or_keys, SQLCOM_SHOW_FIELDS is used because it satisfies 'only_view_structure()'. */ - thd->force_read_stats= get_schema_table_idx(schema_table) == SCH_STATISTICS; lex->sql_command= SQLCOM_SHOW_FIELDS; result= (open_temporary_tables(thd, table_list) || open_normal_and_derived_tables(thd, table_list, @@ -4287,9 +4286,6 @@ fill_schema_table_by_open(THD *thd, bool is_show_fields_or_keys, */ lex->sql_command= old_lex->sql_command; - (void) read_statistics_for_tables_if_needed(thd, table_list); - thd->force_read_stats= false; - DEBUG_SYNC(thd, "after_open_table_ignore_flush"); /* @@ -6165,6 +6161,7 @@ static int get_schema_stat_record(THD *thd, TABLE_LIST *tables, KEY *key_info=show_table->s->key_info; if (show_table->file) { + (void) read_statistics_for_tables(thd, tables); show_table->file->info(HA_STATUS_VARIABLE | HA_STATUS_NO_LOCK | HA_STATUS_TIME); diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index e18cec589be..4a1ed9cde4e 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -2160,54 +2160,6 @@ int alloc_statistics_for_table(THD* thd, TABLE *table) } -/** - @brief - Check whether any persistent statistics for the processed command is needed - - @param - thd The thread handle - - @details - The function checks whether any persitent statistics for the processed - command is needed to be read. - - @retval - TRUE statistics is needed to be read - @retval - FALSE Otherwise -*/ - -static -inline bool statistics_for_command_is_needed(THD *thd) -{ - if (thd->bootstrap || thd->variables.use_stat_tables == NEVER) - return FALSE; - - if (thd->force_read_stats) - return TRUE; - - switch(thd->lex->sql_command) { - case SQLCOM_SELECT: - case SQLCOM_INSERT: - case SQLCOM_INSERT_SELECT: - case SQLCOM_UPDATE: - case SQLCOM_UPDATE_MULTI: - case SQLCOM_DELETE: - case SQLCOM_DELETE_MULTI: - case SQLCOM_REPLACE: - case SQLCOM_REPLACE_SELECT: - case SQLCOM_CREATE_TABLE: - case SQLCOM_SET_OPTION: - case SQLCOM_DO: - break; - default: - return FALSE; - } - - return TRUE; -} - - /** @brief Allocate memory for the statistical data used by a table share @@ -2255,9 +2207,6 @@ static int alloc_statistics_for_table_share(THD* thd, TABLE_SHARE *table_share) DEBUG_SYNC(thd, "statistics_mem_alloc_start1"); DEBUG_SYNC(thd, "statistics_mem_alloc_start2"); - if (!statistics_for_command_is_needed(thd)) - DBUG_RETURN(1); - mysql_mutex_lock(&table_share->LOCK_share); if (stats_cb->stats_can_be_read) @@ -3110,9 +3059,6 @@ bool statistics_for_tables_is_needed(THD *thd, TABLE_LIST *tables) { if (!tables) return FALSE; - - if (!statistics_for_command_is_needed(thd)) - return FALSE; /* Do not read statistics for any query that explicity involves @@ -3244,15 +3190,40 @@ int read_histograms_for_table(THD *thd, TABLE *table, TABLE_LIST *stat_tables) */ int read_statistics_for_tables_if_needed(THD *thd, TABLE_LIST *tables) +{ + switch (thd->lex->sql_command) { + case SQLCOM_SELECT: + case SQLCOM_INSERT: + case SQLCOM_INSERT_SELECT: + case SQLCOM_UPDATE: + case SQLCOM_UPDATE_MULTI: + case SQLCOM_DELETE: + case SQLCOM_DELETE_MULTI: + case SQLCOM_REPLACE: + case SQLCOM_REPLACE_SELECT: + case SQLCOM_CREATE_TABLE: + case SQLCOM_SET_OPTION: + case SQLCOM_DO: + return read_statistics_for_tables(thd, tables); + default: + return 0; + } +} + + +int read_statistics_for_tables(THD *thd, TABLE_LIST *tables) { TABLE_LIST stat_tables[STATISTICS_TABLES]; Open_tables_backup open_tables_backup; - DBUG_ENTER("read_statistics_for_tables_if_needed"); + DBUG_ENTER("read_statistics_for_tables"); + + if (thd->bootstrap || thd->variables.use_stat_tables == NEVER) + DBUG_RETURN(0); for (TABLE_LIST *tl= tables; tl; tl= tl->next_global) { - if (get_use_stat_tables_mode(thd) > NEVER && tl->table) + if (tl->table) { TABLE_SHARE *table_share= tl->table->s; if (table_share && table_share->table_category == TABLE_CATEGORY_USER && diff --git a/sql/sql_statistics.h b/sql/sql_statistics.h index 968f77cd2ca..71d727eab07 100644 --- a/sql/sql_statistics.h +++ b/sql/sql_statistics.h @@ -89,6 +89,7 @@ Use_stat_tables_mode get_use_stat_tables_mode(THD *thd) } int read_statistics_for_tables_if_needed(THD *thd, TABLE_LIST *tables); +int read_statistics_for_tables(THD *thd, TABLE_LIST *tables); int collect_statistics_for_table(THD *thd, TABLE *table); void delete_stat_values_for_table_share(TABLE_SHARE *table_share); int alloc_statistics_for_table(THD *thd, TABLE *table); From db9a4d928dc8e81ce449b54ef0bf02c248d931d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marko=20M=C3=A4kel=C3=A4?= Date: Mon, 7 Oct 2019 17:18:10 +0300 Subject: [PATCH 3/3] Remove orphan declaration buf_flush_wait_batch_end_wait_only() The function was declared but not defined in commit 9d6d1902e091c868bb288e0ccf9f975ccb474db9 --- storage/innobase/include/buf0flu.h | 11 ----------- storage/xtradb/include/buf0flu.h | 11 ----------- 2 files changed, 22 deletions(-) diff --git a/storage/innobase/include/buf0flu.h b/storage/innobase/include/buf0flu.h index b5cfcb125e6..4cc26be3918 100644 --- a/storage/innobase/include/buf0flu.h +++ b/storage/innobase/include/buf0flu.h @@ -141,17 +141,6 @@ buf_flush_wait_batch_end( buf_pool_t* buf_pool, /*!< in: buffer pool instance */ enum buf_flush type); /*!< in: BUF_FLUSH_LRU or BUF_FLUSH_LIST */ -/******************************************************************//** -Waits until a flush batch of the given type ends. This is called by -a thread that only wants to wait for a flush to end but doesn't do -any flushing itself. */ -UNIV_INTERN -void -buf_flush_wait_batch_end_wait_only( -/*===============================*/ - buf_pool_t* buf_pool, /*!< in: buffer pool instance */ - enum buf_flush type); /*!< in: BUF_FLUSH_LRU - or BUF_FLUSH_LIST */ /********************************************************************//** This function should be called at a mini-transaction commit, if a page was modified in it. Puts the block to the list of modified blocks, if it not diff --git a/storage/xtradb/include/buf0flu.h b/storage/xtradb/include/buf0flu.h index ed01b627bf4..83d4dae7f1c 100644 --- a/storage/xtradb/include/buf0flu.h +++ b/storage/xtradb/include/buf0flu.h @@ -143,17 +143,6 @@ buf_flush_wait_batch_end( buf_pool_t* buf_pool, /*!< in: buffer pool instance */ enum buf_flush type); /*!< in: BUF_FLUSH_LRU or BUF_FLUSH_LIST */ -/******************************************************************//** -Waits until a flush batch of the given type ends. This is called by -a thread that only wants to wait for a flush to end but doesn't do -any flushing itself. */ -UNIV_INTERN -void -buf_flush_wait_batch_end_wait_only( -/*===============================*/ - buf_pool_t* buf_pool, /*!< in: buffer pool instance */ - enum buf_flush type); /*!< in: BUF_FLUSH_LRU - or BUF_FLUSH_LIST */ /********************************************************************//** This function should be called at a mini-transaction commit, if a page was modified in it. Puts the block to the list of modified blocks, if it not