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/a8db085c51ccfd6df737d9261abac9d2b0eb8399 on 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/f56bd70f51020c67a30415ae381ae93b9087d88d 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