List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:December 13 2010 9:39am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (sergey.glukhov:3514)
Bug#52157 Bug#54475 Bug#57352 Bug#57703
View as plain text  
Hi Sergey,

On 12/7/10 12:33 PM, Sergey Glukhov wrote:
> #At file:///home/gluh/MySQL/mysql-5.1-bugteam-new/ based on
> revid:sergey.glukhov@stripped
>
>   3514 Sergey Glukhov	2010-12-07
>        Fixed following problems:
>        --Bug#52157 various crashes and assertions with multi-table update, stored
> function
>        --Bug#54475 improper error handling causes cascading crashing failures in
> innodb/ndb
>        --Bug#57703 create view cause Assertion failed: 0, file .\item_subselect.cc,
> line 846
>        --Bug#57352 valgrind warnings when creating view
>        --Recently discovered problem when a nested materialized derived table is
> used
>          before being populated and it leads to incorrect result
>
>        We have several modes when we should disable subquery evaluation.
>        The reasons for disabling are different. It could be
>        uselessness of the evaluation as in case of 'CREATE VIEW'
>        or 'PREPARE stmt', or we should disable subquery evaluation
>        if tables are not locked yet as it happens in bug#54475, or
>        too early evaluation of subqueries can lead to wrong result
>        as it happened in Bug#19077.
>        Main problem is that if subquery items are treated as const
>        they are evaluated in ::fix_fields(), ::fix_length_and_dec()
>        of the parental items as a lot of these methods have
>        Item::val_...() calls inside.
>        We have to make subqueries non-const to prevent unnecessary
>        subquery evaluation. At the moment we have different methods
>        for this. Here is a list of these modes:
>
>        1. PREPARE stmt;
>        We use UNCACHEABLE_PREPARE flag.
>        It is set during parsing in sql_parse.cc, mysql_new_select() for
>        each SELECT_LEX object and cleared at the end of PREPARE in
>        sql_prepare.cc, init_stmt_after_parse(). If this flag is set
>        subquery becomes non-const and evaluation does not happen.
>
>        2. CREATE|ALTER VIEW, SHOW CREATE VIEW, I_S tables which
>           process FRM files
>        We use LEX::view_prepare_mode field. We set it before
>        view preparation and check this flag in
>        ::fix_fields(), ::fix_length_and_dec().
>        Some of bugs are fixed using this approach,

"of" is not needed.

