List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:March 25 2011 1:19pm
Subject:Re: bzr commit into mysql-trunk branch (jorgen.loland:3653) Bug#11766327
View as plain text  
Hi Guilhem,

Thank you for the review. I've committed a new patch for this. See

http://lists.mysql.com/commits/133880

Comments inline:

On 02/17/2011 04:33 PM, Guilhem Bichot wrote:
> Hello,
>
> Jorgen Loland a écrit, Le 16.02.2011 10:27:
>> #At file:///export/home/jl208045/mysql/mysql-trunk-59415/ based on
>> revid:dmitry.lenev@stripped
>>
>> 3653 Jorgen Loland 2011-02-16
>> BUG#11766327: Range analysis should not be done many times for the same index
>> (formerly 59415)
>> Pre-bugfix changes: * Make JOIN_TAB::select_cond private, add getter
>> * Rename JOIN_TAB::select_cond to cond
>> * Rename JOIN_TAB::pre_idx_push_select_cond to pre_idx_push_cond
>> * Make use of extend_*_cond() where possible to simplify code
>> * Rename JOIN_TAB::set_select_cond() to set_jt_cond() since it sets
>> JOIN_TAB->cond, not JOIN_TAB->select->cond as the name suggested
>> * Rename JOIN_TAB::set_cond() to set_jt_and_sel_cond()
>> since it sets JOIN_TAB->cond AND JOIN_TAB->select->cond
>
> My curiosity: do you know how JOIN_TAB->cond differs from
> JOIN_TAB->select->cond ?

I have no idea.

>> === modified file 'sql/sql_select.cc'
>> --- a/sql/sql_select.cc 2011-02-10 09:30:20 +0000
>> +++ b/sql/sql_select.cc 2011-02-16 09:27:55 +0000
>
>> @@ -9301,9 +9301,7 @@ static void add_not_null_conds(JOIN *joi
>> DBUG_EXECUTE("where",print_where(notnull,
>> referred_tab->table->alias,
>> QT_ORDINARY););
>> - Item *new_cond= referred_tab->select_cond;
>> - add_cond_and_fix(&new_cond, notnull);
>> - referred_tab->set_select_cond(new_cond, __LINE__);
>> + referred_tab->extend_jt_cond(notnull);
>
> Difference between old and new code: if referred_tab->cond is initially NULL,
> old code (in add_cond_and_fix()) just did
> referred_tab->cond= notnull;
> but new code additionally does
> notnull->update_used_tables();
> notnull->quick_fix_field();
> Is that ok? I see that the old extend_select_cond() did that too, so maybe it's ok.

Yes, this is ok.

> I wonder: after the patch the function add_cond_and_fix() still exists is is
> used, and it seems to have common points with extend_jt_cond(). Duplication...
> can't we have one call the other?

Done. Thanks for letting me know.

>> @@ -10819,19 +10796,19 @@ static bool is_cond_sj_in_equality(Item }
>>
>>
>> -void remove_sj_conds(Item **tree)
>> +void remove_sj_conds(Item *cond)
>> {
>> - if (*tree)
>> + if (cond)
>> {
>> - if (is_cond_sj_in_equality(*tree))
>> + if (is_cond_sj_in_equality(cond))
>> {
>> - *tree= NULL;
>> + cond= NULL;
>
> "cond" is a parameter, local for your function; setting it to NULL has no effect
> (it will be lost after return). Surprising that no test fails :-(
> Setting *tree had effects outside of the function.

Oops... fixed.

>> +/**
>> + Extend JOIN_TAB->cond and JOIN_TAB->select->cond by AND'ing
>> + add_cond to them
>> +
>> + @param add_cond The condition to AND with the existing conditions
>> +
>> + @return
>> + true if there was an error, in which case the conditions remain
>> + unchanged
>> + false otherwise
>
> The multi-line @return shows up like this in the generated doxygen html output:
>
>> Returns:
>> true if there was an error, in which case JOIN_TAB::cond remains unchanged
>> false otherwise
>
> So it's hard to parse. I suggest to delete @return and use instead:
>
> @retval true if there was an error, in which case JOIN_TAB::cond remains unchanged
> @retval false otherwise
>
> which produces
>
>> Return values:
>> true if there was an error, in which case JOIN_TAB::cond remains unchanged
>> false otherwise
>
> ("unchanged" can be put on the next line in the comment in the code file,
> doxygen does not care about such new line, it "glues" everything).
>

OK, done

>> +*/
>> +bool JOIN_TAB::extend_jt_and_sel_cond(Item *add_cond)
>> +{
>> + if (extend_jt_cond(add_cond))
>> + return true;
>> + if (select)
>> + select->cond= cond;
>> + return false;
>> +}
>> +
>> +/**
>> + Extend JOIN_TAB->cond by AND'ing add_cond to it
>> +
>> + @param add_cond The condition to AND with the existing cond
>> + for this JOIN_TAB
>> + @return
>> + true if there was an error, in which case JOIN_TAB::cond remains
>> + unchanged
>
> if "cond" is NULL and "add_cond" is NULL, this function will return true, but
> should this really be interpreted as an error? This is important to decide,
> because an error makes extend_jt_and_sel_cond() terminate before setting
> select->cond.
> Or maybe it's impossible that add_cond is NULL (can we add DBUG_ASSERT() for
> this then?); that case would indeed be a bit meaningless.

Changed. exten_jt_and_sel_cond will only terminate early if there is a memory 
allocation error now.

>> @@ -17630,11 +17658,11 @@ evaluate_join_record(JOIN *join, JOIN_TA
>> join->thd->send_kill_message();
>> DBUG_RETURN(NESTED_LOOP_KILLED); /* purecov: inspected */
>> }
>> - DBUG_PRINT("info", ("select cond 0x%lx", (ulong)select_cond));
>> + DBUG_PRINT("info", ("jointab cond 0x%lx", (ulong)jointab_cond));
>
> can change to %p and no cast, while we're at it.

Good point

-- 
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
Thread
bzr commit into mysql-trunk branch (jorgen.loland:3653) Bug#11766327Jorgen Loland16 Feb
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3653) Bug#11766327Guilhem Bichot17 Feb
    • Re: bzr commit into mysql-trunk branch (jorgen.loland:3653) Bug#11766327Jorgen Loland25 Mar