List:Commits« Previous MessageNext Message »
From:Jon Olav Hauglid Date:June 10 2011 8:27am
Subject:Re: bzr commit into mysql-trunk branch (jorgen.loland:3172) Bug#11882110
View as plain text  
Hello!

On 06/09/2011 12:35 PM, Jorgen Loland wrote:
>   3172 Jorgen Loland	2011-06-09
>        BUG#11882110: UPDATE REPORTS ER_KEY_NOT_FOUND IF TABLE IS
>                      UPDATED TWICE

Patch approved, but please consider the comments below.

>        For multi update it is not allowed to update certain columns

certain columns? Please clarify this comment.

>        of a table if that table is accessed through multiple aliases
>        if either
>        1) one of the updated columns is used as partitioning key
>        2) one of the updated columns is part of the primary key
>           and the primary key is clustered
>
>        This check is done in unsafe_key_update().
>
>        The bug was that for case 2), it was checked whether
>        updated_column_number == table_share->primary_key
>        However, the primary_key variable is the index number of the
>        primary key, not a column number.

Consider including a sentence or two about what the visible effects
of this bug was to users. Also mention that the issue was specific to
InnoDB and why.

>        The columns covered by an index is found in
>        table->key_info[idx_number]->key_part. The bugfix is to check
>        if any of the columns in the keyparts of the primary key is
>        updated.
>       @ mysql-test/r/multi_update.result
>          Add test for bug#11882110

I think it should be mentioned that this MyISAM test is added to
document existing behavior rather than serve as a regression test
for this bug. Please also include this as a comment in the test
case itself.

>       @ mysql-test/r/multi_update_innodb.result
>          Add test for bug#11882110
>       @ mysql-test/t/multi_update.test
>          Add test for bug#11882110
>       @ mysql-test/t/multi_update_innodb.test
>          Add test for bug#11882110
>       @ sql/sql_update.cc
>          unsafe_key_update() wrongly checked if the primary key index number was the
> same as updated column number. Now it is checked whether any of the columns making up the
> primary key is updated.

Long line.

>       @ sql/table.h
>          Remove comment that has been introduced by a merge conflict (as per dlenev)
>
>      modified:
>        mysql-test/r/multi_update.result
>        mysql-test/r/multi_update_innodb.result
>        mysql-test/t/multi_update.test
>        mysql-test/t/multi_update_innodb.test
>        sql/sql_update.cc
>        sql/table.h

> === modified file 'sql/sql_update.cc'
> --- a/sql/sql_update.cc	2011-06-09 08:58:41 +0000
> +++ b/sql/sql_update.cc	2011-06-09 10:35:26 +0000
> @@ -1069,17 +1069,27 @@ bool unsafe_key_update(TABLE_LIST *leave
>               return true;
>             }
>
> -          if (primkey_clustered&&
> -              (bitmap_is_set(table1->write_set, table1->s->primary_key) ||
> -               bitmap_is_set(table2->write_set, table2->s->primary_key)))
> -          {
> -            // Clustered primary key is updated
> -            my_error(ER_MULTI_UPDATE_KEY_CONFLICT, MYF(0),
> -                     tl->belong_to_view ? tl->belong_to_view->alias
> -                                        : tl->alias,
> -                     tl2->belong_to_view ? tl2->belong_to_view->alias
> -                                         : tl2->alias);
> -            return true;
> +          if (primkey_clustered)
> +          {

Trailing space.

> +            // The primary key can cover multiple columns
> +            KEY key_info= table1->key_info[table1->s->primary_key];
> +            KEY_PART_INFO *key_part= key_info.key_part;
> +            KEY_PART_INFO *key_part_end= key_part + key_info.key_parts;
> +
> +            for (;key_part != key_part_end; ++key_part)
> +            {
> +              if (bitmap_is_set(table1->write_set, key_part->fieldnr-1) ||
> +                  bitmap_is_set(table2->write_set, key_part->fieldnr-1))
> +              {
> +                // Clustered primary key is updated
> +                my_error(ER_MULTI_UPDATE_KEY_CONFLICT, MYF(0),
> +                         tl->belong_to_view ? tl->belong_to_view->alias
> +                         : tl->alias,
> +                         tl2->belong_to_view ? tl2->belong_to_view->alias
> +                         : tl2->alias);
> +                return true;
> +              }
> +            }
>             }
>           }
>         }
>
> === modified file 'sql/table.h'
> --- a/sql/table.h	2011-05-26 15:20:09 +0000
> +++ b/sql/table.h	2011-06-09 10:35:26 +0000
> @@ -669,7 +669,6 @@ struct TABLE_SHARE
>     uint db_options_in_use;		/* Options in use */
>     uint db_record_offset;		/* if HA_REC_IN_SEQ */
>     uint rowid_field_offset;		/* Field_nr +1 to rowid field */
> -  /* Index of auto-updated TIMESTAMP field in field array */
>     uint primary_key;
>     uint next_number_index;               /* autoincrement key number */
>     uint next_number_key_offset;          /* autoinc keypart offset in a key */

Maybe add a comment explaining primary_key? Mention that this is the
index number and not the column number :-)

Thanks,

--- Jon Olav
Thread
bzr commit into mysql-trunk branch (jorgen.loland:3172) Bug#11882110Jorgen Loland9 Jun
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3172) Bug#11882110Jon Olav Hauglid10 Jun