List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:September 1 2010 7:18am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (martin.hansson:3497)
Bug#54543
View as plain text  
  Alexey Kopytov skrev 2010-08-31 17.07:
> 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.
Done.
>
> 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;
The reported bug was that way, but I changed it.

When I make a test case, I try to make the test itself as simple as 
possible. I try to make all joins consist of tables that are numbered in 
the order they are accessed. In this test case you can trust that first 
t1 is accessed, then t2, then t3. With only t1, it would be less clear. 
In your test you have to introduce aliases to achieve that. But this is 
only a discussion of taste, really. What is more serious is that when I 
see a table joined to itself, I assume that the bug happens only for 
self-joins.
>
> 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).
Done.
>
>> +
>> +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:
Yes, it looks much better. Thank you.

Thank you for the review.

Kind Regards

Martin
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