Merge lp:~akopytov/percona-server/bugs-1039536-1081003-5.1 into lp:percona-server/5.1

[Alexey Kopytov]

Bug #1039536: mysqldump --innodb-optimize-keys can generate invalid
              table definitions
Bug #1081003: mysqldump --innodb-optimize-keys handles AUTO_INCREMENT
              columns inefficiently

The problem in bug #1039536 was that mysqldump --innodb-optimize-keys
did not handle composite indexes correctly when verifying if the
optimization is applicable with respect to AUTO_INCREMENT columns. When
an AUTO_INCREMENT column was encountered in the SHOW CREATE TABLE
output, the column was marked so that subsequent index specifications
involving that column would not be used in deferred index creation
mechanism, as MySQL does not allow creating tables with unindexed
AUTO_INCREMENT columns. However, the code checking if an index
specification involves a previously marked AUTO_INCREMENT column failed
to handle composite keys correctly, so those keys were optimized away
resulting in an invalid table definition.

A closely related problem in bug #1081003 was that even in cases where
indexes with AUTO_INCREMENT columns where correctly detected, mysqldump
prevented all such keys from optimization, even though it is sufficient
to skip just one (e.g. the first one).

Fixed by refactoring the AUTO_INCREMENT handling code in mysqldump
--innodb-optimize-keys to:

- process composite keys correctly

- prevent only the first key indexing an AUTO_INCREMENT columns from
  optimization

- use a simple pointer instead of the hash table to keep track of
  AUTO_INCREMENT column with a simple pointer, as the server only allows
  one such column per table anyway

[Alexey Kopytov (akopytov)]

http://jenkins.percona.com/view/PS%205.1/job/percona-server-5.1-param/464/

[Laurynas Biveinis (laurynas-biveinis)]

Looks good.

Were there any serious issues, I'd also suggest addressing two minor comments:
1) strncmp("foo", blah, sizeof("foo") - 1)) effectively does string prefix comparison, as sizeof - 1 removes the trailing NUL from comparison. Which is probably unintentional, but no real difference.
2) instead of a trinary operator at contains_autoinc_column call I'd add if (type == KEY_TYPE_NONE) return FALSE; to the function itself, seems more natural to me.

review: Approve

[Alexey Kopytov (akopytov)]

Laurynas,

The prefix comparison was intentional, so removing -1 from "sizeof(...) - 1" would break the check.

As to returning FALSE for KEY_TYPE_NONE in contains_autoinc_column() -- yes, it might be implemented like that, but I wanted to assert the function is never called with a wrong key type.
The general principle in the MySQL code is that callers are responsible for validating arguments to a function, not the callee.

[Laurynas Biveinis (laurynas-biveinis)]

Right, it must be the prefix comparison, sorry.

=== modified file 'Percona-Server/client/mysqldump.c'
--- Percona-Server/client/mysqldump.c	2012-08-20 03:14:02 +0000
+++ Percona-Server/client/mysqldump.c	2012-11-20 09:37:22 +0000
@@ -85,6 +85,13 @@
 #define IGNORE_DATA 0x01 /* don't dump data for this table */
 #define IGNORE_INSERT_DELAYED 0x02 /* table doesn't support INSERT DELAYED */
 
+typedef enum {
+  KEY_TYPE_NONE,
+  KEY_TYPE_PRIMARY,
+  KEY_TYPE_UNIQUE,
+  KEY_TYPE_NON_UNIQUE
+} key_type_t;
+
 /* general_log or slow_log tables under mysql database */
 static inline my_bool general_log_or_slow_log_tables(const char *db, 
                                                      const char *table)
@@ -2460,20 +2467,62 @@
 }
 
 /*
-  Parse the specified key definition string and check if the key indexes
-  any of the columns from ignored_columns.
+  Parse the specified key definition string and check if the key contains an
+  AUTO_INCREMENT column as the first key part. We only check for the first key
+  part, because unlike MyISAM, InnoDB does not allow the AUTO_INCREMENT column
+  as a secondary key column, i.e. the AUTO_INCREMENT column would not be
+  considered indexed for such key specification.
 */
