From: Roy Lyseng
Date: December 13 2010 2:32pm
Subject: Re: bzr commit into mysql-5.5-bugteam branch (jon.hauglid:3184) Bug#58730
List-Archive: http://lists.mysql.com/commits/126649
Message-Id: <4D062E7C.4070509@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Jon Olav,

thank you for a nice patch and great comment.

Patch approved, but please see some review comments below.

On 10.12.10 15.59, Jon Olav Hauglid wrote:
> #At file:///export/home/x/mysql-5.5-bugteam-bug58730/ based on revid:dmitry.shulga@stripped
>
>   3184 Jon Olav Hauglid	2010-12-10
>        Bug #58730 Assertion failed: table->key_read == 0 in close_thread_table,
>                   temptable views
>
>        The TABLE::key_read field indicates if the optimizer has found that row
>        retrieval only should access the index tree. The triggered assert
>        inside close_thread_table() checks that this field has been reset when
>        the table is about to be closed.
>
>        During normal execution, these fields are reset right before tables are
>        closed at the end of mysql_execute_command(). But in the case of errors,
>        tables are closed earlier. The patch for Bug#52044 refactored the open
>        tables code so that close_thread_tables() is called immediately if
>        opening of tables fails. At this point in the execution, it could
>        happend that all TABLE::key_read fields had not been properly reset,
>        therefore triggering the assert.
>
>        The problematic statement in this case was EXPLAIN where the query
>        accessed two derived tables and where the first derived table was
>        processed successfully while the second derived table was not.
>        Since it was an EXPLAIN, TABLE::key_read fields were not reset after
>        successful derived table processing since the state needs to be
>        accessible afterwards. When processing of the second derived table
>        failed, it's corresponding SELECT_LEX_UNIT was cleaned, which caused
>        it's TABLE::key_read fields to be reset. Since processing failed,
>        the error path of open_and_lock_tables() was entered and
>        close_thread_tables() was called. The assert was then triggered due
>        to the TABLE::key_read fields set during processing of the first
>        derived table.
>
>        This patch fixes the problem by adding a new derived table processor,
>        mysql_derived_cleanup() that is called after mysql_derived_filling().
>        It causes cleanup of all SELECT_LEX_UNITs to be called, resetting
>        all relevant TABLE::key_read fields.
>
>        Test case added to derived.test.
>
>      modified:
>        mysql-test/r/derived.result
>        mysql-test/t/derived.test
>        sql/sql_base.cc
>        sql/sql_derived.cc
>        sql/sql_derived.h
>        sql/sql_update.cc
> === modified file 'mysql-test/r/derived.result'
> --- a/mysql-test/r/derived.result	2009-11-10 18:48:46 +0000
> +++ b/mysql-test/r/derived.result	2010-12-10 14:59:14 +0000
> @@ -401,3 +401,19 @@ SELECT 0 FROM
>   0
>   0
>   # End of 5.0 tests
> +#
> +# Bug#58730 Assertion failed: table->key_read == 0 in close_thread_table,
> +#           temptable views
> +#
> +DROP TABLE IF EXISTS t1, t2, t3;
> +DROP VIEW IF EXISTS v1, v2;

Dropping tables and views up-front is not necessary.

> +CREATE TABLE t1 (a INT);
> +CREATE TABLE t2 (b INT, KEY (b)) engine=innodb;

I think that you can add two rows to table t2 and remove the innodb dependency, 
and still have the error.

