From 7d1cccae44dbf21bc7d935552fd5ec0968752833 Mon Sep 17 00:00:00 2001 From: Mattias Jonsson Date: Thu, 15 Sep 2011 20:49:39 +0200 Subject: [PATCH] Bug#12696518: MEMORY LEAKS IN HA_PARTITION (VALGRIND TESTS ON TRUNK) (also 5.5+ solution for bug#11766879/bug#60106) The valgrind warning was due to an unused 'new handler_add_index(...)' which was never freed. The error handling did not work (fails as in bug#11766879) and the implementation was not as transparant as it could, therefore I made it a bit simpler and more transparant to the underlying handlers. This way it follows the api better and the error handling works and is also now tested. Also added a debug test to verify the error handling. Improved according to Jon Olavs review: Added class ha_partition_add_index. Also added base class Sql_alloc to handler_add_index. Update 3. --- mysql-test/r/partition_innodb_plugin.result | 34 +++ .../parts/r/partition_debug_innodb.result | 71 ++++++ .../suite/parts/t/partition_debug_innodb.test | 35 +++ mysql-test/t/partition_innodb_plugin.test | 27 +++ sql/ha_partition.cc | 206 ++++++++++++++---- sql/handler.h | 6 +- 6 files changed, 331 insertions(+), 48 deletions(-) diff --git a/mysql-test/r/partition_innodb_plugin.result b/mysql-test/r/partition_innodb_plugin.result index aa0944e96b5..f30dbd20119 100644 --- a/mysql-test/r/partition_innodb_plugin.result +++ b/mysql-test/r/partition_innodb_plugin.result @@ -1,3 +1,37 @@ +# +# Bug#11766879/Bug#60106: DIFF BETWEEN # OF INDEXES IN MYSQL VS INNODB, +# PARTITONING, ON INDEX CREATE +# Bug#12696518: MEMORY LEAKS IN HA_PARTITION (VALGRIND TESTS ON TRUNK) +# +CREATE TABLE t1 ( +id bigint NOT NULL AUTO_INCREMENT, +time date, +id2 bigint not null, +PRIMARY KEY (id,time) +) ENGINE=InnoDB DEFAULT CHARSET=utf8 +/*!50100 PARTITION BY RANGE(TO_DAYS(time)) +(PARTITION p10 VALUES LESS THAN (734708) ENGINE = InnoDB, +PARTITION p20 VALUES LESS THAN MAXVALUE ENGINE = InnoDB) */; +INSERT INTO t1 (time,id2) VALUES ('2011-07-24',1); +INSERT INTO t1 (time,id2) VALUES ('2011-07-25',1); +INSERT INTO t1 (time,id2) VALUES ('2011-07-25',1); +CREATE UNIQUE INDEX uk_time_id2 on t1(time,id2); +ERROR 23000: Duplicate entry '2011-07-25-1' for key 'uk_time_id2' +SELECT COUNT(*) FROM t1; +COUNT(*) +3 +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `id` bigint(20) NOT NULL AUTO_INCREMENT, + `time` date NOT NULL DEFAULT '0000-00-00', + `id2` bigint(20) NOT NULL, + PRIMARY KEY (`id`,`time`) +) ENGINE=InnoDB AUTO_INCREMENT=4 DEFAULT CHARSET=utf8 +/*!50100 PARTITION BY RANGE (TO_DAYS(time)) +(PARTITION p10 VALUES LESS THAN (734708) ENGINE = InnoDB, + PARTITION p20 VALUES LESS THAN MAXVALUE ENGINE = InnoDB) */ +DROP TABLE t1; call mtr.add_suppression("nnoDB: Error: table `test`.`t1` .* Partition.* InnoDB internal"); # # Bug#55091: Server crashes on ADD PARTITION after a failed attempt diff --git a/mysql-test/suite/parts/r/partition_debug_innodb.result b/mysql-test/suite/parts/r/partition_debug_innodb.result index 49d863bca8e..bae4faf434e 100644 --- a/mysql-test/suite/parts/r/partition_debug_innodb.result +++ b/mysql-test/suite/parts/r/partition_debug_innodb.result @@ -1,4 +1,75 @@ DROP TABLE IF EXISTS t1; +# +# Bug#12696518/Bug#11766879/60106:DIFF BETWEEN # OF INDEXES IN MYSQL +# VS INNODB, PARTITONING, ON INDEX CREATE +# +CREATE TABLE t1 +(a INT PRIMARY KEY, +b VARCHAR(64)) +ENGINE = InnoDB +PARTITION BY HASH (a) PARTITIONS 3; +INSERT INTO t1 VALUES (0, 'first row'), (1, 'second row'), (2, 'Third row'); +INSERT INTO t1 VALUES (3, 'row id 3'), (4, '4 row'), (5, 'row5'); +INSERT INTO t1 VALUES (6, 'X 6 row'), (7, 'Seventh row'), (8, 'Last row'); +ALTER TABLE t1 ADD INDEX new_b_index (b); +ALTER TABLE t1 DROP INDEX new_b_index; +SET SESSION debug= "+d,ha_partition_fail_final_add_index"; +ALTER TABLE t1 ADD INDEX (b); +ERROR HY000: Table has no partition for value 0 +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) NOT NULL, + `b` varchar(64) DEFAULT NULL, + PRIMARY KEY (`a`) +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +/*!50100 PARTITION BY HASH (a) +PARTITIONS 3 */ +SELECT * FROM t1; +a b +0 first row +1 second row +2 Third row +3 row id 3 +4 4 row +5 row5 +6 X 6 row +7 Seventh row +8 Last row +FLUSH TABLES; +CREATE INDEX new_index ON t1 (b); +ERROR HY000: Table has no partition for value 0 +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) NOT NULL, + `b` varchar(64) DEFAULT NULL, + PRIMARY KEY (`a`) +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +/*!50100 PARTITION BY HASH (a) +PARTITIONS 3 */ +SELECT * FROM t1; +a b +0 first row +1 second row +2 Third row +3 row id 3 +4 4 row +5 row5 +6 X 6 row +7 Seventh row +8 Last row +SET SESSION debug= "-d,ha_partition_fail_final_add_index"; +SHOW CREATE TABLE t1; +Table Create Table +t1 CREATE TABLE `t1` ( + `a` int(11) NOT NULL, + `b` varchar(64) DEFAULT NULL, + PRIMARY KEY (`a`) +) ENGINE=InnoDB DEFAULT CHARSET=latin1 +/*!50100 PARTITION BY HASH (a) +PARTITIONS 3 */ +DROP TABLE t1; call mtr.add_suppression("InnoDB: Warning: allocated tablespace .*, old maximum was"); call mtr.add_suppression("InnoDB: Error: table .* does not exist in the InnoDB internal"); call mtr.add_suppression("InnoDB: Warning: MySQL is trying to drop table "); diff --git a/mysql-test/suite/parts/t/partition_debug_innodb.test b/mysql-test/suite/parts/t/partition_debug_innodb.test index c5d8df33213..b546f6d73f8 100644 --- a/mysql-test/suite/parts/t/partition_debug_innodb.test +++ b/mysql-test/suite/parts/t/partition_debug_innodb.test @@ -12,6 +12,41 @@ DROP TABLE IF EXISTS t1; --let $DATADIR= `SELECT @@datadir;` +--echo # +--echo # Bug#12696518/Bug#11766879/60106:DIFF BETWEEN # OF INDEXES IN MYSQL +--echo # VS INNODB, PARTITONING, ON INDEX CREATE +--echo # +CREATE TABLE t1 +(a INT PRIMARY KEY, + b VARCHAR(64)) +ENGINE = InnoDB +PARTITION BY HASH (a) PARTITIONS 3; +INSERT INTO t1 VALUES (0, 'first row'), (1, 'second row'), (2, 'Third row'); +INSERT INTO t1 VALUES (3, 'row id 3'), (4, '4 row'), (5, 'row5'); +INSERT INTO t1 VALUES (6, 'X 6 row'), (7, 'Seventh row'), (8, 'Last row'); + +ALTER TABLE t1 ADD INDEX new_b_index (b); +ALTER TABLE t1 DROP INDEX new_b_index; + +SET SESSION debug= "+d,ha_partition_fail_final_add_index"; + +--error ER_NO_PARTITION_FOR_GIVEN_VALUE +ALTER TABLE t1 ADD INDEX (b); +SHOW CREATE TABLE t1; +--sorted_result +SELECT * FROM t1; + +FLUSH TABLES; +--error ER_NO_PARTITION_FOR_GIVEN_VALUE +CREATE INDEX new_index ON t1 (b); +SHOW CREATE TABLE t1; +--sorted_result +SELECT * FROM t1; + +SET SESSION debug= "-d,ha_partition_fail_final_add_index"; +SHOW CREATE TABLE t1; +DROP TABLE t1; + # Checking with #innodb what this is... call mtr.add_suppression("InnoDB: Warning: allocated tablespace .*, old maximum was"); # If there is a crash or failure between the ddl_log is written and the diff --git a/mysql-test/t/partition_innodb_plugin.test b/mysql-test/t/partition_innodb_plugin.test index e8b73687177..ee428e0662f 100644 --- a/mysql-test/t/partition_innodb_plugin.test +++ b/mysql-test/t/partition_innodb_plugin.test @@ -3,6 +3,33 @@ let $MYSQLD_DATADIR= `SELECT @@datadir`; +--echo # +--echo # Bug#11766879/Bug#60106: DIFF BETWEEN # OF INDEXES IN MYSQL VS INNODB, +--echo # PARTITONING, ON INDEX CREATE +--echo # Bug#12696518: MEMORY LEAKS IN HA_PARTITION (VALGRIND TESTS ON TRUNK) +--echo # +CREATE TABLE t1 ( + id bigint NOT NULL AUTO_INCREMENT, + time date, + id2 bigint not null, + PRIMARY KEY (id,time) +) ENGINE=InnoDB DEFAULT CHARSET=utf8 +/*!50100 PARTITION BY RANGE(TO_DAYS(time)) +(PARTITION p10 VALUES LESS THAN (734708) ENGINE = InnoDB, + PARTITION p20 VALUES LESS THAN MAXVALUE ENGINE = InnoDB) */; + +INSERT INTO t1 (time,id2) VALUES ('2011-07-24',1); +INSERT INTO t1 (time,id2) VALUES ('2011-07-25',1); +INSERT INTO t1 (time,id2) VALUES ('2011-07-25',1); + +--error ER_DUP_ENTRY +CREATE UNIQUE INDEX uk_time_id2 on t1(time,id2); + +SELECT COUNT(*) FROM t1; +SHOW CREATE TABLE t1; + +DROP TABLE t1; + call mtr.add_suppression("nnoDB: Error: table `test`.`t1` .* Partition.* InnoDB internal"); --echo # --echo # Bug#55091: Server crashes on ADD PARTITION after a failed attempt diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc index 9e22553ef0e..7afe148dd17 100644 --- a/sql/ha_partition.cc +++ b/sql/ha_partition.cc @@ -6663,49 +6663,81 @@ bool ha_partition::check_if_incompatible_data(HA_CREATE_INFO *create_info, /** - Support of in-place add/drop index + Helper class for [final_]add_index, see handler.h */ + +class ha_partition_add_index : public handler_add_index +{ +public: + handler_add_index **add_array; + ha_partition_add_index(TABLE* table_arg, KEY* key_info_arg, + uint num_of_keys_arg) + : handler_add_index(table_arg, key_info_arg, num_of_keys_arg) + {} + ~ha_partition_add_index() {} +}; + + +/** + Support of in-place add/drop index + + @param table_arg Table to add index to + @param key_info Struct over the new keys to add + @param num_of_keys Number of keys to add + @param[out] add Data to be submitted with final_add_index + + @return Operation status + @retval 0 Success + @retval != 0 Failure (error code returned, and all operations rollbacked) +*/ + int ha_partition::add_index(TABLE *table_arg, KEY *key_info, uint num_of_keys, handler_add_index **add) { - handler **file; + uint i; int ret= 0; + THD *thd= ha_thd(); + ha_partition_add_index *part_add_index; DBUG_ENTER("ha_partition::add_index"); - *add= new handler_add_index(table, key_info, num_of_keys); /* There has already been a check in fix_partition_func in mysql_alter_table before this call, which checks for unique/primary key violations of the partitioning function. So no need for extra check here. */ - for (file= m_file; *file; file++) + + /* + This will be freed at the end of the statement. + And destroyed at final_add_index. (Sql_alloc does not free in delete). + */ + part_add_index= new (thd->mem_root) + ha_partition_add_index(table_arg, key_info, num_of_keys); + if (!part_add_index) + DBUG_RETURN(HA_ERR_OUT_OF_MEM); + part_add_index->add_array= (handler_add_index **) + thd->alloc(sizeof(void *) * m_tot_parts); + if (!part_add_index->add_array) { - handler_add_index *add_index; - if ((ret= (*file)->add_index(table_arg, key_info, num_of_keys, &add_index))) - goto err; - if ((ret= (*file)->final_add_index(add_index, true))) + delete part_add_index; + DBUG_RETURN(HA_ERR_OUT_OF_MEM); + } + + for (i= 0; i < m_tot_parts; i++) + { + if ((ret= m_file[i]->add_index(table_arg, key_info, num_of_keys, + &part_add_index->add_array[i]))) goto err; } + *add= part_add_index; DBUG_RETURN(ret); err: - if (file > m_file) + /* Rollback all prepared partitions. i - 1 .. 0 */ + while (i) { - uint *key_numbers= (uint*) ha_thd()->alloc(sizeof(uint) * num_of_keys); - uint old_num_of_keys= table_arg->s->keys; - uint i; - /* The newly created keys have the last id's */ - for (i= 0; i < num_of_keys; i++) - key_numbers[i]= i + old_num_of_keys; - if (!table_arg->key_info) - table_arg->key_info= key_info; - while (--file >= m_file) - { - (void) (*file)->prepare_drop_index(table_arg, key_numbers, num_of_keys); - (void) (*file)->final_drop_index(table_arg); - } - if (table_arg->key_info == key_info) - table_arg->key_info= NULL; + i--; + (void) m_file[i]->final_add_index(part_add_index->add_array[i], false); } + delete part_add_index; DBUG_RETURN(ret); } @@ -6713,37 +6745,119 @@ err: /** Second phase of in-place add index. + @param add Info from add_index + @param commit Should we commit or rollback the add_index operation + + @return Operation status + @retval 0 Success + @retval != 0 Failure (error code returned) + @note If commit is false, index changes are rolled back by dropping the added indexes. If commit is true, nothing is done as the indexes were already made active in ::add_index() - */ +*/ int ha_partition::final_add_index(handler_add_index *add, bool commit) { + ha_partition_add_index *part_add_index; + uint i; + int ret= 0; + DBUG_ENTER("ha_partition::final_add_index"); - // Rollback by dropping indexes. - if (!commit) + + if (!add) { - TABLE *table_arg= add->table; - uint num_of_keys= add->num_of_keys; - handler **file; - uint *key_numbers= (uint*) ha_thd()->alloc(sizeof(uint) * num_of_keys); - uint old_num_of_keys= table_arg->s->keys; - uint i; - /* The newly created keys have the last id's */ - for (i= 0; i < num_of_keys; i++) - key_numbers[i]= i + old_num_of_keys; - if (!table_arg->key_info) - table_arg->key_info= add->key_info; - for (file= m_file; *file; file++) - { - (void) (*file)->prepare_drop_index(table_arg, key_numbers, num_of_keys); - (void) (*file)->final_drop_index(table_arg); - } - if (table_arg->key_info == add->key_info) - table_arg->key_info= NULL; + DBUG_ASSERT(!commit); + DBUG_RETURN(0); } - DBUG_RETURN(0); + part_add_index= static_cast(add); + + for (i= 0; i < m_tot_parts; i++) + { + if ((ret= m_file[i]->final_add_index(part_add_index->add_array[i], commit))) + goto err; + DBUG_EXECUTE_IF("ha_partition_fail_final_add_index", { + /* Simulate a failure by rollback the second partition */ + if (m_tot_parts > 1) + { + i++; + m_file[i]->final_add_index(part_add_index->add_array[i], false); + /* Set an error that is specific to ha_partition. */ + ret= HA_ERR_NO_PARTITION_FOUND; + goto err; + } + }); + } + delete part_add_index; + DBUG_RETURN(ret); +err: + uint j; + uint *key_numbers= NULL; + KEY *old_key_info= NULL; + uint num_of_keys= 0; + int error; + + /* How could this happen? Needed to create a covering test case :) */ + DBUG_ASSERT(ret == HA_ERR_NO_PARTITION_FOUND); + + if (i > 0) + { + num_of_keys= part_add_index->num_of_keys; + key_numbers= (uint*) ha_thd()->alloc(sizeof(uint) * num_of_keys); + if (!key_numbers) + { + sql_print_error("Failed with error handling of adding index:\n" + "committing index failed, and when trying to revert " + "already committed partitions we failed allocating\n" + "memory for the index for table '%s'", + table_share->table_name.str); + DBUG_RETURN(HA_ERR_OUT_OF_MEM); + } + old_key_info= table->key_info; + /* + Use the newly added key_info as table->key_info to remove them. + Note that this requires the subhandlers to use name lookup of the + index. They must use given table->key_info[key_number], they cannot + use their local view of the keys, since table->key_info only include + the indexes to be removed here. + */ + for (j= 0; j < num_of_keys; j++) + key_numbers[j]= j; + table->key_info= part_add_index->key_info; + } + + for (j= 0; j < m_tot_parts; j++) + { + if (j < i) + { + /* Remove the newly added index */ + error= m_file[j]->prepare_drop_index(table, key_numbers, num_of_keys); + if (error || m_file[j]->final_drop_index(table)) + { + sql_print_error("Failed with error handling of adding index:\n" + "committing index failed, and when trying to revert " + "already committed partitions we failed removing\n" + "the index for table '%s' partition nr %d", + table_share->table_name.str, j); + } + } + else if (j > i) + { + /* Rollback non finished partitions */ + if (m_file[j]->final_add_index(part_add_index->add_array[j], false)) + { + /* How could this happen? */ + sql_print_error("Failed with error handling of adding index:\n" + "Rollback of add_index failed for table\n" + "'%s' partition nr %d", + table_share->table_name.str, j); + } + } + } + if (i > 0) + table->key_info= old_key_info; + delete part_add_index; + DBUG_RETURN(ret); } int ha_partition::prepare_drop_index(TABLE *table_arg, uint *key_num, diff --git a/sql/handler.h b/sql/handler.h index d1222cb56a6..cbdfc231ed5 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -1163,10 +1163,12 @@ uint calculate_key_len(TABLE *, uint, const uchar *, key_part_map); /** Index creation context. - Created by handler::add_index() and freed by handler::final_add_index(). + Created by handler::add_index() and destroyed by handler::final_add_index(). + And finally freed at the end of the statement. + (Sql_alloc does not free in delete). */ -class handler_add_index +class handler_add_index : public Sql_alloc { public: /* Table where the indexes are added */