MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Tor Didriksen Date:June 14 2010 3:03pm
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch
(tor.didriksen:3197) Bug#52538
View as plain text  
On 2010-06-14 15:17, Guilhem Bichot wrote:
> 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?

I changed the signature of init_left_expr_cache()
-  @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

so that it is possible to catch memory allocation errors
(previously, the return value was ignored)

>
>>      @ sql/sql_bitmap.h
>>         Initialize Bitmap in constructor.
>
> is that needed?

JOIN_TAB contains a few key_map's, I want them to initialize themselves.
The alternative is to do const_keys.set_prefix(0); etc. in the body of 
JOIN_TAB constructor.

>
>> === 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?

I hope so.
That's what I'm getting in trunk and next-mr as well.

>
>> === 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

done

>
>> +  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.

I know. There should have been an assignment operator instead.

>
>> +{
>> +  /*
>> +    @todo Add constructor to READ_RECORD.
>
> If /** */ were used, doxygen would pick up the @todo.

done

>
>> +    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()?

Actually yes we do, somewhere near the end of make_simple_join()
That can be removed I think.
I would like to cleanup READ_RECORD as well, but in a separate patch.

-- didrik

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