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);
> }
>
>
>
> ------------------------------------------------------------------------
>
>
