MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:June 14 2010 1:17pm
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch
(tor.didriksen:3197) Bug#52538
View as plain text  
Hello,

Tor Didriksen a écrit, Le 14.06.2010 14:38:
> #At file:///export/home/didrik/repo/next-mr-opt-backporting-bug52538/ based on
> revid:jorgen.loland@stripped
> 
>  3197 Tor Didriksen	2010-06-14
>       Bug #52538 Valgrind bug: Item_in_subselect::init_left_expr_cache()
>       
>       The JOIN_TAB created in make_simple_join() was not properly initialized.
>       Use new (thd->mem_root) rather than thd->alloc()
>      @ mysql-test/r/subselect_mat.result
>         Add test case.
>      @ mysql-test/t/subselect_mat.test
>         Add test case.
>      @ sql/item_subselect.cc
>         Do not ignore return value of init_left_expr_cache()
>         Initialize allocated JOIN_TAB.
>         Don't do function calls in DBUG_RETURN().
>      @ sql/item_subselect.h
>         Introduce new member need_expr_cache.

what is this for? How is this connected to the bug? What was wrong 
before, which is correct now?

>      @ sql/sql_bitmap.h
>         Initialize Bitmap in constructor.

is that needed?

> === modified file 'mysql-test/r/subselect_mat.result'
> --- a/mysql-test/r/subselect_mat.result	2010-06-09 14:16:33 +0000
> +++ b/mysql-test/r/subselect_mat.result	2010-06-14 12:37:58 +0000

> +SELECT table2.col_varchar_key AS field1,
> +table2.col_int_nokey AS field2
> +FROM ( t1 AS table1 LEFT OUTER JOIN t1 AS table2
> +ON (table2.col_varchar_key = table1.col_varchar_key  ) ) 
> +WHERE table1.pk = 6
> +HAVING  ( field2 ) IN 
> +( SELECT SUBQUERY2_t2.col_int_nokey AS SUBQUERY2_field2 
> +FROM ( t1 AS SUBQUERY2_t1 JOIN t1 AS SUBQUERY2_t2
> +ON (SUBQUERY2_t2.col_varchar_key = SUBQUERY2_t1.col_varchar_key ) ) )
> +ORDER BY field2 
> +;
> +field1	field2
> +t	1
> +t	2

is that a correct result?

> === modified file 'sql/item_subselect.cc'
> --- a/sql/item_subselect.cc	2010-05-14 10:58:39 +0000
> +++ b/sql/item_subselect.cc	2010-06-14 12:37:58 +0000
> @@ -150,6 +150,7 @@ void Item_in_subselect::cleanup()
>      left_expr_cache= NULL;
>    }
>    first_execution= TRUE;
> +  need_expr_cache= TRUE;
>    Item_subselect::cleanup();
>    DBUG_VOID_RETURN;
>  }
> @@ -325,8 +326,9 @@ bool Item_in_subselect::exec()
>      - on a cost-based basis, that takes into account the cost of a cache
>        lookup, the cache hit rate, and the savings per cache hit.
>    */
> -  if (!left_expr_cache && exec_method == MATERIALIZATION)
> -    init_left_expr_cache();
> +  if (need_expr_cache && !left_expr_cache && exec_method ==
> MATERIALIZATION &&
> +      init_left_expr_cache())
> +    DBUG_RETURN(TRUE);
>  
>    /* If the new left operand is already in the cache, reuse the old result. */
>    if (left_expr_cache && test_if_item_cache_changed(*left_expr_cache) <
> 0)

