On Mon, Oct 9, 2017 at 3:00 PM, jan 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