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

+#
+# 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

mysql-test/suite/parts/r/partition_debug_innodb.result

 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 ");

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

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

sql/ha_partition.cc

@@ -6662,50 +6662,82 @@ bool ha_partition::check_if_incompatible_data(HA_CREATE_INFO *create_info,
 }
 
 
+/**
+  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)
-  {
-    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++)
+ 
+  if (!add)
+  {
+    DBUG_ASSERT(!commit);
+    DBUG_RETURN(0);
+  }
+  part_add_index= static_cast<class ha_partition_add_index*>(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)
     {
-      (void) (*file)->prepare_drop_index(table_arg, key_numbers, num_of_keys);
-      (void) (*file)->final_drop_index(table_arg);
+      /* 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 (table_arg->key_info == add->key_info)
-      table_arg->key_info= NULL;
   }
-  DBUG_RETURN(0);
+  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,

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 */