>        some are not(Bug#57352, Bug#57703). The problem here is.

Stray dot above: "The problem here is."

>        that we have a lot of ::fix_fields(), ::fix_length_and_dec()
>        where we use Item::val_...() calls for const items.
>
>        3. Derived tables with subquery = wrong result(Bug19077)
>        The reason of this bug is too early subquery evaluation.
>        It's fixed by adding Item::with_subselect field

I think you mean: "It was fixed by".

>        The check of this field in appropriate places prevents
>        const item evaluation if the item have subquery.
>        The fix for Bug19077 fixes only the problem with.

Stray dot above.

>        convert_constant_item() function and does not cover
>        other places(::fix_fields(), ::fix_length_and_dec() again)
>        where subqueries could be evaluated.
>
>        Example:
>        CREATE TABLE t1 (i INT, j BIGINT);
>        INSERT INTO t1 VALUES (1, 2), (2, 2), (3, 2);
>        SELECT * FROM (SELECT MIN(i) FROM t1
>        WHERE j = SUBSTRING('12', (SELECT * FROM (SELECT MIN(j) FROM t1) t2))) t3;
>        DROP TABLE t1;
>
>        4. Derived tables with subquery where subquery
>           is evaluated before table locking(Bug#54475, Bug#52157)
>
>        Suggested solution is following:
>
>        -Introduce new field LEX::context_analysis_only with the following
>         possible flags:
>         #define CONTEXT_ANALYSIS_ONLY_PREPARE 1
>         #define CONTEXT_ANALYSIS_ONLY_VIEW    2
>         #define CONTEXT_ANALYSIS_ONLY_DERIVED 4
>        -Set/clean these flags when we perform
>         context analysis operation
>        -Item_subselect::const_item() returns
>         result depending on LEX::context_analysis_only.
>         If context_analysis_only is set then we return
>         FALSE that means that subquery is non-const.
>         As all subquery types are wrapped by Item_subselect
>         it allow as to make subquery non-const when
>         it's necessary.

Some comments below.

[..]

> === modified file 'sql/item.cc'
> --- a/sql/item.cc	2010-11-18 13:11:18 +0000
> +++ b/sql/item.cc	2010-12-07 14:32:55 +0000
> @@ -1712,16 +1712,7 @@ bool agg_item_set_converter(DTCollation
>
>       if (!(conv= (*arg)->safe_charset_converter(coll.collation))&&
>           ((*arg)->collation.repertoire == MY_REPERTOIRE_ASCII))
> -    {
> -      /*
> -        We should disable const subselect item evaluation because
> -        subselect transformation does not happen in view_prepare_mode
> -        and thus val_...() methods can not be called for const items.
> -      */
> -      bool resolve_const= ((*arg)->type() == Item::SUBSELECT_ITEM&&
> -                           thd->lex->view_prepare_mode) ? FALSE : TRUE;
> -      conv= new Item_func_conv_charset(*arg, coll.collation, resolve_const);
> -    }
> +      conv= new Item_func_conv_charset(*arg, coll.collation, 1);

The third parameter is now always constant.
More unnecessary code that you can remove.

>       if (!conv)
>       {
>
> === modified file 'sql/item_cmpfunc.cc'
> --- a/sql/item_cmpfunc.cc	2010-09-09 12:48:06 +0000
> +++ b/sql/item_cmpfunc.cc	2010-12-07 14:32:55 +0000
> @@ -401,7 +401,7 @@ static bool convert_constant_item(THD *t
>     Field *field= field_item->field;
>     int result= 0;
>
> -  if (!(*item)->with_subselect&&  (*item)->const_item())
> +  if ((*item)->const_item())
>     {
>       TABLE *table= field->table;
>       ulong orig_sql_mode= thd->variables.sql_mode;
> @@ -497,7 +497,9 @@ void Item_bool_func2::fix_length_and_dec
>     }
>
>     thd= current_thd;
> -  if (!thd->is_context_analysis_only())
> +  if (!(thd->lex->context_analysis_only&
> +        (CONTEXT_ANALYSIS_ONLY_PREPARE |
> +         CONTEXT_ANALYSIS_ONLY_VIEW)))

Let's still use a wrapper function, I suggest we simply rename 
context_analysis_only() to is_ps_or_view_context_analysis().

[..]

> === modified file 'sql/item_subselect.cc'
> --- a/sql/item_subselect.cc	2010-10-18 12:12:27 +0000
> +++ b/sql/item_subselect.cc	2010-12-07 14:32:55 +0000
> @@ -123,20 +123,6 @@ void Item_subselect::cleanup()
>   }
>
>
> -/*
> -   We cannot use generic Item::safe_charset_converter() because
> -   Subselect transformation does not happen in view_prepare_mode
> -   and thus we can not evaluate val_...() for const items.
> -*/
> -
> -Item *Item_subselect::safe_charset_converter(CHARSET_INFO *tocs)
> -{
> -  Item_func_conv_charset *conv=
> -    new Item_func_conv_charset(this, tocs, thd->lex->view_prepare_mode ? 0 :
> 1);
> -  return conv->safe ? conv : NULL;
> -}
> -
> -
>   void Item_singlerow_subselect::cleanup()
>   {
>     DBUG_ENTER("Item_singlerow_subselect::cleanup");
> @@ -271,6 +257,7 @@ bool Item_subselect::exec()
>     if (thd->is_error() || thd->killed)
>       return 1;
>
> +  DBUG_ASSERT(!thd->lex->context_analysis_only);
>     /*
>       Simulate a failure in sub-query execution. Used to test e.g.
>       out of memory or query being killed conditions.
> @@ -307,6 +294,8 @@ table_map Item_subselect::used_tables()
>
>   bool Item_subselect::const_item() const
>   {
> +  if (thd->lex->context_analysis_only)
> +    return FALSE;
>     return const_item_cache;

Suggestion:

     return thd->lex->context_analysis_only ? FALSE : const_item_cache;

>   }
>
> @@ -1638,7 +1627,8 @@ bool Item_in_subselect::fix_fields(THD *
>   {
>     bool result = 0;
>
> -  if (thd_arg->lex->view_prepare_mode&&  left_expr&& 
> !left_expr->fixed)
> +  if ((thd_arg->lex->context_analysis_only& 
> CONTEXT_ANALYSIS_ONLY_VIEW)&&
> +      left_expr&&  !left_expr->fixed)
>       result = left_expr->fix_fields(thd_arg,&left_expr);
>
>     return result || Item_subselect::fix_fields(thd_arg, ref);
>
> === modified file 'sql/item_subselect.h'
> --- a/sql/item_subselect.h	2010-04-06 07:26:59 +0000
> +++ b/sql/item_subselect.h	2010-12-07 14:32:55 +0000
> @@ -126,7 +126,6 @@ public:
>     virtual void reset_value_registration() {}
>     enum_parsing_place place() { return parsing_place; }
>     bool walk(Item_processor processor, bool walk_subquery, uchar *arg);
> -  Item *safe_charset_converter(CHARSET_INFO *tocs);
>
>     /**
>       Get the SELECT_LEX structure associated with this Item.
>
> === modified file 'sql/mysql_priv.h'
> --- a/sql/mysql_priv.h	2010-10-07 08:13:11 +0000
> +++ b/sql/mysql_priv.h	2010-12-07 14:32:55 +0000
> @@ -566,17 +566,25 @@ protected:
>
>   #define MY_CHARSET_BIN_MB_MAXLEN 1
>
> +/*
> +  These flags are set when we perform
> +  context analysis of the statement and make
> +  subqueries non-const. It prevents subquery
> +  evaluation at context analysis stage.
> +*/
> +#define CONTEXT_ANALYSIS_ONLY_PREPARE 1
> +#define CONTEXT_ANALYSIS_ONLY_VIEW    2
> +#define CONTEXT_ANALYSIS_ONLY_DERIVED 4
> +
>   // uncachable cause
>   #define UNCACHEABLE_DEPENDENT   1
>   #define UNCACHEABLE_RAND        2
>   #define UNCACHEABLE_SIDEEFFECT	4
>   /// forcing to save JOIN for explain
>   #define UNCACHEABLE_EXPLAIN     8
> -/** Don't evaluate subqueries in prepare even if they're not correlated */
> -#define UNCACHEABLE_PREPARE    16
>   /* For uncorrelated SELECT in an UNION with some correlated SELECTs */
> -#define UNCACHEABLE_UNITED     32
> -#define UNCACHEABLE_CHECKOPTION   64
> +#define UNCACHEABLE_UNITED     16
> +#define UNCACHEABLE_CHECKOPTION 32
>

[..]

> === modified file 'sql/sql_lex.h'
> --- a/sql/sql_lex.h	2010-08-18 04:56:06 +0000
> +++ b/sql/sql_lex.h	2010-12-07 14:32:55 +0000
> @@ -1715,14 +1715,8 @@ typedef struct st_lex : public Query_tab
>     bool verbose, no_write_to_binlog;
>
>     bool tx_chain, tx_release;
> -  /*
> -    Special JOIN::prepare mode: changing of query is prohibited.
> -    When creating a view, we need to just check its syntax omitting
> -    any optimizations: afterwards definition of the view will be
> -    reconstructed by means of ::print() methods and written to
> -    to an .frm file. We need this definition to stay untouched.
> -  */

Please move this comment to above where CONTEXT_ANALYSIS_ONLY_VIEW is 
declared.

> -  bool view_prepare_mode;
> +
> +  uint8 context_analysis_only;
>     bool safe_to_cache_query;
>     bool subqueries, ignore;
>     st_parsing_options parsing_options;
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2010-10-23 12:55:44 +0000
> +++ b/sql/sql_parse.cc	2010-12-07 14:32:55 +0000
> @@ -5865,13 +5865,6 @@ mysql_new_select(LEX *lex, bool move_dow
>       DBUG_RETURN(1);
>     }
>     select_lex->nest_level= lex->nest_level;
> -  /*
> -    Don't evaluate this subquery during statement prepare even if
> -    it's a constant one. The flag is switched off in the end of
> -    mysqld_stmt_prepare.
> -  */

Please move this comment to above where CONTEXT_ANALYSIS_ONLY_PREPARE is 
declared.

> -  if (thd->stmt_arena->is_stmt_prepare())
> -    select_lex->uncacheable|= UNCACHEABLE_PREPARE;
>     if (move_down)
>     {
>       SELECT_LEX_UNIT *unit;
>

Overall, looks good. Thanks for working on this!

Regards,

Davi
Thread
bzr commit into mysql-5.1-bugteam branch (sergey.glukhov:3514) Bug#52157Bug#54475 Bug#57352 Bug#57703Sergey Glukhov7 Dec
  • Re: bzr commit into mysql-5.1-bugteam branch (sergey.glukhov:3514)Bug#52157 Bug#54475 Bug#57352 Bug#57703Davi Arnaut13 Dec