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
