List:Commits« Previous MessageNext Message »
From:Jon Olav Hauglid Date:November 11 2010 2:19pm
Subject:Re: bzr commit into mysql-5.5-bugteam branch (jorgen.loland:3120)
Bug#54812
View as plain text  
Hello,

Great to get rid of this annoying bug for RQG testing :-)

Comments about the patch in-lined.

On 11/11/2010 01:06 PM, Jorgen Loland wrote:
> #At file:///export/home/jl208045/mysql/mysql-5.5-bugteam-54812/ based on
> revid:svoj@stripped
>
>   3120 Jorgen Loland	2010-11-11
>        Bug#54812: assert in Diagnostics_area::set_ok_status
>                   during EXPLAIN
>
>        Before the patch, send_eof() of some subclasses of
>        select_result (e.g., select_send::send_eof()) could
>        handle being called after an error had occured while others
>        could not.

I think you could clarify the consequences of not handling
being called after an error. For example, you could mention that it
would cause an assert on debug builds and that release builds would
be unaffected.

>        In the bug, an ASSERT triggered because
>        select_dumpvar::send_eof() was not well behaved after an
>        error had occured, but the problem also applied to other
>        subclasses of select_result. This patch uniforms send_eof()
>        of all subclasses of select_result to handle being called
>        after an error has occured.

Did you consider multi_delete::send_eof() and
Select_fetch_protocol_binary::send_eof()?

>       @ mysql-test/r/errors.result
>          Add test for BUG#54812
>       @ mysql-test/t/errors.test
>          Add test for BUG#54812

I don't think you should use errors.test for the test case.
Since you use privileges, it won't work with embedded server.

> === modified file 'mysql-test/t/errors.test'
> --- a/mysql-test/t/errors.test	2010-11-04 12:36:36 +0000
> +++ b/mysql-test/t/errors.test	2010-11-11 12:06:45 +0000

First of all, kudos for finding a non-concurrent and simple
test case for this bug :-)

I have some (mostly minor) suggestions for the test case based on how we
do it in runtime. Some may be only due to different styles, feel free to
disagree.

> @@ -171,3 +171,47 @@ SELECT UPDATEXML(-73 * -2465717823867977
>   --echo #
>   --echo # End Bug#57882
>   --echo #
> +--echo # Bug#54812: assert in Diagnostics_area::set_ok_status during EXPLAIN
> +--echo #

I suggest a bit of whitespace between the test cases.

I also suggest doing DROP .. IF EXISTS for the tables/function you will
use, enclosed in --disable-warnings / --enable-warnings to prevent your
test case from being affected in case a test case above start failing.

> +--error 1370
> +SELECT MAX(key1) FROM t1 WHERE f()<  1 INTO OUTFILE 'mytest';
> +
> +--error 1370
> +INSERT INTO t2 SELECT MAX(key1) FROM t1 WHERE f()<  1;
> +
> +--error 1370
> +SELECT MAX(key1) INTO @dummy FROM t1 WHERE f()<  1;
> +
> +--error 1370
> +CREATE TABLE t3 (i INT) AS SELECT MAX(key1) FROM t1 WHERE f()<  1;

For the errors, I suggest replacing the error number with the symbol.
E.g. ER_PROCACCESS_DENIED_ERROR

I also suggest disconnecting con1 to return to the state before your
test started. We tend to use:
   connection con1;
   disconnect con1;
   --source include/wait_until_disconnected.inc

> === modified file 'sql/sql_class.cc'
> === modified file 'sql/sql_insert.cc'
> === modified file 'sql/sql_update.cc'

The code changes look fine.

Thanks,

--- Jon Olav
Thread
bzr commit into mysql-5.5-bugteam branch (jorgen.loland:3120) Bug#54812Jorgen Loland11 Nov
  • Re: bzr commit into mysql-5.5-bugteam branch (jorgen.loland:3120)Bug#54812Jon Olav Hauglid11 Nov
    • Re: bzr commit into mysql-5.5-bugteam branch (jorgen.loland:3120)Bug#54812Jorgen Loland11 Nov