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

Proposed by Alexey Kopytov on 2012-11-20
Status: Merged
Approved by: Laurynas Biveinis on 2012-11-30
Approved revision: 503
Merged at revision: 501
Proposed branch: lp:~akopytov/percona-server/bugs-1039536-1081003-5.1
Merge into: lp:percona-server/5.1
Diff against target: 339 lines (+222/-35)
3 files modified
Percona-Server/client/mysqldump.c (+108/-35)
Percona-Server/mysql-test/r/percona_mysqldump_innodb_optimize_keys.result (+74/-0)
Percona-Server/mysql-test/t/percona_mysqldump_innodb_optimize_keys.test (+40/-0)
To merge this branch: bzr merge lp:~akopytov/percona-server/bugs-1039536-1081003-5.1
Reviewer Review Type Date Requested Status
Laurynas Biveinis (community) 2012-11-20 Approve on 2012-11-30
Review via email: mp+135091@code.launchpad.net

Description of the change

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

To post a comment you must log in.

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) wrote :

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.

Right, it must be the prefix comparison, sorry.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'Percona-Server/client/mysqldump.c'
2--- Percona-Server/client/mysqldump.c 2012-08-20 03:14:02 +0000
3+++ Percona-Server/client/mysqldump.c 2012-11-20 09:37:22 +0000
4@@ -85,6 +85,13 @@
5 #define IGNORE_DATA 0x01 /* don't dump data for this table */
6 #define IGNORE_INSERT_DELAYED 0x02 /* table doesn't support INSERT DELAYED */
7
8+typedef enum {
9+ KEY_TYPE_NONE,
10+ KEY_TYPE_PRIMARY,
11+ KEY_TYPE_UNIQUE,
12+ KEY_TYPE_NON_UNIQUE
13+} key_type_t;
14+
15 /* general_log or slow_log tables under mysql database */
16 static inline my_bool general_log_or_slow_log_tables(const char *db,
17 const char *table)
18@@ -2460,20 +2467,62 @@
19 }
20
21 /*
22- Parse the specified key definition string and check if the key indexes
23- any of the columns from ignored_columns.
24+ Parse the specified key definition string and check if the key contains an
25+ AUTO_INCREMENT column as the first key part. We only check for the first key
26+ part, because unlike MyISAM, InnoDB does not allow the AUTO_INCREMENT column
27+ as a secondary key column, i.e. the AUTO_INCREMENT column would not be
28+ considered indexed for such key specification.
29 */
30-static my_bool contains_ignored_column(HASH *ignored_columns, char *keydef)
31+static my_bool contains_autoinc_column(const char *autoinc_column,
32+ const char *keydef,
33+ key_type_t type)
34 {
35- char *leftp, *rightp;
36-
37- if ((leftp = strchr(keydef, '(')) &&
38- (rightp = strchr(leftp, ')')) &&
39- rightp > leftp + 3 && /* (`...`) */
40- leftp[1] == '`' &&
41- rightp[-1] == '`' &&
42- hash_search(ignored_columns, (uchar *) leftp + 2, rightp - leftp - 3))
43- return TRUE;
44+ char *from, *to;
45+ uint idnum;
46+
47+ DBUG_ASSERT(type != KEY_TYPE_NONE);
48+
49+ if (autoinc_column == NULL || !(from= strchr(keydef, '`')))
50+ return FALSE;
51+
52+ to= from;
53+ idnum= 0;
54+
55+ while ((to= strchr(to + 1, '`')))
56+ {
57+ /*
58+ Double backticks represent a backtick in identifier, rather than a quote
59+ character.
60+ */
61+ if (to[1] == '`')
62+ {
63+ to++;
64+ continue;
65+ }
66+
67+ if (to <= from + 1)
68+ break; /* Broken key definition */
69+
70+ idnum++;
71+
72+ /*
73+ Skip the check if it's the first identifier and we are processing a
74+ secondary key.
75+ */
76+ if ((type == KEY_TYPE_PRIMARY || idnum != 1) &&
77+ !strncmp(autoinc_column, from + 1, to - from - 1))
78+ return TRUE;
79+
80+ /*
81+ Check only the first (for PRIMARY KEY) or the second (for secondary keys)
82+ quoted identifier.
83+ */
84+ if ((idnum == 1 + test(type != KEY_TYPE_PRIMARY)) ||
85+ !(from= strchr(to + 1, '`')))
86+ break;
87+
88+ to= from;
89+ }
90
91 return FALSE;
92 }
93@@ -2500,14 +2549,11 @@
94 static void skip_secondary_keys(char *create_str, my_bool has_pk)
95 {
96 char *ptr, *strend;
97- char *last_comma = NULL;
98- HASH ignored_columns;
99+ char *last_comma= NULL;
100 my_bool pk_processed= FALSE;
101-
102- if (hash_init(&ignored_columns, charset_info, 16, 0, 0,
103- (hash_get_key) get_table_key,
104- (hash_free_key) free_table_ent, 0))
105- exit(EX_EOM);
106+ char *autoinc_column= NULL;
107+ my_bool has_autoinc= FALSE;
108+ key_type_t type;
109
110 strend= create_str + strlen(create_str);
111
112@@ -2515,7 +2561,6 @@
113 while (*ptr)
114 {
115 char *tmp, *orig_ptr, c;
116- my_bool UNINIT_VAR(is_unique);
117
118 orig_ptr= ptr;
119 /* Skip leading whitespace */
120@@ -2528,12 +2573,22 @@
121 c= *tmp;
122 *tmp= '\0'; /* so strstr() only processes the current line */
123
124+ if (!strncmp(ptr, "UNIQUE KEY ", sizeof("UNIQUE KEY ") - 1))
125+ type= KEY_TYPE_UNIQUE;
126+ else if (!strncmp(ptr, "KEY ", sizeof("KEY ") - 1))
127+ type= KEY_TYPE_NON_UNIQUE;
128+ else if (!strncmp(ptr, "PRIMARY KEY ", sizeof("PRIMARY KEY ") - 1))
129+ type= KEY_TYPE_PRIMARY;
130+ else
131+ type= KEY_TYPE_NONE;
132+
133+ has_autoinc= (type != KEY_TYPE_NONE) ?
134+ contains_autoinc_column(autoinc_column, ptr, type) : FALSE;
135+
136 /* Is it a secondary index definition? */
137 if (c == '\n' &&
138- (((is_unique= !strncmp(ptr, "UNIQUE KEY ", sizeof("UNIQUE KEY ")-1)) &&
139- (pk_processed || !has_pk)) ||
140- !strncmp(ptr, "KEY ", sizeof("KEY ") - 1)) &&
141- !contains_ignored_column(&ignored_columns, ptr))
142+ ((type == KEY_TYPE_UNIQUE && (pk_processed || !has_pk)) ||
143+ type == KEY_TYPE_NON_UNIQUE) && !has_autoinc)
144 {
145 char *data, *end= tmp - 1;
146
147@@ -2566,23 +2621,41 @@
148 *last_comma= ',';
149 }
150
151- if ((has_pk && is_unique && !pk_processed) ||
152- !strncmp(ptr, "PRIMARY KEY ", sizeof("PRIMARY KEY ") - 1))
153+ /*
154+ If we are skipping a key which indexes an AUTO_INCREMENT column, it is
155+ safe to optimize all subsequent keys, i.e. we should not be checking for
156+ that column anymore.
157+ */
158+ if (type != KEY_TYPE_NONE && has_autoinc)
159+ {
160+ DBUG_ASSERT(autoinc_column != NULL);
161+
162+ my_free(autoinc_column, MYF(0));
163+ autoinc_column= NULL;
164+ }
165+
166+ if ((has_pk && type == KEY_TYPE_UNIQUE && !pk_processed) ||
167+ type == KEY_TYPE_PRIMARY)
168 pk_processed= TRUE;
169
170 if (strstr(ptr, "AUTO_INCREMENT") && *ptr == '`')
171 {
172 /*
173- If a secondary key is defined on this column later,
174- it cannot be skipped, as CREATE TABLE would fail on import.
175+ The first secondary key defined on this column later cannot be
176+ skipped, as CREATE TABLE would fail on import. Unless there is a
177+ PRIMARY KEY and it indexes that column.
178 */
179- for (end= ptr + 1; *end != '`' && *end != '\0'; end++);
180- if (*end == '`' && end > ptr + 1 &&
181- my_hash_insert(&ignored_columns,
182- (uchar *) my_strndup(ptr + 1,
183- end - ptr - 1, MYF(0))))
184+ for (end= ptr + 1;
185+ /* Skip double backticks as they are a part of identifier */
186+ *end != '\0' && (*end != '`' || end[1] == '`');
187+ end++)
188+ /* empty */;
189+
190+ if (*end == '`' && end > ptr + 1)
191 {
192- exit(EX_EOM);
193+ DBUG_ASSERT(autoinc_column == NULL);
194+
195+ autoinc_column= my_strndup(ptr + 1, end - ptr - 1, MYF(MY_FAE));
196 }
197 }
198
199@@ -2594,7 +2667,7 @@
200 }
201 }
202
203- hash_free(&ignored_columns);
204+ my_free(autoinc_column, MYF(MY_ALLOW_ZERO_PTR));
205 }
206
207 /*
208
209=== modified file 'Percona-Server/mysql-test/r/percona_mysqldump_innodb_optimize_keys.result'
210--- Percona-Server/mysql-test/r/percona_mysqldump_innodb_optimize_keys.result 2011-11-24 02:01:33 +0000
211+++ Percona-Server/mysql-test/r/percona_mysqldump_innodb_optimize_keys.result 2012-11-20 09:37:22 +0000
212@@ -365,3 +365,77 @@
213
214 ######################################
215 DROP TABLE t1, t2;
216+CREATE TABLE t1 (
217+id INT NOT NULL AUTO_INCREMENT,
218+uid INT NOT NULL,
219+`id``` INT NOT NULL,
220+```id` INT NOT NULL,
221+# The following ones may be skipped and used in ALTER TABLE later
222+KEY k1 (```id`, id),
223+KEY k2 (```id`, `id```),
224+# The following one should be kept in CREATE TABLE
225+KEY k3 (id, uid),
226+# The following one may be skipped again
227+KEY k4 (id, `id```)
228+) ENGINE=InnoDB;
229+CREATE TABLE t2 (
230+id INT NOT NULL AUTO_INCREMENT,
231+PRIMARY KEY (id),
232+KEY k1 (id),
233+KEY k2 (id)
234+) ENGINE=InnoDB;
235+######################################
236+
237+/*!40101 SET @OLD_CHARACTER_SET_CLIENT=@@CHARACTER_SET_CLIENT */;
238+/*!40101 SET @OLD_CHARACTER_SET_RESULTS=@@CHARACTER_SET_RESULTS */;
239+/*!40101 SET @OLD_COLLATION_CONNECTION=@@COLLATION_CONNECTION */;
240+/*!40101 SET NAMES utf8 */;
241+/*!40103 SET @OLD_TIME_ZONE=@@TIME_ZONE */;
242+/*!40103 SET TIME_ZONE='+00:00' */;
243+/*!40014 SET @OLD_UNIQUE_CHECKS=@@UNIQUE_CHECKS, UNIQUE_CHECKS=0 */;
244+/*!40014 SET @OLD_FOREIGN_KEY_CHECKS=@@FOREIGN_KEY_CHECKS, FOREIGN_KEY_CHECKS=0 */;
245+/*!40101 SET @OLD_SQL_MODE=@@SQL_MODE, SQL_MODE='NO_AUTO_VALUE_ON_ZERO' */;
246+/*!40111 SET @OLD_SQL_NOTES=@@SQL_NOTES, SQL_NOTES=0 */;
247+DROP TABLE IF EXISTS `t1`;
248+/*!40101 SET @saved_cs_client = @@character_set_client */;
249+/*!40101 SET character_set_client = utf8 */;
250+CREATE TABLE `t1` (
251+ `id` int(11) NOT NULL AUTO_INCREMENT,
252+ `uid` int(11) NOT NULL,
253+ `id``` int(11) NOT NULL,
254+ ```id` int(11) NOT NULL,
255+ KEY `k3` (`id`,`uid`)
256+) ENGINE=InnoDB DEFAULT CHARSET=latin1;
257+/*!40101 SET character_set_client = @saved_cs_client */;
258+
259+LOCK TABLES `t1` WRITE;
260+/*!40000 ALTER TABLE `t1` DISABLE KEYS */;
261+ALTER TABLE `t1` ADD KEY `k1` (```id`,`id`), ADD KEY `k2` (```id`,`id```), ADD KEY `k4` (`id`,`id```);
262+/*!40000 ALTER TABLE `t1` ENABLE KEYS */;
263+UNLOCK TABLES;
264+DROP TABLE IF EXISTS `t2`;
265+/*!40101 SET @saved_cs_client = @@character_set_client */;
266+/*!40101 SET character_set_client = utf8 */;
267+CREATE TABLE `t2` (
268+ `id` int(11) NOT NULL AUTO_INCREMENT,
269+ PRIMARY KEY (`id`)
270+) ENGINE=InnoDB DEFAULT CHARSET=latin1;
271+/*!40101 SET character_set_client = @saved_cs_client */;
272+
273+LOCK TABLES `t2` WRITE;
274+/*!40000 ALTER TABLE `t2` DISABLE KEYS */;
275+ALTER TABLE `t2` ADD KEY `k1` (`id`), ADD KEY `k2` (`id`);
276+/*!40000 ALTER TABLE `t2` ENABLE KEYS */;
277+UNLOCK TABLES;
278+/*!40103 SET TIME_ZONE=@OLD_TIME_ZONE */;
279+
280+/*!40101 SET SQL_MODE=@OLD_SQL_MODE */;
281+/*!40014 SET FOREIGN_KEY_CHECKS=@OLD_FOREIGN_KEY_CHECKS */;
282+/*!40014 SET UNIQUE_CHECKS=@OLD_UNIQUE_CHECKS */;
283+/*!40101 SET CHARACTER_SET_CLIENT=@OLD_CHARACTER_SET_CLIENT */;
284+/*!40101 SET CHARACTER_SET_RESULTS=@OLD_CHARACTER_SET_RESULTS */;
285+/*!40101 SET COLLATION_CONNECTION=@OLD_COLLATION_CONNECTION */;
286+/*!40111 SET SQL_NOTES=@OLD_SQL_NOTES */;
287+
288+######################################
289+DROP TABLE t1, t2;
290
291=== modified file 'Percona-Server/mysql-test/t/percona_mysqldump_innodb_optimize_keys.test'
292--- Percona-Server/mysql-test/t/percona_mysqldump_innodb_optimize_keys.test 2011-11-24 02:01:33 +0000
293+++ Percona-Server/mysql-test/t/percona_mysqldump_innodb_optimize_keys.test 2012-11-20 09:37:22 +0000
294@@ -183,5 +183,45 @@
295
296 DROP TABLE t1, t2;
297
298+########################################################################
299+# Bug #1039536: mysqldump --innodb-optimize-keys can generate invalid table
300+# definitions
301+########################################################################
302+
303+CREATE TABLE t1 (
304+ id INT NOT NULL AUTO_INCREMENT,
305+ uid INT NOT NULL,
306+ `id``` INT NOT NULL,
307+ ```id` INT NOT NULL,
308+ # The following ones may be skipped and used in ALTER TABLE later
309+ KEY k1 (```id`, id),
310+ KEY k2 (```id`, `id```),
311+ # The following one should be kept in CREATE TABLE
312+ KEY k3 (id, uid),
313+ # The following one may be skipped again
314+ KEY k4 (id, `id```)
315+) ENGINE=InnoDB;
316+
317+CREATE TABLE t2 (
318+ id INT NOT NULL AUTO_INCREMENT,
319+ PRIMARY KEY (id),
320+ KEY k1 (id),
321+ KEY k2 (id)
322+) ENGINE=InnoDB;
323+
324+--exec $MYSQL_DUMP --skip-comments --innodb-optimize-keys test t1 t2 >$file
325+
326+--echo ######################################
327+--cat_file $file
328+--echo ######################################
329+
330+# Check that the resulting dump can be imported back
331+
332+--exec $MYSQL test < $file
333+
334+--remove_file $file
335+
336+DROP TABLE t1, t2;
337+
338 # Wait till we reached the initial number of concurrent sessions
339 --source include/wait_until_count_sessions.inc

Subscribers

People subscribed via source and target branches