List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:June 30 2010 9:03am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch
(jorgen.loland:3275) Bug#54511
View as plain text  
Hello,

Jorgen Loland a écrit, Le 30.06.2010 10:33:
> #At file:///localhome/jl208045/mysql/mysql-next-mr-bugfixing/ based on
> revid:guilhem@stripped
> 
>  3275 Jorgen Loland	2010-06-30
>       Bug#54511: Assertion failed: cache != 0L in file 
>                  sql_select.cc::sub_select_cache on HAVING
>       
>       Code cleanup followup patch: make JOIN::having_value and 
>       cond_value point to select_lex versions of the variables 
>       with same name to avoid duplication of information. Also 
>       add documentation to having_value and cond_value in 
>       st_select_lex and JOIN and to optimize_cond().
>      @ sql/sql_lex.h
>         Add documentation to st_select_lex::having_value and cond_value
>      @ sql/sql_select.cc
>         Make JOIN::having_value and cond_value point to select_lex 
>         variables with same name and document optimize_cond()
>      @ sql/sql_select.h
>         Make JOIN::having_value and cond_value point to select_lex 
>         variables with same name and document how they work.
> 
>     modified:
>       sql/sql_lex.h
>       sql/sql_select.cc
>       sql/sql_select.h
> === modified file 'sql/sql_lex.h'
> --- a/sql/sql_lex.h	2010-06-26 07:43:14 +0000
> +++ b/sql/sql_lex.h	2010-06-30 08:33:05 +0000
> @@ -639,7 +639,17 @@ public:
>    Item *where, *having;                         /* WHERE & HAVING clauses */
>    Item *prep_where; /* saved WHERE clause for prepared statement processing */
>    Item *prep_having;/* saved HAVING clause for prepared statement processing */
> -  /* Saved values of the WHERE and HAVING clauses*/
> +  /**
> +    Saved values of the WHERE and HAVING clauses. Allowed values are: 
> +     - COND_UNDEF if the condition was not specified in the query or if it 
> +       has not been optimized yet
> +     - COND_TRUE if the condition is always true
> +     - COND_FALSE if the condition is impossible
> +     - COND_OK otherwise
> +    NOTE: In contrast to JOIN::{having|conds}, conds and having are

you can use @note instead of NOTE, for doxygen
Can you explain why "In contrast etc"? I have no clue why there is 
different treatment.

> +    not set to NULL here even if the associated *_value is set to
> +    COND_TRUE or COND_FALSE.
> +  */
>    Item::cond_result cond_value, having_value;
>    /* point on lex in which it was created, used in view subquery detection */
>    LEX *parent_lex;
> 
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-06-26 07:43:14 +0000
> +++ b/sql/sql_select.cc	2010-06-30 08:33:05 +0000
> @@ -508,6 +508,8 @@ JOIN::prepare(Item ***rref_pointer_array
>    tables_list= tables_init;
>    select_lex= select_lex_arg;
>    select_lex->join= this;
> +  having_value= &select_lex->having_value;
> +  cond_value= &select_lex->cond_value;
>    join_list= &select_lex->top_join_list;
>    union_part= unit_arg->is_union();
>  
> @@ -898,7 +900,7 @@ JOIN::optimize()
>        thd->restore_active_arena(arena, &backup);
>    }
>  
> -  conds= optimize_cond(this, conds, join_list, &cond_value);   
> +  conds= optimize_cond(this, conds, join_list, cond_value);
>    if (thd->is_error())
>    {
>      error= 1;
> @@ -907,24 +909,19 @@ JOIN::optimize()
>    }
>  
>    {
> -    having= optimize_cond(this, having, join_list, &having_value);
> +    having= optimize_cond(this, having, join_list, having_value);
>      if (thd->is_error())
>      {
>        error= 1;
>        DBUG_PRINT("error",("Error from optimize_cond"));
>        DBUG_RETURN(1);
>      }
> -    if (select_lex->where)
> -      select_lex->cond_value= cond_value;
> -    if (select_lex->having)
> -      select_lex->having_value= having_value;
> -
> -    if (cond_value == Item::COND_FALSE || having_value == Item::COND_FALSE || 
> +    if (*cond_value == Item::COND_FALSE || *having_value == Item::COND_FALSE || 
>          (!unit->select_limit_cnt && !(select_options &
> OPTION_FOUND_ROWS)))
>      {						/* Impossible cond */
> -      DBUG_PRINT("info", (having_value == Item::COND_FALSE ? 
> +      DBUG_PRINT("info", (*having_value == Item::COND_FALSE ? 
>                              "Impossible HAVING" : "Impossible WHERE"));
> -      zero_result_cause=  having_value == Item::COND_FALSE ?
> +      zero_result_cause=  *having_value == Item::COND_FALSE ?
>                             "Impossible HAVING" : "Impossible WHERE";
>        tables= 0;
>        error= 0;
> @@ -1149,10 +1146,9 @@ JOIN::optimize()
>    if (having && const_table_map)
>    {
>      having->update_used_tables();
> -    having= remove_eq_conds(thd, having, &having_value);
> -    if (having_value == Item::COND_FALSE)
> +    having= remove_eq_conds(thd, having, having_value);
> +    if (*having_value == Item::COND_FALSE)
>      {
> -      having= new Item_int((longlong) 0,1);
>        zero_result_cause= "Impossible HAVING noticed after reading const tables";
>        error= 0;
>        DBUG_RETURN(0);
> @@ -1823,8 +1819,8 @@ JOIN::exec()
>          In this case JOIN::exec must check for JOIN::having_value, in the
>          same way it checks for JOIN::cond_value.
>        */
> -      if (cond_value != Item::COND_FALSE &&
> -          having_value != Item::COND_FALSE &&
> +      if (*cond_value != Item::COND_FALSE &&
> +          *having_value != Item::COND_FALSE &&
>            (!conds || conds->val_int()) &&
>            (!having || having->val_int()))
>        {
> @@ -9278,6 +9274,28 @@ static void restore_prev_nj_state(JOIN_T
>  }
>  
>  
> +/**
> +  Optimize conditions by 
> +
> +     a) applying transitivity to build multiple equality predicates
> +        (MEP): if x=y and y=z the MEP x=y=z is built. 
> +     b) apply constants where possible. If the value of x is known to be
> +        42, x is replaced with a constant of value 42. By transitivity, this
> +        also applies to MEPs, so the MEP in a) will become 42=x=y=z.
> +     c) remove conditions that are impossible or always true
> +  
> +  @param      join         pointer to the structure providing all context info
> +                           for the query
> +  @param      conds        conditions to optimize
> +  @param      join_list    list of join tables to which the condition
> +                           refers to
> +  @param[out] cond_value   Not changed if conds was empty 
> +                           COND_TRUE if conds is always true
> +                           COND_FALSE if conds is impossible
> +                           COND_OK otherwise

thanks for the comment!

> +
> +  @return optimized conditions
> +*/
>  static COND *
>  optimize_cond(JOIN *join, COND *conds, List<TABLE_LIST> *join_list,
>                Item::cond_result *cond_value)
> @@ -9285,9 +9303,7 @@ optimize_cond(JOIN *join, COND *conds, L
>    THD *thd= join->thd;
>    DBUG_ENTER("optimize_cond");
>  
> -  if (!conds)
> -    *cond_value= Item::COND_TRUE;
> -  else
> +  if (conds)

looks like a real change? That's the only thing in the patch which 
worries me (can this introduce a bug?)

>    {
>      /* 
>        Build all multiple equality predicates and eliminate equality
> 
> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h	2010-06-22 20:57:34 +0000
> +++ b/sql/sql_select.h	2010-06-30 08:33:05 +0000
> @@ -418,7 +418,19 @@ public:
>  
>    bool need_tmp, hidden_group_fields;
>    DYNAMIC_ARRAY keyuse;
> -  Item::cond_result cond_value, having_value;
> +  /** 
> +    Pointers to variables with same names in select_lex. The allowed 
> +    states are: 
> +     - COND_UNDEF if the condition was not specified in the query or if it 
> +       has not been optimized yet
> +     - COND_TRUE if the condition is always true.
> +     - COND_FALSE if the condition is impossible
> +     - COND_OK otherwise
> +    If cond_result is set to COND_TRUE or COND_FALSE, conds is also

should "(cond|having)_result" be "(cond|having)_value"?

> +    set to NULL. The same applies to the having_result/having variable
> +    pair.
> +  */
> +  Item::cond_result *cond_value, *having_value; 
>    List<Item> all_fields; ///< to store all fields that used in query
>    ///Above list changed to use temporary table
>    List<Item> tmp_all_fields1, tmp_all_fields2, tmp_all_fields3;
> @@ -573,7 +585,7 @@ public:
>    bool send_row_on_empty_set()
>    {
>      return (do_send_rows && tmp_table_param.sum_func_count != 0 &&
> -	    !group_list && having_value != Item::COND_FALSE);
> +	    !group_list && *having_value != Item::COND_FALSE);
>    }
>    bool change_result(select_result *result);
>    bool is_top_level_join() const

This change with pointers is ok. What do you think of going even 
further, i.e. removing the pointers from JOIN, and using directly 
select_lex variables? It would make apparent the dependency of certain 
pieces of JOIN code over select_lex.

-- 
Mr. Guilhem Bichot <guilhem@stripped>
Sun Microsystems / MySQL / Optimizer team, Lead Software Engineer
Bordeaux, France
www.sun.com / www.mysql.com
Thread
bzr commit into mysql-next-mr-bugfixing branch (jorgen.loland:3275) Bug#54511Jorgen Loland30 Jun
  • Re: bzr commit into mysql-next-mr-bugfixing branch(jorgen.loland:3275) Bug#54511Guilhem Bichot30 Jun
    • Re: bzr commit into mysql-next-mr-bugfixing branch(jorgen.loland:3275) Bug#54511Jørgen Løland30 Jun
      • Re: bzr commit into mysql-next-mr-bugfixing branch(jorgen.loland:3275) Bug#54511Guilhem Bichot30 Jun
        • Re: bzr commit into mysql-next-mr-bugfixing branch(jorgen.loland:3275) Bug#54511Jørgen Løland1 Jul