From: Jon Olav Hauglid
Date: June 10 2011 8:27am
Subject: Re: bzr commit into mysql-trunk branch (jorgen.loland:3172) Bug#11882110
List-Archive: http://lists.mysql.com/commits/139038
Message-Id: <4DF1D559.9020607@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

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