-static my_bool contains_ignored_column(HASH *ignored_columns, char *keydef)
+static my_bool contains_autoinc_column(const char *autoinc_column,
+                                       const char *keydef,
+                                       key_type_t type)
 {
-  char *leftp, *rightp;
-
-  if ((leftp = strchr(keydef, '(')) &&
-      (rightp = strchr(leftp, ')')) &&
-      rightp > leftp + 3 &&                      /* (`...`) */
-      leftp[1] == '`' &&
-      rightp[-1] == '`' &&
-      hash_search(ignored_columns, (uchar *) leftp + 2, rightp - leftp - 3))
-    return TRUE;
+  char *from, *to;
+  uint idnum;
+
+  DBUG_ASSERT(type != KEY_TYPE_NONE);
+
+  if (autoinc_column == NULL || !(from= strchr(keydef, '`')))
+    return FALSE;
+
+  to= from;
+  idnum= 0;
+
+  while ((to= strchr(to + 1, '`')))
+  {
+    /*
+      Double backticks represent a backtick in identifier, rather than a quote
+      character.
+    */
+    if (to[1] == '`')
+    {
+      to++;
+      continue;
+    }
+
+    if (to <= from + 1)
+      break;                                    /* Broken key definition */
+
+    idnum++;
+
+    /*
+      Skip the check if it's the first identifier and we are processing a
+      secondary key.
+    */
+    if ((type == KEY_TYPE_PRIMARY || idnum != 1) &&
+        !strncmp(autoinc_column, from + 1, to - from - 1))
+      return TRUE;
+
+    /*
+      Check only the first (for PRIMARY KEY) or the second (for secondary keys)
+      quoted identifier.
+    */
+    if ((idnum == 1 + test(type != KEY_TYPE_PRIMARY)) ||
+        !(from= strchr(to + 1, '`')))
+      break;
+
+    to= from;
+  }
 
   return FALSE;
 }
@@ -2500,14 +2549,11 @@
 static void skip_secondary_keys(char *create_str, my_bool has_pk)
 {
   char *ptr, *strend;
-  char *last_comma = NULL;
-  HASH ignored_columns;
+  char *last_comma= NULL;
   my_bool pk_processed= FALSE;
-
-  if (hash_init(&ignored_columns, charset_info, 16, 0, 0,
-                (hash_get_key) get_table_key,
-                (hash_free_key) free_table_ent, 0))
-    exit(EX_EOM);
+  char *autoinc_column= NULL;
+  my_bool has_autoinc= FALSE;
+  key_type_t type;
 
   strend= create_str + strlen(create_str);
 
@@ -2515,7 +2561,6 @@
   while (*ptr)
   {
     char *tmp, *orig_ptr, c;
-    my_bool UNINIT_VAR(is_unique);
 
     orig_ptr= ptr;
     /* Skip leading whitespace */
@@ -2528,12 +2573,22 @@
     c= *tmp;
     *tmp= '\0'; /* so strstr() only processes the current line */
 
+    if (!strncmp(ptr, "UNIQUE KEY ", sizeof("UNIQUE KEY ") - 1))
+      type= KEY_TYPE_UNIQUE;
+    else if (!strncmp(ptr, "KEY ", sizeof("KEY ") - 1))
+      type= KEY_TYPE_NON_UNIQUE;
+    else if (!strncmp(ptr, "PRIMARY KEY ", sizeof("PRIMARY KEY ") - 1))
+      type= KEY_TYPE_PRIMARY;
+    else
+      type= KEY_TYPE_NONE;
+
+    has_autoinc= (type != KEY_TYPE_NONE) ?
+      contains_autoinc_column(autoinc_column, ptr, type) : FALSE;
+
     /* Is it a secondary index definition? */
     if (c == '\n' &&
-        (((is_unique= !strncmp(ptr, "UNIQUE KEY ", sizeof("UNIQUE KEY ")-1)) &&
-          (pk_processed || !has_pk)) ||
-         !strncmp(ptr, "KEY ", sizeof("KEY ") - 1)) &&
-        !contains_ignored_column(&ignored_columns, ptr))
+        ((type == KEY_TYPE_UNIQUE && (pk_processed || !has_pk)) ||
+         type == KEY_TYPE_NON_UNIQUE) && !has_autoinc)
     {
       char *data, *end= tmp - 1;
 
@@ -2566,23 +2621,41 @@
         *last_comma= ',';
       }
 
-      if ((has_pk && is_unique && !pk_processed) ||
-          !strncmp(ptr, "PRIMARY KEY ", sizeof("PRIMARY KEY ") - 1))
+      /*
+        If we are skipping a key which indexes an AUTO_INCREMENT column, it is
+        safe to optimize all subsequent keys, i.e. we should not be checking for
+        that column anymore.
+      */
+      if (type != KEY_TYPE_NONE && has_autoinc)
+      {
+          DBUG_ASSERT(autoinc_column != NULL);
+
+          my_free(autoinc_column, MYF(0));
+          autoinc_column= NULL;
+      }
+
+      if ((has_pk && type == KEY_TYPE_UNIQUE && !pk_processed) ||
+          type == KEY_TYPE_PRIMARY)
         pk_processed= TRUE;
 
       if (strstr(ptr, "AUTO_INCREMENT") && *ptr == '`')
       {
         /*
-          If a secondary key is defined on this column later,
-          it cannot be skipped, as CREATE TABLE would fail on import.
+          The first secondary key defined on this column later cannot be
+          skipped, as CREATE TABLE would fail on import. Unless there is a
+          PRIMARY KEY and it indexes that column.
         */
-        for (end= ptr + 1; *end != '`' && *end != '\0'; end++);
-        if (*end == '`' && end > ptr + 1 &&
-            my_hash_insert(&ignored_columns,
-                           (uchar *) my_strndup(ptr + 1,
-                                                end - ptr - 1, MYF(0))))
+        for (end= ptr + 1;
+             /* Skip double backticks as they are a part of identifier */
+             *end != '\0' && (*end != '`' || end[1] == '`');
+             end++)
+          /* empty */;
+
+        if (*end == '`' && end > ptr + 1)
         {
-          exit(EX_EOM);
+          DBUG_ASSERT(autoinc_column == NULL);
+
+          autoinc_column= my_strndup(ptr + 1, end - ptr - 1, MYF(MY_FAE));
         }
       }
 
@@ -2594,7 +2667,7 @@
     }
   }
 
