MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:June 15 2010 12:32pm
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 15.06.2010 11:11:
> On 2010-06-14 22:08, Guilhem Bichot wrote:
>> Hello,
>>
>> No big objections, just more questions and suggestions.
>>
>> Tor Didriksen a écrit, Le 14.06.2010 17:03:
>>> 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()


>>>>>      @ 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.
>>
>> Your change is fine.
>> It touches Bitmap<64>, you could make your change more similar to the 
>> other Bitmap class in this file (this one:
>> template <uint default_width> class Bitmap ): it has a constructor 
>> which only calls init(), and init() does the zero-ing. You could do 
>> the same for Bitmap<64>: it currently has an empty init(); init() 
>> could do the zero-ing and the constructor could call init().
> 
> It certainly makes sense to have the same behaviour in both classes.
> BTW: olejohn (on telco branch) wants this bitmap change as well.
> Maybe I should have a separate patch for bitmap init (so he can 
> cherry-pick it)?

This change is only a few lines, so it doesn't really matter whether you 
and he commit it in two trees independently. In fact, I don't care :-) 
As long as if you don't make the constructor of Bitamp<64> init to 0 
now, you remember to add explicit bitmap zeroing in the constructor of 
JOIN_TAB.

> He has an array of Bitmap<64> and would like the default constructor to 
> do the initialization.
> 
>>
>> It seems that your change allows to eliminate some explicit settings 
>> to 0 in other places of code. For example field.cc:
>> Field::Field(uchar *ptr_arg,uint32 length_arg,uchar *null_ptr_arg,
>>          uchar null_bit_arg,
>>          utype unireg_check_arg, const char *field_name_arg)
>>   :ptr(ptr_arg), null_ptr(null_ptr_arg),
>>    table(0), orig_table(0), table_name(0),
>>    field_name(field_name_arg),
>>    key_start(0), part_of_key(0), part_of_key_not_clustered(0),
>>    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>    at least those three are Bitmap<64>, and there are more I think (I 
>> only grep-ped for "part_of_key" which is a key_part which is a typedef 
>> for Bitmap<64>).
>> Another one, sql_select.cc:
>>
>>     field= new Field_varstring(uniq_tuple_length_arg, FALSE, "rowids", 
>> share,
>> &my_charset_bin);
>>     if (!field)
>>       DBUG_RETURN(0);
>>     field->table= table;
>> !    field->key_start.init(0);
>> !    field->part_of_key.init(0);
>> !    field->part_of_sortkey.init(0);
>>
>> I suggest eliminating those now-useless settings (shorter code - 
>> yessss!), if it doesn't scare you. It would be possible to do it in a 
>> second patch.
>>
> 
> Not so scared, but would prefer a separate patch.

Ok. Then could you please put it on your TODO? Otherwise we will keep 
now-useless code which will just take time to execute and make the code 
longer.

>> JFYI A drawback of the change is that now this code:
>>   bool check_quick(THD *thd, bool force_quick_range, ha_rows limit)
>>   {
>>     key_map tmp;
>>     tmp.set_all();
>> is less efficient (first sets to 0 and sets all bits).
> 
> That should have been key_map tmp(64);
> Or mabye key_map tmp(ALL_BITS);
> Again: separate patch.

Ok. Could you please put it on your TODO too?

>>>>> === 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.
>>
>> Should we do it, while we are at this?
> 
> OK

Thanks!

>>
>>>>
>>>>> +{
>>>>> +  /*
>>>>> +    @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.
>>
>> Yes, let's not introduce a duplicate bzero now. Either put bzero in 
>> the JOIN_TAB constructor and remove it elsewhere, or leave it elsewhere.
>>
> 
> Lots of explicit initialization removed.

Thanks.

Your latest patch is approved, I suggest just one minor last change in 
item_subselect.h (sorry for not mentioning it earlier):

+  /** The need for expr cache may be optimized away, see 
init_left_expr_cache. */
+  bool need_expr_cache;

I suggest changing "see" to @sa. It will make an HTML link from this 
comment to the code of init_left_expr_cache, in doxygen's output. @sa 
stands for "see also".

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