From 83f7151da5259fa07ab2c26600009d53adef50f3 Mon Sep 17 00:00:00 2001 From: Alexey Botchkov Date: Fri, 9 Dec 2016 17:13:43 +0400 Subject: [PATCH] MDEV-10435 crash with bad stat tables. Functions from sql/statistics.cc don't seem to expect stat tables to fail or to have inadequate structure. Table open errors suppressed and some validity checks added. Invalid tables reported to the server log. --- mysql-test/r/statistics.result | 20 +++++ mysql-test/t/statistics.test | 24 +++++ sql/event_db_repository.cc | 14 +-- sql/rpl_gtid.cc | 14 +-- sql/sql_statistics.cc | 157 ++++++++++++++++++++++++++++++--- sql/sql_statistics.h | 9 +- sql/table.cc | 9 ++ sql/table.h | 10 +++ 8 files changed, 216 insertions(+), 41 deletions(-) diff --git a/mysql-test/r/statistics.result b/mysql-test/r/statistics.result index be2e39665b352..2d0f18f1ed76c 100644 --- a/mysql-test/r/statistics.result +++ b/mysql-test/r/statistics.result @@ -1676,4 +1676,24 @@ analyze table t1; Table Op Msg_type Msg_text test.t1 analyze status Table is already up to date drop table t1; +# +# MDEV-10435 crash with bad stat tables +# +set use_stat_tables='preferably'; +call mtr.add_suppression("Column count of mysql.table_stats is wrong. Expected 3, found 1. The table is probably corrupted"); +rename table mysql.table_stats to test.table_stats; +flush tables; +create table t1 (a int); +rename table t1 to t2, t3 to t4; +ERROR 42S02: Table 'test.t3' doesn't exist +drop table t1; +rename table test.table_stats to mysql.table_stats; +rename table mysql.table_stats to test.table_stats; +create table mysql.table_stats (a int); +flush tables; +create table t1 (a int); +rename table t1 to t2, t3 to t4; +ERROR 42S02: Table 'test.t3' doesn't exist +drop table t1, mysql.table_stats; +rename table test.table_stats to mysql.table_stats; set use_stat_tables=@save_use_stat_tables; diff --git a/mysql-test/t/statistics.test b/mysql-test/t/statistics.test index 805c169b2a423..3f08e2e133cb1 100644 --- a/mysql-test/t/statistics.test +++ b/mysql-test/t/statistics.test @@ -740,4 +740,28 @@ show variables like 'use_stat_tables'; analyze table t1; drop table t1; +--echo # +--echo # MDEV-10435 crash with bad stat tables +--echo # + +set use_stat_tables='preferably'; +call mtr.add_suppression("Column count of mysql.table_stats is wrong. Expected 3, found 1. The table is probably corrupted"); + +rename table mysql.table_stats to test.table_stats; +flush tables; +create table t1 (a int); +--error ER_NO_SUCH_TABLE +rename table t1 to t2, t3 to t4; +drop table t1; +rename table test.table_stats to mysql.table_stats; + +rename table mysql.table_stats to test.table_stats; +create table mysql.table_stats (a int); +flush tables; +create table t1 (a int); +--error ER_NO_SUCH_TABLE +rename table t1 to t2, t3 to t4; +drop table t1, mysql.table_stats; +rename table test.table_stats to mysql.table_stats; + set use_stat_tables=@save_use_stat_tables; diff --git a/sql/event_db_repository.cc b/sql/event_db_repository.cc index 666f6e3e24cab..ba782a19f8c84 100644 --- a/sql/event_db_repository.cc +++ b/sql/event_db_repository.cc @@ -166,20 +166,8 @@ const TABLE_FIELD_TYPE event_table_fields[ET_FIELD_COUNT] = static const TABLE_FIELD_DEF event_table_def= {ET_FIELD_COUNT, event_table_fields, 0, (uint*) 0}; -class Event_db_intact : public Table_check_intact -{ -protected: - void report_error(uint, const char *fmt, ...) - { - va_list args; - va_start(args, fmt); - error_log_print(ERROR_LEVEL, fmt, args); - va_end(args); - } -}; - /** In case of an error, a message is printed to the error log. */ -static Event_db_intact table_intact; +static Table_check_intact_log_error table_intact; /** diff --git a/sql/rpl_gtid.cc b/sql/rpl_gtid.cc index f54ef2b0081a2..dfec97bf021c7 100644 --- a/sql/rpl_gtid.cc +++ b/sql/rpl_gtid.cc @@ -448,19 +448,7 @@ static const TABLE_FIELD_DEF mysql_gtid_slave_pos_tabledef= { mysql_rpl_slave_state_pk_parts }; -class Gtid_db_intact : public Table_check_intact -{ -protected: - void report_error(uint, const char *fmt, ...) - { - va_list args; - va_start(args, fmt); - error_log_print(ERROR_LEVEL, fmt, args); - va_end(args); - } -}; - -static Gtid_db_intact gtid_table_intact; +static Table_check_intact_log_error gtid_table_intact; /* Check that the mysql.gtid_slave_pos table has the correct definition. diff --git a/sql/sql_statistics.cc b/sql/sql_statistics.cc index 27bc0fb4cd36b..62d56a700b805 100644 --- a/sql/sql_statistics.cc +++ b/sql/sql_statistics.cc @@ -129,6 +129,128 @@ inline void init_table_list_for_single_stat_table(TABLE_LIST *tbl, } +static Table_check_intact_log_error stat_table_intact; + +static const +TABLE_FIELD_TYPE table_stat_fields[TABLE_STAT_N_FIELDS] = +{ + { + { C_STRING_WITH_LEN("db_name") }, + { C_STRING_WITH_LEN("varchar(64)") }, + { C_STRING_WITH_LEN("utf8") } + }, + { + { C_STRING_WITH_LEN("table_name") }, + { C_STRING_WITH_LEN("varchar(64)") }, + { C_STRING_WITH_LEN("utf8") } + }, + { + { C_STRING_WITH_LEN("cardinality") }, + { C_STRING_WITH_LEN("bigint(21)") }, + { NULL, 0 } + }, +}; +static const uint table_stat_pk_col[]= {0,1}; +static const TABLE_FIELD_DEF +table_stat_def= {TABLE_STAT_N_FIELDS, table_stat_fields, 2, table_stat_pk_col }; + +static const +TABLE_FIELD_TYPE column_stat_fields[COLUMN_STAT_N_FIELDS] = +{ + { + { C_STRING_WITH_LEN("db_name") }, + { C_STRING_WITH_LEN("varchar(64)") }, + { C_STRING_WITH_LEN("utf8") } + }, + { + { C_STRING_WITH_LEN("table_name") }, + { C_STRING_WITH_LEN("varchar(64)") }, + { C_STRING_WITH_LEN("utf8") } + }, + { + { C_STRING_WITH_LEN("column_name") }, + { C_STRING_WITH_LEN("varchar(64)") }, + { C_STRING_WITH_LEN("utf8") } + }, + { + { C_STRING_WITH_LEN("min_value") }, + { C_STRING_WITH_LEN("varbinary(255)") }, + { NULL, 0 } + }, + { + { C_STRING_WITH_LEN("max_value") }, + { C_STRING_WITH_LEN("varbinary(255)") }, + { NULL, 0 } + }, + { + { C_STRING_WITH_LEN("nulls_ratio") }, + { C_STRING_WITH_LEN("decimal(12,4)") }, + { NULL, 0 } + }, + { + { C_STRING_WITH_LEN("avg_length") }, + { C_STRING_WITH_LEN("decimal(12,4)") }, + { NULL, 0 } + }, + { + { C_STRING_WITH_LEN("avg_frequency") }, + { C_STRING_WITH_LEN("decimal(12,4)") }, + { NULL, 0 } + }, + { + { C_STRING_WITH_LEN("hist_size") }, + { C_STRING_WITH_LEN("tinyint(3)") }, + { NULL, 0 } + }, + { + { C_STRING_WITH_LEN("hist_type") }, + { C_STRING_WITH_LEN("enum('SINGLE_PREC_HB','DOUBLE_PREC_HB')") }, + { C_STRING_WITH_LEN("utf8") } + }, + { + { C_STRING_WITH_LEN("histogram") }, + { C_STRING_WITH_LEN("varbinary(255)") }, + { NULL, 0 } + } +}; +static const uint column_stat_pk_col[]= {0,1,2}; +static const TABLE_FIELD_DEF +column_stat_def= {COLUMN_STAT_N_FIELDS, column_stat_fields, 3, column_stat_pk_col}; + +static const +TABLE_FIELD_TYPE index_stat_fields[INDEX_STAT_N_FIELDS] = +{ + { + { C_STRING_WITH_LEN("db_name") }, + { C_STRING_WITH_LEN("varchar(64)") }, + { C_STRING_WITH_LEN("utf8") } + }, + { + { C_STRING_WITH_LEN("table_name") }, + { C_STRING_WITH_LEN("varchar(64)") }, + { C_STRING_WITH_LEN("utf8") } + }, + { + { C_STRING_WITH_LEN("index") }, + { C_STRING_WITH_LEN("varchar(64)") }, + { C_STRING_WITH_LEN("utf8") } + }, + { + { C_STRING_WITH_LEN("prefix_arity") }, + { C_STRING_WITH_LEN("int(11)") }, + { NULL, 0 } + }, + { + { C_STRING_WITH_LEN("avg_frequency") }, + { C_STRING_WITH_LEN("decimal(12,4)") }, + { NULL, 0 } + } +}; +static const uint index_stat_pk_col[]= {0,1,2,3}; +static const TABLE_FIELD_DEF +index_stat_def= {INDEX_STAT_N_FIELDS, index_stat_fields, 4, index_stat_pk_col}; + + /** @brief Open all statistical tables and lock them @@ -139,10 +261,30 @@ inline int open_stat_tables(THD *thd, TABLE_LIST *tables, Open_tables_backup *backup, bool for_write) { + int rc; + + Dummy_error_handler deh; // suppress errors + thd->push_internal_handler(&deh); init_table_list_for_stat_tables(tables, for_write); init_mdl_requests(tables); - return open_system_tables_for_read(thd, tables, backup); -} + rc= open_system_tables_for_read(thd, tables, backup); + thd->pop_internal_handler(); + + + /* If the number of tables changes, we should revise the check below. */ + DBUG_ASSERT(STATISTICS_TABLES == 3); + + if (!rc && + (stat_table_intact.check(tables[TABLE_STAT].table, &table_stat_def) || + stat_table_intact.check(tables[COLUMN_STAT].table, &column_stat_def) || + stat_table_intact.check(tables[INDEX_STAT].table, &index_stat_def))) + { + close_system_tables(thd, backup); + rc= 1; + } + + return rc; +} /** @@ -2725,10 +2867,7 @@ int update_statistics_for_table(THD *thd, TABLE *table) DEBUG_SYNC(thd, "statistics_update_start"); if (open_stat_tables(thd, tables, &open_tables_backup, TRUE)) - { - thd->clear_error(); DBUG_RETURN(rc); - } save_binlog_format= thd->set_current_stmt_binlog_format_stmt(); @@ -3156,10 +3295,7 @@ int delete_statistics_for_table(THD *thd, LEX_STRING *db, LEX_STRING *tab) DBUG_ENTER("delete_statistics_for_table"); if (open_stat_tables(thd, tables, &open_tables_backup, TRUE)) - { - thd->clear_error(); DBUG_RETURN(rc); - } save_binlog_format= thd->set_current_stmt_binlog_format_stmt(); @@ -3398,10 +3534,7 @@ int rename_table_in_stat_tables(THD *thd, LEX_STRING *db, LEX_STRING *tab, DBUG_ENTER("rename_table_in_stat_tables"); if (open_stat_tables(thd, tables, &open_tables_backup, TRUE)) - { - thd->clear_error(); - DBUG_RETURN(rc); - } + DBUG_RETURN(0); // not an error save_binlog_format= thd->set_current_stmt_binlog_format_stmt(); diff --git a/sql/sql_statistics.h b/sql/sql_statistics.h index 20b2eb664492a..f46583839d1cf 100644 --- a/sql/sql_statistics.h +++ b/sql/sql_statistics.h @@ -52,7 +52,8 @@ enum enum_table_stat_col { TABLE_STAT_DB_NAME, TABLE_STAT_TABLE_NAME, - TABLE_STAT_CARDINALITY + TABLE_STAT_CARDINALITY, + TABLE_STAT_N_FIELDS }; enum enum_column_stat_col @@ -67,7 +68,8 @@ enum enum_column_stat_col COLUMN_STAT_AVG_FREQUENCY, COLUMN_STAT_HIST_SIZE, COLUMN_STAT_HIST_TYPE, - COLUMN_STAT_HISTOGRAM + COLUMN_STAT_HISTOGRAM, + COLUMN_STAT_N_FIELDS }; enum enum_index_stat_col @@ -76,7 +78,8 @@ enum enum_index_stat_col INDEX_STAT_TABLE_NAME, INDEX_STAT_INDEX_NAME, INDEX_STAT_PREFIX_ARITY, - INDEX_STAT_AVG_FREQUENCY + INDEX_STAT_AVG_FREQUENCY, + INDEX_STAT_N_FIELDS }; inline diff --git a/sql/table.cc b/sql/table.cc index 85ffd560992a4..686b3569a9702 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -3861,6 +3861,15 @@ Table_check_intact::check(TABLE *table, const TABLE_FIELD_DEF *table_def) } +void Table_check_intact_log_error::report_error(uint, const char *fmt, ...) +{ + va_list args; + va_start(args, fmt); + error_log_print(ERROR_LEVEL, fmt, args); + va_end(args); +} + + /** Traverse portion of wait-for graph which is reachable through edge represented by this flush ticket in search for deadlocks. diff --git a/sql/table.h b/sql/table.h index eb4076e02e2da..093c357e5750b 100644 --- a/sql/table.h +++ b/sql/table.h @@ -524,6 +524,16 @@ class Table_check_intact }; +/* + If the table isn't valid, report the error to the server log only. +*/ +class Table_check_intact_log_error : public Table_check_intact +{ +protected: + void report_error(uint, const char *fmt, ...); +}; + + /** Class representing the fact that some thread waits for table share to be flushed. Is used to represent information about