-  hash_free(&ignored_columns);
+  my_free(autoinc_column, MYF(MY_ALLOW_ZERO_PTR));
 }
 
 /*
 
=== modified file 'Percona-Server/mysql-test/r/percona_mysqldump_innodb_optimize_keys.result'
--- Percona-Server/mysql-test/r/percona_mysqldump_innodb_optimize_keys.result	2011-11-24 02:01:33 +0000
+++ Percona-Server/mysql-test/r/percona_mysqldump_innodb_optimize_keys.result	2012-11-20 09:37:22 +0000
@@ -365,3 +365,77 @@
 
 ######################################
 DROP TABLE t1, t2;
+CREATE TABLE t1 (
+id INT NOT NULL AUTO_INCREMENT,
+uid INT NOT NULL,
+`id``` INT NOT NULL,
+```id` INT NOT NULL,
+# The following ones may be skipped and used in ALTER TABLE later
+KEY k1 (```id`, id),
+KEY k2 (```id`, `id```),
+# The following one should be kept in CREATE TABLE
+KEY k3 (id, uid),
+# The following one may be skipped again
+KEY k4 (id, `id```)
+) ENGINE=InnoDB;
+CREATE TABLE t2 (
+id INT NOT NULL AUTO_INCREMENT,
+PRIMARY KEY (id),
+KEY k1 (id),
+KEY k2 (id)
+) ENGINE=InnoDB;
+######################################
+
+/*!40101 SET @OLD_CHARACTER_SET_CLIENT=@@CHARACTER_SET_CLIENT */;
+/*!40101 SET @OLD_CHARACTER_SET_RESULTS=@@CHARACTER_SET_RESULTS */;
+/*!40101 SET @OLD_COLLATION_CONNECTION=@@COLLATION_CONNECTION */;
+/*!40101 SET NAMES utf8 */;
+/*!40103 SET @OLD_TIME_ZONE=@@TIME_ZONE */;
+/*!40103 SET TIME_ZONE='+00:00' */;
+/*!40014 SET @OLD_UNIQUE_CHECKS=@@UNIQUE_CHECKS, UNIQUE_CHECKS=0 */;
+/*!40014 SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0 */;
+/*!40101 SET @OLD_SQL_MODE=@@SQL_MODE, SQL_MODE='NO_AUTO_VALUE_ON_ZERO' */;
+/*!40111 SET @OLD_SQL_NOTES=@@SQL_NOTES, SQL_NOTES=0 */;
+DROP TABLE IF EXISTS `t1`;
+/*!40101 SET @saved_cs_client     = @@character_set_client */;
+/*!40101 SET character_set_client = utf8 */;
+CREATE TABLE `t1` (
+  `id` int(11) NOT NULL AUTO_INCREMENT,
+  `uid` int(11) NOT NULL,
+  `id``` int(11) NOT NULL,
+  ```id` int(11) NOT NULL,
+  KEY `k3` (`id`,`uid`) 
+) ENGINE=InnoDB DEFAULT CHARSET=latin1;
+/*!40101 SET character_set_client = @saved_cs_client */;
+
+LOCK TABLES `t1` WRITE;
+/*!40000 ALTER TABLE `t1` DISABLE KEYS */;
+ALTER TABLE `t1` ADD KEY `k1` (```id`,`id`), ADD KEY `k2` (```id`,`id```), ADD KEY `k4` (`id`,`id```);
+/*!40000 ALTER TABLE `t1` ENABLE KEYS */;
+UNLOCK TABLES;
+DROP TABLE IF EXISTS `t2`;
+/*!40101 SET @saved_cs_client     = @@character_set_client */;
+/*!40101 SET character_set_client = utf8 */;
+CREATE TABLE `t2` (
+  `id` int(11) NOT NULL AUTO_INCREMENT,
+  PRIMARY KEY (`id`) 
+) ENGINE=InnoDB DEFAULT CHARSET=latin1;
+/*!40101 SET character_set_client = @saved_cs_client */;
+
+LOCK TABLES `t2` WRITE;
+/*!40000 ALTER TABLE `t2` DISABLE KEYS */;
+ALTER TABLE `t2` ADD KEY `k1` (`id`), ADD KEY `k2` (`id`);
+/*!40000 ALTER TABLE `t2` ENABLE KEYS */;
+UNLOCK TABLES;
+/*!40103 SET TIME_ZONE=@OLD_TIME_ZONE */;
+
+/*!40101 SET SQL_MODE=@OLD_SQL_MODE */;
+/*!40014 SET FOREIGN_KEY_CHECKS=@OLD_FOREIGN_KEY_CHECKS */;
+/*!40014 SET UNIQUE_CHECKS=@OLD_UNIQUE_CHECKS */;
+/*!40101 SET CHARACTER_SET_CLIENT=@OLD_CHARACTER_SET_CLIENT */;
+/*!40101 SET CHARACTER_SET_RESULTS=@OLD_CHARACTER_SET_RESULTS */;
+/*!40101 SET COLLATION_CONNECTION=@OLD_COLLATION_CONNECTION */;
+/*!40111 SET SQL_NOTES=@OLD_SQL_NOTES */;
+
+######################################
+DROP TABLE t1, t2;
 
