Hi Martin,
Below are some comments on the patch.
On 31.08.10 12:41, Martin Hansson wrote:
> #At file:///data0/martin/bzr/bug54543/5.1bt-commit/ based on
> revid:gshchepa@stripped
>
> 3497 Martin Hansson 2010-08-31
> Bug#54543: update ignore with incorrect subquery leads to assertion failure:
> inited==INDEX
>
> When an error occurs while sending the data in a temporary table there was no
> cleanup performed. This caused a failed assertion in the case when different
> access methods were used for populating the table vs. retrieving the data
> from
> the table if IGNORE was specified and sql_safe_updates = 0. In this case
> execution continues, but the handler expects to continue with the access
> method used for row retrieval.
>
> Fixed by doing the cleanup even if errors occur.
>
> modified:
> mysql-test/r/multi_update.result
> mysql-test/t/multi_update.test
> sql/sql_select.cc
> === modified file 'mysql-test/r/multi_update.result'
> --- a/mysql-test/r/multi_update.result 2010-05-24 13:54:08 +0000
> +++ b/mysql-test/r/multi_update.result 2010-08-31 08:41:02 +0000
> @@ -639,4 +639,25 @@ SET SESSION sql_safe_updates = 1;
> UPDATE IGNORE t1, t1 t1a SET t1.a = 1 WHERE t1a.a = 1;
> ERROR HY000: You are using safe update mode and you tried to update a table without
> a WHERE that uses a KEY column
> DROP TABLE t1;
> +#
> +# update ignore with incorrect subquery leads to assertion failure:
> +# inited==INDEX
> +#
> +SET SESSION sql_safe_updates = 0;
> +CREATE TABLE t1 ( a INT );
> +INSERT INTO t1 VALUES (1), (2);
> +CREATE TABLE t2 ( a INT );
> +INSERT INTO t2 VALUES (1), (2);
> +CREATE TABLE t3 ( a INT );
> +INSERT INTO t3 VALUES (1), (2);
> +SET @v = 1;
> +# Should not crash
> +UPDATE IGNORE
> +( SELECT ( SELECT COUNT(*) FROM t1 GROUP BY a, @v ) a FROM t2 ) x, t3
> +SET t3.a = 0;
> +Warnings:
> +Error 1242 Subquery returns more than 1 row
> +Error 1242 Subquery returns more than 1 row
> +DROP TABLE t1, t2, t3;
> +SET SESSION sql_safe_updates = DEFAULT;
> end of tests
>
> === modified file 'mysql-test/t/multi_update.test'
> --- a/mysql-test/t/multi_update.test 2010-05-24 13:54:08 +0000
> +++ b/mysql-test/t/multi_update.test 2010-08-31 08:41:02 +0000
> @@ -651,5 +651,28 @@ SET SESSION sql_safe_updates = 1;
> UPDATE IGNORE t1, t1 t1a SET t1.a = 1 WHERE t1a.a = 1;
> DROP TABLE t1;
>
> +--echo #
> +--echo # update ignore with incorrect subquery leads to assertion failure:
> +--echo # inited==INDEX
> +--echo #
Please add the bug # to the test case comments.
And while you are at it, please consider compacting the test cast as
described below.
> +SET SESSION sql_safe_updates = 0;
> +CREATE TABLE t1 ( a INT );
> +INSERT INTO t1 VALUES (1), (2);
> +
> +CREATE TABLE t2 ( a INT );
> +INSERT INTO t2 VALUES (1), (2);
> +
> +CREATE TABLE t3 ( a INT );
> +INSERT INTO t3 VALUES (1), (2);
> +
> +SET @v = 1;
> +
> +--echo # Should not crash
> +UPDATE IGNORE
> + ( SELECT ( SELECT COUNT(*) FROM t1 GROUP BY a, @v ) a FROM t2 ) x, t3
> +SET t3.a = 0;
You can actually go by with just 1 table:
UPDATE IGNORE
( SELECT ( SELECT COUNT(*) FROM t1 GROUP BY a, @v ) a FROM t1 t2 ) x, t1
t3 SET t3.a = 0;
And you don't need to initialize the variable, the test case has the
same effect when used with uninitialized @v (i.e. having NULL value).
> +
> +DROP TABLE t1, t2, t3;
> +SET SESSION sql_safe_updates = DEFAULT;
>
> --echo end of tests
>
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc 2010-08-26 09:31:04 +0000
> +++ b/sql/sql_select.cc 2010-08-31 08:41:02 +0000
> @@ -11080,6 +11080,11 @@ Next_select_func setup_end_select_func(J
> /**
> Make a join of all tables and write it on socket or to table.
>
> + @param table Temporary table for GROUP BY, may be null. If the table is
> + specified, aggregates are used with GROUP BY and an index is used to
> + retrieve rows, the handler is initialized for index read here. The query
> + block is also set to reference the temporary table.
> +
> @retval
> 0 if ok
> @retval
> @@ -11176,7 +11181,12 @@ do_select(JOIN *join,List<Item> *fields,
> DBUG_PRINT("info",("%ld records output", (long) join->send_records));
> }
> else
> + {
> rc= -1;
> + if (table == NULL)
> + join->join_free();
> + }
> +
Since we now have common code in both if..else branches, the following
equivalent patch results in more compact and readable code:
=== modified file 'sql/sql_select.cc'
--- sql/sql_select.cc 2010-07-09 10:39:47 +0000
+++ sql/sql_select.cc 2010-08-31 14:53:26 +0000
@@ -11155,22 +11155,21 @@
if (error == NESTED_LOOP_NO_MORE_ROWS)
error= NESTED_LOOP_OK;
+
+ /*
+ The following will unlock all cursors if the command wasn't an
+ update command
+ */
+ if (table == NULL)
+ join->join_free(); // Unlock all cursors
if (error == NESTED_LOOP_OK)
{
/*
Sic: this branch works even if rc != 0, e.g. when
send_data above returns an error.
*/
- if (!table) // If sending data to client
- {
- /*
- The following will unlock all cursors if the command wasn't an
- update command
- */
- join->join_free(); // Unlock all cursors
- if (join->result->send_eof())
- rc= 1; // Don't send error
- }
+ if (table == NULL && join->result->send_eof()) // If sending data
to client
+ rc= 1; // Don't send error
DBUG_PRINT("info",("%ld records output", (long) join->send_records));
}
else
Best regards,
Alexey.
