List:Commits« Previous MessageNext Message »
From:Jon Olav Hauglid Date:February 15 2011 10:15am
Subject:Re: bzr commit into mysql-trunk branch (jorgen.loland:3641) Bug#11762751
Bug#11764529
View as plain text  
Hello,

Comments inline.

On 02/14/2011 03:51 PM, Jorgen Loland wrote:
>   3641 Jorgen Loland	2011-02-14
>        BUG#11762751: UPDATE STATEMENT THROWS AN ERROR, BUT STILL
>                      UPDATES THE TABLE ENTRIES (formerly 55385)
>        BUG#11764529: MULTI UPDATE+INNODB REPORTS ER_KEY_NOT_FOUND
>                      IF A TABLE IS UPDATED TWICE (formerly 57373)
>
>        If multiple-table update updates a row through two aliases and
>        the first update physically moves the row, the second update will
>        fail to locate the row. This results in an error.

Maybe you could mention why this was a problem? After all, we end up 
returning an error after this patch as well.

>        The fix for these bugs is to return with an error if multiple-table
>        update is about to:
>          a) Update a table through multiple aliases, and
>          b) Perform an update that may physically more the row
>             in at least one of these aliases

Same as above, consider describing why this is an improvement over the 
current situation.

> === added file 'mysql-test/r/multi_update_innodb.result'
> --- a/mysql-test/r/multi_update_innodb.result	1970-01-01 00:00:00 +0000
> +++ b/mysql-test/r/multi_update_innodb.result	2011-02-14 14:51:47 +0000
> @@ -0,0 +1,25 @@
> +#
> +# BUG#57373: Multi update+InnoDB reports ER_KEY_NOT_FOUND if a
> +#            table is updated twice
> +#
> +CREATE TABLE t1(
> +pk INT,
> +a INT,
> +PRIMARY KEY (pk)
> +) ENGINE=InnoDB;
> +INSERT INTO t1 VALUES (0,0);
> +UPDATE t1 AS A, t1 AS B SET A.pk = 1, B.a = 2;
> +ERROR HY000: Primary key/partition key update is not allowed since the table is
> updated both as 'A' and 'B'.
> +SELECT * FROM t1;
> +pk	a
> +0	0
> +CREATE VIEW v1 AS SELECT * FROM t1;
> +UPDATE v1 AS A, t1 AS B SET A.pk = 1, B.a = 2;
> +ERROR HY000: Primary key/partition key update is not allowed since the table is
> updated both as 'A' and 'B'.
> +
> +# Should be (0,0)
> +SELECT * FROM t1;
> +pk	a
> +0	0
> +DROP VIEW v1;
> +DROP TABLE t1;

I think it would be good to also cover the behavior of an InnoDB table 
without a primary key.

> === modified file 'mysql-test/r/partition.result'
> --- a/mysql-test/r/partition.result	2011-01-10 16:37:47 +0000
> +++ b/mysql-test/r/partition.result	2011-02-14 14:51:47 +0000
> @@ -2263,3 +2263,51 @@ INSERT INTO t1 VALUES(0);
>   DROP TABLE t1;
>   SET GLOBAL myisam_use_mmap=default;
>   End of 5.1 tests
> +#
> +# BUG#55385: UPDATE statement throws an error, but still updates
> +#            the table entries
> +CREATE TABLE t1_part (
> +col1 int,
> +col2 int
> +) PARTITION BY LINEAR HASH(col1) PARTITIONS 3;
> +INSERT INTO t1_part VALUES (1, 1) , (10, 10);
> +CREATE VIEW v1 AS SELECT * FROM t1_part;

Using other names than col1, col2 would make the results easier to read.
E.g partkey or similar.

> === modified file 'sql/handler.cc'
> --- a/sql/handler.cc	2011-02-08 15:54:12 +0000
> +++ b/sql/handler.cc	2011-02-14 14:51:47 +0000
> @@ -2879,6 +2879,7 @@ void handler::print_error(int error, myf
>       break;
>     case HA_ERR_KEY_NOT_FOUND:
>     case HA_ERR_NO_ACTIVE_RECORD:
> +  case HA_ERR_RECORD_DELETED:
>     case HA_ERR_END_OF_FILE:
>       textno=ER_KEY_NOT_FOUND;
>       break;

Is this change really needed anymore?

> === modified file 'sql/share/errmsg-utf8.txt'
> --- a/sql/share/errmsg-utf8.txt	2011-02-08 17:48:20 +0000
> +++ b/sql/share/errmsg-utf8.txt	2011-02-14 14:51:47 +0000
> @@ -6454,3 +6454,6 @@ ER_STMT_CACHE_FULL
>           eng "Multi-row statements required more than 'max_binlog_stmt_cache_size'
> bytes of storage; increase this mysqld variable and try again"
>   ER_BINLOG_STMT_CACHE_SIZE_GREATER_THAN_MAX
>     eng "Option binlog_stmt_cache_size (%lu) is greater than
> max_binlog_stmt_cache_size (%lu); setting binlog_stmt_cache_size equal to
> max_binlog_stmt_cache_size."
> +
> +ER_MULTI_UPDATE_KEY_CONFLICT
> +  eng "Primary key/partition key update is not allowed since the table is updated
> both as '%-.192s' and '%-.192s'."

Maybe replace allowed with supported?
Also, I think it would be good to check with #docs regarding the error 
message text.

> === modified file 'sql/sql_update.cc'
> --- a/sql/sql_update.cc	2010-12-29 00:38:59 +0000
> +++ b/sql/sql_update.cc	2011-02-14 14:51:47 +0000

General comment: Several lines here have trailing whitespace. Please 
remove them.

Otherwise the patch looks good.

Thanks for the patience with these bugs :-)

--- Jon Olav
Thread
bzr commit into mysql-trunk branch (jorgen.loland:3641) Bug#11762751Bug#11764529Jorgen Loland14 Feb
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3641) Bug#11762751Bug#11764529Jon Olav Hauglid15 Feb
    • Re: bzr commit into mysql-trunk branch (jorgen.loland:3641) Bug#11762751Bug#11764529Jorgen Loland15 Feb
      • Re: bzr commit into mysql-trunk branch (jorgen.loland:3641) Bug#11762751Bug#11764529Jon Olav Hauglid15 Feb
        • Re: bzr commit into mysql-trunk branch (jorgen.loland:3641) Bug#11762751Bug#11764529Jorgen Loland17 Feb
          • Re: bzr commit into mysql-trunk branch (jorgen.loland:3641) Bug#11762751Bug#11764529MATTIAS JONSSON17 Feb
            • Re: bzr commit into mysql-trunk branch (jorgen.loland:3641) Bug#11762751Bug#11764529Jorgen Loland17 Feb