=== modified file 'Percona-Server/mysql-test/t/percona_mysqldump_innodb_optimize_keys.test'
--- Percona-Server/mysql-test/t/percona_mysqldump_innodb_optimize_keys.test	2011-11-24 02:01:33 +0000
+++ Percona-Server/mysql-test/t/percona_mysqldump_innodb_optimize_keys.test	2012-11-20 09:37:22 +0000
@@ -183,5 +183,45 @@
 
 DROP TABLE t1, t2;
 
+########################################################################
+# Bug #1039536: mysqldump --innodb-optimize-keys can generate invalid table
+#               definitions
+########################################################################
+
+CREATE TABLE t1 (
+       id INT NOT NULL AUTO_INCREMENT,
+       uid INT NOT NULL,
+       `id``` INT NOT NULL,
+       ```id` INT NOT NULL,
+       # The following ones may be skipped and used in ALTER TABLE later
+       KEY k1 (```id`, id),
+       KEY k2 (```id`, `id```),
+       # The following one should be kept in CREATE TABLE
+       KEY k3 (id, uid),
+       # The following one may be skipped again
+       KEY k4 (id, `id```)
+) ENGINE=InnoDB;
+
+CREATE TABLE t2 (
+       id INT NOT NULL AUTO_INCREMENT,
+       PRIMARY KEY (id),
+       KEY k1 (id),
+       KEY k2 (id)
+) ENGINE=InnoDB;
+
+--exec $MYSQL_DUMP --skip-comments --innodb-optimize-keys test t1 t2 >$file
+
+--echo ######################################
+--cat_file $file
+--echo ######################################
+
+# Check that the resulting dump can be imported back
+
+--exec $MYSQL test < $file
+
+--remove_file $file
+
+DROP TABLE t1, t2;
+
 # Wait till we reached the initial number of concurrent sessions
 --source include/wait_until_count_sessions.inc