List:Commits« Previous MessageNext Message »
From:Alexey Kopytov Date:August 31 2010 3:07pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3497)
Bug#54543
View as plain text  
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.
Thread
bzr commit into mysql-5.1-bugteam branch (martin.hansson:3497) Bug#54543Martin Hansson31 Aug
  • Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3497)Bug#54543Alexey Kopytov31 Aug
    • Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3497)Bug#54543Martin Hansson1 Sep