← Back to team overview

maria-developers team mailing list archive

Re: [Commits] 0240b866abc: MDEV-13838: Wrong result after altering a partitioned table

 

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