List:Commits« Previous MessageNext Message »
From:Olav Sandstaa Date:November 8 2010 10:20pm
Subject:Re: bzr commit into mysql-5.5-bugteam branch (oystein.grovlen:3258)
Bug#57704
View as plain text  
Hi Oystein,

I think the patch looks fine and I have only two issues that I think you 
should consider:

Oystein Grovlen wrote:
> #At file:///home/og136792/mysql/mysql-5.5-bugteam/ based on
> revid:davi.arnaut@stripped
>
>  3258 Oystein Grovlen	2010-11-04
>       Bug#57704 Cleanup code dies with void TABLE::set_keyread(bool): Assertion
> `file' failed.
>       
>       This bug was introduced in this revision: 
>       kostja@stripped
>       ("A pre-requisite patch for the fix for Bug#52044.")
>       
>       It happens because close_thread_tables() is now called in
>       open_and_lock_tables upon failure.  Hence, table is no longer
>       open when optimizer tries to do cleanup.
>       
>       Fix: Make sure to do cleanup in st_select_lex_unit::prepare()
>       upon failure. This way, cleanup() is called before tables are
>       released.
>      @ mysql-test/r/subselect.result
>         Added test case for Bug#57704.
>      @ mysql-test/t/subselect.test
>         Added test case for Bug#57704.
>      @ sql/sql_union.cc
>         st_select_lex_unit::prepare():  On failure, make sure cleanup()
>         is called.
>
>     modified:
>       mysql-test/r/subselect.result
>       mysql-test/t/subselect.test
>       sql/sql_union.cc
> === modified file 'mysql-test/r/subselect.result'
> --- a/mysql-test/r/subselect.result	2010-09-09 15:00:33 +0000
> +++ b/mysql-test/r/subselect.result	2010-11-04 10:36:24 +0000
> @@ -5005,3 +5005,15 @@ SELECT * FROM t2 UNION SELECT * FROM t2
>  ORDER BY (SELECT * FROM t1 WHERE MATCH(a) AGAINST ('+abc' IN BOOLEAN MODE));
>  DROP TABLE t1,t2;
>  End of 5.1 tests
> +#
> +# Bug #57704: Cleanup code dies with void TABLE::set_keyread(bool): 
> +#             Assertion `file' failed.
> +#
> +CREATE TABLE t1 (a INT);
> +SELECT 1 FROM 
> +(SELECT ROW(
> +(SELECT 1 FROM t1 RIGHT JOIN 
> +(SELECT 1 FROM t1, t1 t2) AS d ON 1),
> +1) FROM t1) AS e;
> +ERROR 21000: Operand should contain 1 column(s)
> +DROP TABLE t1;
>
> === modified file 'mysql-test/t/subselect.test'
> --- a/mysql-test/t/subselect.test	2010-06-25 13:32:47 +0000
> +++ b/mysql-test/t/subselect.test	2010-11-04 10:36:24 +0000
> @@ -3946,3 +3946,21 @@ DROP TABLE t1,t2;
>  --enable_result_log
>  
>  --echo End of 5.1 tests
> +
> +--echo #
> +--echo # Bug #57704: Cleanup code dies with void TABLE::set_keyread(bool): 
> +--echo #             Assertion `file' failed.
> +--echo #
> +
> +CREATE TABLE t1 (a INT);
> +
> +--error 1241
>   

1. Instead of using the error number here would it be better to use the 
error name? (In this case ER_OPERAND_COLUMNS).

> +SELECT 1 FROM 
> +  (SELECT ROW(
> +    (SELECT 1 FROM t1 RIGHT JOIN 
> +      (SELECT 1 FROM t1, t1 t2) AS d ON 1),
> +    1) FROM t1) AS e;
> +
> +DROP TABLE t1;
> +
> +
>
> === modified file 'sql/sql_union.cc'
> --- a/sql/sql_union.cc	2010-07-27 12:42:36 +0000
> +++ b/sql/sql_union.cc	2010-11-04 10:36:24 +0000
> @@ -443,6 +443,7 @@ bool st_select_lex_unit::prepare(THD *th
>  
>  err:
>    thd_arg->lex->current_select= lex_select_save;
> +  cleanup();
>   

2. I replaced your call to cleanup() with the following code (due to 
that I considered adding "always check return values" to your "avoiding 
regressions" list) :

  bool xxx= cleanup();
  DBUG_ASSERT(xxx == false);

and with this change I hit this assert when running the subselect test 
(not in your test case but an earlier test in the same file).
Given that we already have run into an "error situation" and are trying 
to clean up after it it might be expected that cleanup() will not always 
return without error. Still, I think it is worth consider if this is the 
case or not (I do not know).

If you think this is fine, then it is OK to push this patch.

Best regards,
Olav


>    DBUG_RETURN(TRUE);
>  }
>  
>
>   
> ------------------------------------------------------------------------
>
>

Thread
bzr commit into mysql-5.5-bugteam branch (oystein.grovlen:3258) Bug#57704Oystein Grovlen4 Nov
Re: bzr commit into mysql-5.5-bugteam branch (oystein.grovlen:3258)Bug#57704Olav Sandstaa8 Nov
  • Re: bzr commit into mysql-5.5-bugteam branch (oystein.grovlen:3258)Bug#57704Øystein Grøvlen10 Nov