> @@ -743,6 +746,7 @@ bool Item_in_subselect::test_limit(st_se
>  Item_in_subselect::Item_in_subselect(Item * left_exp,
>  				     st_select_lex *select_lex):
>    Item_exists_subselect(), left_expr_cache(0), first_execution(TRUE),
> +  need_expr_cache(TRUE),
>    optimizer(0), pushed_cond_guards(NULL), exec_method(NOT_TRANSFORMED),
>    upper_item(0)
>  {
> @@ -1911,8 +1915,7 @@ bool Item_in_subselect::setup_engine()
>    but it takes a different kind of collection of items, and the
>    list we push to is dynamically allocated.
>  
> -  @retval TRUE  if a memory allocation error occurred or the cache is
> -                not applicable to the current query
> +  @retval TRUE  if a memory allocation error occurred
>    @retval FALSE if success
>  */
>  
> @@ -1928,7 +1931,11 @@ bool Item_in_subselect::init_left_expr_c
>      been optimzied away.
>    */ 
>    if (!outer_join || !outer_join->tables || !outer_join->tables_list)
> -    return TRUE;
> +  {
> +    need_expr_cache= FALSE;
> +    return FALSE;
> +  }
> +
>    /*
>      If we use end_[send | write]_group to handle complete rows of the outer
>      query, make the cache of the left IN operand use Item_field::result_field

> === modified file 'sql/item_subselect.h'
> --- a/sql/item_subselect.h	2010-06-14 06:59:59 +0000
> +++ b/sql/item_subselect.h	2010-06-14 12:37:58 +0000
> @@ -296,6 +296,8 @@ protected:
>    */
>    List<Cached_item> *left_expr_cache;
>    bool first_execution;
> +  /* The need for expr cache may be optimized away, see init_left_expr_cache() */

/** */ for doxygen

> +  bool need_expr_cache;

> === modified file 'sql/sql_select.h'
> --- a/sql/sql_select.h	2010-06-11 12:27:38 +0000
> +++ b/sql/sql_select.h	2010-06-14 12:37:58 +0000
> @@ -179,9 +179,10 @@ inline bool sj_is_materialize_strategy(u
>    return strategy >= SJ_OPT_MATERIALIZE_LOOKUP;
>  }
>  
> -typedef struct st_join_table
> +typedef struct st_join_table : public Sql_alloc
>  {
> -  st_join_table() {}                          /* Remove gcc warning */
> +  st_join_table();
> +
>    TABLE		*table;
>    KEYUSE	*keyuse;			/**< pointer to first used key */
>    SQL_SELECT	*select;
> @@ -380,6 +381,91 @@ typedef struct st_join_table
>    uint get_sj_strategy() const;
>  } JOIN_TAB;
>  
> +
> +inline
> +st_join_table::st_join_table()
> +  : table(NULL),
> +    keyuse(NULL),
> +    select(NULL),
> +    select_cond(NULL),
> +    quick(NULL),
> +    on_expr_ref(NULL),
...
> +
> +    keep_current_rowid(0),
> +    embedding_map(0)

FYI JOIN::save_join_tab() does
join_tab_save= (JOIN_TAB*)thd->memdup((uchar*) join_tab,
						sizeof(JOIN_TAB) * tables)))
I'm not sure this is a problem though.

> +{
> +  /*
> +    @todo Add constructor to READ_RECORD.

If /** */ were used, doxygen would pick up the @todo.

> +    All users do init_read_record(), which does bzero(),
> +    rather than invoking a constructor.
> +  */
> +  bzero(&read_record, sizeof(read_record));

so here we do a bzero() and another bzero() of the same bytes in 
init_read_record()?

Thread
bzr commit into mysql-next-mr-opt-backporting branch (tor.didriksen:3197)Bug#52538Tor Didriksen14 Jun
  • Re: bzr commit into mysql-next-mr-opt-backporting branch(tor.didriksen:3197) Bug#52538Guilhem Bichot14 Jun
    • Re: bzr commit into mysql-next-mr-opt-backporting branch(tor.didriksen:3197) Bug#52538Tor Didriksen14 Jun
      • Re: bzr commit into mysql-next-mr-opt-backporting branch(tor.didriksen:3197) Bug#52538Guilhem Bichot14 Jun
        • Re: bzr commit into mysql-next-mr-opt-backporting branch(tor.didriksen:3197) Bug#52538Tor Didriksen15 Jun
          • Re: bzr commit into mysql-next-mr-opt-backporting branch(tor.didriksen:3197) Bug#52538Guilhem Bichot15 Jun