On Mon, Oct 9, 2017 at 3:00 PM, jan <jan.lindstrom@mariadb.com> wrote:
revision-id: 109463fdb86d (mariadb-10.0.32-54-g0240b866abc)
parent(s): 172cc70bf8c0aea3d2d0c73bcf94f36c172b769a
author: Jan Lindström
committer: Jan Lindström
timestamp: 2017-10-09 14:52:51 +0300
message:
MDEV-13838: Wrong result after altering a partitioned table
Reverted incorrect changes done on MDEV-7367 and MDEV-9469. Fixes properly
also related bugs:
MDEV-13668: InnoDB unnecessarily rebuilds table when renaming a column and
adding index
MDEV-9469: 'Incorrect key file' on ALTER TABLE
MDEV-9548: Alter table (renaming and adding index) fails with "Incorrect
key file for table"
MDEV-10535: ALTER TABLE causes standalone/wsrep cluster crash
MDEV-13640: ALTER TABLE CHANGE and ADD INDEX on auto_increment column
fails with "Incorrect key file for table..."
This is a very welcome change that looks correct to me. I only have a few
comments on the formatting and tests.
The patch does not cleanly apply to 10.0 due to some difference in the
comments of removed functions.
Later, you informed me that this is
https://github.com/MariaDB/server/commit/a8db085c51ccfd6df737d9261abac9d2b0e...
on bb-10.0-MDEV-13838
<https://github.com/MariaDB/server/compare/bb-10.0-MDEV-13838>.
+Warnings:
+Note 1831 Duplicate index `c4_2`. This is deprecated and will be
disallowed in a future release.
The intention of the test was not to create duplicated indexes. The "rename
column" and "add index" operations were split into two ALTER TABLE in order
to work around the limitation that was introduced in a previous incorrect
commit. You should have removed the added ALTER TABLE statements instead of
adding SHOW CREATE TABLE statements.
Please refer to
https://github.com/MariaDB/server/commit/f56bd70f51020c67a30415ae381ae93b908...
and revert the relevant parts of the test changes. The other tests seem to
be adjusted fine.
@@ -171,3 +172,39 @@ ALTER TABLE ticket
SHOW CREATE TABLE ticket;
DROP TABLE ticket;
+
+#
+# MDEV-13838: Wrong result after altering a partitioned table
+#
+
+CREATE TABLE `t` (
+`id` bigint(20) unsigned NOT NULL auto_increment,
+`d` date NOT NULL,
+`a` bigint(20) unsigned NOT NULL,
+`b` smallint(5) unsigned DEFAULT NULL,
+PRIMARY KEY (`id`,`d`)
+) ENGINE=InnoDB DEFAULT CHARSET=latin1 COLLATE=latin1_general_cs
STATS_SAMPLE_PAGES=200
+/*!50500 PARTITION BY RANGE COLUMNS(d)
+(PARTITION p20170913 VALUES LESS THAN ('2017-09-14') ENGINE = InnoDB,
+PARTITION p20170914 VALUES LESS THAN ('2017-09-15') ENGINE = InnoDB,
+PARTITION p20170915 VALUES LESS THAN ('2017-09-16') ENGINE = InnoDB,
+PARTITION p20170916 VALUES LESS THAN ('2017-09-17') ENGINE = InnoDB,
+PARTITION p20170917 VALUES LESS THAN ('2017-09-18') ENGINE = InnoDB,
+PARTITION p99991231 VALUES LESS THAN (MAXVALUE) ENGINE = InnoDB) */;
Please clean up the CREATE TABLE statement. Remove trailing space and
unnecessary quotes and comments.
+insert into t(d,a,b) values ('2017-09-15',rand()*10000,rand()*10);
+insert into t(d,a,b) values ('2017-09-15',rand()*10000,rand()*10);
+
+replace into t(d,a,b) select '2017-09-15',rand()*10000,rand()*10 from t
t1, t t2, t t3, t t4, t t5, t t6, t t7, t t8, t t9, t t10, t t11, t t12, t
t13, t t14;
Does the table really have to be this big for the bug to repeat? Can you
please reduce the test? Shouldn’t it work with 1 or 2 partitions and a few
records in each?
+
+select count(*) from t where d ='2017-09-15';
+
+ALTER TABLE t CHANGE b c smallint(5) unsigned , ADD KEY idx_d_a (d, a);
+SHOW CREATE TABLE t;
+analyze table t;
+
+select count(*) from t where d ='2017-09-15';
+select count(*) from t force index(primary) where d ='2017-09-15';
+
+DROP TABLE t;
+
Please do not end files in empty lines.
@@ -1625,13 +1606,9 @@ innobase_create_index_def(
index->ind_type |= DICT_FTS;
}
- if (!new_clustered) {
- altered_table = NULL;
- }
-
for (i = 0; i < n_fields; i++) {
- innobase_create_index_field_def(
- altered_table, &key->key_part[i],
&index->fields[i], fields);
+ innobase_create_index_field_def(new_clustered,
+ altered_table, &key->key_part[i],
&index->fields[i]);
}
DBUG_VOID_RETURN;
Do not add the first parameter to the same line with the function name.
Instead, add it on a line on its own. In this way, it follows the
indentation rules set by
(defun innodb-cc-mode (&optional map)
"InnoDB adjustments to cc-mode. In MAP, binds C-c C-c to 'compile."
(if (and
(buffer-file-name)
(string-match "/\\(innobase\\|xtradb\\).*/.*[ihc]\\'"
(buffer-file-name)))
(progn
(setq
mode-name (concat "Ib" mode-name)
c-basic-offset 8
c-label-minimum-indentation 0)
(add-to-list 'c-offsets-alist '(c . 0))
(add-to-list 'c-offsets-alist '(arglist-intro . +))
(add-to-list 'c-offsets-alist '(label . [0]))))
(if map
(define-key map "\C-c\C-c" 'compile)))
@@ -2121,7 +2097,7 @@ struct ha_innobase_inplace_ctx : public
inplace_alter_handler_ctx
/** mapping of old column numbers to new ones, or NULL */
const ulint* col_map;
/** new column names, or NULL if nothing was renamed */
- const char** col_names;
+ const char** col_names;
/** added AUTO_INCREMENT column position, or ULINT_UNDEFINED */
const ulint add_autoinc;
/** default values of ADD COLUMN, or NULL */
Please do not change TAB to spaces.
@@ -3067,7 +3043,7 @@ prepare_inplace_alter_table_dict(
ctx->add_index[a] = row_merge_create_index(
ctx->trx, ctx->new_table,
- &index_defs[a], ctx->col_names);
+ &index_defs[a]);
add_key_nums[a] = index_defs[a].key_number;
Consider joining the line after new_table.
Best regards,
Marko