> +INSERT INTO t1 VALUES (1),(1);
> +CREATE algorithm=temptable VIEW v1 AS
> +SELECT 1 FROM t1 LEFT JOIN t1 t3 ON 1>  (SELECT 1 FROM t1);
> +CREATE algorithm=temptable VIEW v2 AS SELECT 1 FROM t2;
> +EXPLAIN SELECT 1 FROM t1 JOIN v1 ON 1>  (SELECT 1 FROM v2);
> +ERROR 21000: Subquery returns more than 1 row
> +DROP TABLE t1, t2;
> +DROP VIEW v1, v2;
>
> === modified file 'mysql-test/t/derived.test'
> --- a/mysql-test/t/derived.test	2009-11-10 18:48:46 +0000
> +++ b/mysql-test/t/derived.test	2010-12-10 14:59:14 +0000
> @@ -302,3 +302,29 @@ SELECT 0 FROM
>   (SELECT 0) t61; # 61 == MAX_TABLES
>
>   --echo # End of 5.0 tests
> +
> +
> +--echo #
> +--echo # Bug#58730 Assertion failed: table->key_read == 0 in close_thread_table,
> +--echo #           temptable views
> +--echo #
> +
> +--disable_warnings
> +DROP TABLE IF EXISTS t1, t2, t3;
> +DROP VIEW IF EXISTS v1, v2;
> +--enable_warnings
> +
> +CREATE TABLE t1 (a INT);
> +CREATE TABLE t2 (b INT, KEY (b)) engine=innodb;
> +INSERT INTO t1 VALUES (1),(1);
> +
> +CREATE algorithm=temptable VIEW v1 AS
> +  SELECT 1 FROM t1 LEFT JOIN t1 t3 ON 1>  (SELECT 1 FROM t1);
> +CREATE algorithm=temptable VIEW v2 AS SELECT 1 FROM t2;
> +
> +# This caused the assert to be triggered.
> +--error ER_SUBQUERY_NO_1_ROW
> +EXPLAIN SELECT 1 FROM t1 JOIN v1 ON 1>  (SELECT 1 FROM v2);
> +
> +DROP TABLE t1, t2;
> +DROP VIEW v1, v2;
>
> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc	2010-11-11 17:11:05 +0000
> +++ b/sql/sql_base.cc	2010-12-10 14:59:14 +0000
> @@ -5408,11 +5408,19 @@ bool open_and_lock_tables(THD *thd, TABL
>     if (lock_tables(thd, tables, counter, flags))
>       goto err;
>
> -  if (derived&&
> -      (mysql_handle_derived(thd->lex,&mysql_derived_prepare) ||
> -       (thd->fill_derived_tables()&&
> -        mysql_handle_derived(thd->lex,&mysql_derived_filling))))
> -    goto err;
> +  if (derived)
> +  {
> +    if (mysql_handle_derived(thd->lex,&mysql_derived_prepare))
> +      goto err;
> +    if (thd->fill_derived_tables()&&
> +        mysql_handle_derived(thd->lex,&mysql_derived_filling))
> +    {
> +      mysql_handle_derived(thd->lex,&mysql_derived_cleanup);
> +      goto err;
> +    }
> +    if (!thd->lex->describe)
> +      mysql_handle_derived(thd->lex,&mysql_derived_cleanup);
> +  }
>
>     DBUG_RETURN(FALSE);
>   err:
>
> === modified file 'sql/sql_derived.cc'
> --- a/sql/sql_derived.cc	2010-06-11 01:30:49 +0000
> +++ b/sql/sql_derived.cc	2010-12-10 14:59:14 +0000
> @@ -306,13 +306,21 @@ bool mysql_derived_filling(THD *thd, LEX
>         */
>         if (derived_result->flush())
>           res= TRUE;
> -
> -      if (!lex->describe)
> -        unit->cleanup();
>       }
> -    else
> -      unit->cleanup();
>       lex->current_select= save_current_select;
>     }
>     return res;
>   }
> +
> +
> +/**
> +   Cleans up the SELECT_LEX_UNIT for the derived table (if any).
> +*/
> +
> +bool mysql_derived_cleanup(THD *thd, LEX *lex, TABLE_LIST *table_list)
> +{
> +  SELECT_LEX_UNIT *unit= table_list->derived;
> +  if (unit)
> +    unit->cleanup();
> +  return false;
> +}
>
> === modified file 'sql/sql_derived.h'
> --- a/sql/sql_derived.h	2010-04-12 13:17:37 +0000
> +++ b/sql/sql_derived.h	2010-12-10 14:59:14 +0000
> @@ -26,4 +26,16 @@ bool mysql_handle_derived(LEX *lex, bool
>   bool mysql_derived_prepare(THD *thd, LEX *lex, TABLE_LIST *t);
>   bool mysql_derived_filling(THD *thd, LEX *lex, TABLE_LIST *t);
>
> +/**
> +   Cleans up the SELECT_LEX_UNIT for the derived table (if any).
> +
> +   @param  thd         Thread handler
> +   @param  db          LEX for this thread

db -> lex
> +   @param  table_list  TABLE_LIST for the upper SELECT

Please replace comment: TABLE_LIST for the derived table
> +
> +   @retval  false  Success
> +   @retval  true   Failure
> +*/
> +bool mysql_derived_cleanup(THD *thd, LEX *lex, TABLE_LIST *table_list);

Please replace "TABLE_LIST *table_list" with "TABLE_LIST *derived"

The two comments above will align your patch with the forthcoming WL5274 (and 
even make sense in this context).

> +
>   #endif /* SQL_DERIVED_INCLUDED */
>
> === modified file 'sql/sql_update.cc'
> --- a/sql/sql_update.cc	2010-11-16 09:05:19 +0000
> +++ b/sql/sql_update.cc	2010-12-10 14:59:14 +0000
> @@ -296,11 +296,17 @@ int mysql_update(THD *thd,
>     if (lock_tables(thd, table_list, table_count, 0))
>       DBUG_RETURN(1);
>
> -  if (mysql_handle_derived(thd->lex,&mysql_derived_prepare) ||
> -      (thd->fill_derived_tables()&&
> -       mysql_handle_derived(thd->lex,&mysql_derived_filling)))
> +  if (mysql_handle_derived(thd->lex,&mysql_derived_prepare))
>       DBUG_RETURN(1);
>
> +  if (thd->fill_derived_tables()&&
> +      mysql_handle_derived(thd->lex,&mysql_derived_filling))
> +  {
> +    mysql_handle_derived(thd->lex,&mysql_derived_cleanup);
> +    DBUG_RETURN(1);
> +  }
> +  mysql_handle_derived(thd->lex,&mysql_derived_cleanup);
> +
>     thd_proc_info(thd, "init");
>     table= table_list->table;
>
> @@ -1194,7 +1200,11 @@ int mysql_multi_update_prepare(THD *thd)
>
>     if (thd->fill_derived_tables()&&
>         mysql_handle_derived(lex,&mysql_derived_filling))
> +  {
> +    mysql_handle_derived(lex,&mysql_derived_cleanup);
>       DBUG_RETURN(TRUE);
> +  }
> +  mysql_handle_derived(lex,&mysql_derived_cleanup);
>
>     DBUG_RETURN (FALSE);
>   }

Thanks,
Roy