List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:May 2 2011 10:32am
Subject:Re: bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327
View as plain text  
On 02.05.11 11.23, Jorgen Loland wrote:
> Roy,
>
> thank you for the review. See comments inline.
>
> On 04/29/2011 10:10 AM, Roy Lyseng wrote:
>> Hi Jørgen,
>>
>> Thank you for the bugfix. I have a few additional suggestions, and some problem
>> areas that I think you need to look further into (ie the issue of fixed
>> arguments to extend_...()).
>>
>> Generally I like the changes, but I think that you may be even more consistent
>> and e.g. spell out "condition" in full, at least in function and member names.
>>
>> On 06.04.11 14.55, Jorgen Loland wrote:
>>> #At file:///export/home/jl208045/mysql/mysql-trunk-59415/ based on
>>> revid:georgi.kodinov@stripped
>>>
>>> 3333 Jorgen Loland 2011-04-06
>>> 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
>>> * Make JOIN_TAB::remove_sj_conditions(), needed because
>>> select_cond is now private.
>>> * 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_condition() since it sets JOIN_TAB->cond, not
>>> JOIN_TAB->select->cond as the name suggested
>>
>> It probably made more sense before your name change ;)
>
> ?
>

The member used to be called select_cond and the function set_select_cond().

>>> @@ -9248,28 +9248,39 @@ JOIN::make_simple_join(JOIN *parent, TAB
>>> DBUG_RETURN(FALSE);
>>> }
>>>
>>> +/**
>>> + Extend e1 by AND'ing e2 to the condition e1 points to.
>>> +
>>> + @param[in,out] e1 Pointer to condition that will be extended with e2
>>> + @param e2 Condition that will extend e1
>>>
>>> -inline void add_cond_and_fix(Item **e1, Item *e2)
>>> + @retval true if there was a memory allocation error, in which case
>>> + e1 remains unchanged
>>> + @retval false otherwise
>>> +*/
>>> +inline bool add_cond_and_fix(Item **e1, Item *e2)
>>
>> add_conjunctive_condition is probably a better name for this function. Your
>> choice.
>>
>> The "fix" thing may just as well be documented in the description...
>
> How about and_conditions()?

Less formal, but OK...
>
>>
>> I think that this function requires inputs that are already fixed. Please
>> reflect that in the documentation and add DBUG_ASSERTs on
> <input>->fixed.
>
> Good suggestion
>
>>> /**
>>> - Add to join_tab->select_cond[i] "table.field IS NOT NULL" conditions
>>> + Add to join_tab->cond[i] "table.field IS NOT NULL" conditions
>>
>> Is "join_tab[i]->cond" what is really meant?
>
> Seems so :)
>
>>> @@ -9545,9 +9536,8 @@ static bool pushdown_on_conditions(JOIN*
>>> tmp_cond= new
> Item_func_trig_cond(tmp_cond,&cond_tab->not_null_compl);
>>> if (!tmp_cond)
>>> DBUG_RETURN(1);
>>> - tmp_cond->quick_fix_field();
>>
>> I think this one is actually needed.
>
> Fixed here and elsewhere
>
>>> @@ -9598,11 +9588,9 @@ static bool pushdown_on_conditions(JOIN*
>>> */
>>> tmp_cond= new Item_func_trig_cond(tmp_cond,
>>> &first_inner_tab->not_null_compl);
>>
>> Missing error check here?
>
> done
>
>>> - if (tmp_cond)
>>> - tmp_cond->quick_fix_field();
>>
>> I think that this quick_fix_field() call is actually needed. The extend_...()
>> functions only seem to fix the new root node that they add.
>>
>>>
>>> /* Add the predicate to other pushed down predicates */
>>> - if (extend_select_cond(cond_tab, tmp_cond))
>>> + if (cond_tab->extend_jt_and_sel_cond(tmp_cond, __LINE__))
>>> DBUG_RETURN(1);
>>
>> DBUG_RETURN(true) ?
>
> Fixed all that are close to my modifications
>
>>> @@ -10373,30 +10361,30 @@ static void push_index_cond(JOIN_TAB *ta
>>> if (idx_remainder_cond != idx_cond)
>>> tab->ref.disable_cache= TRUE;
>>>
>>> - Item *row_cond= make_cond_remainder(tab->select_cond, TRUE);
>>> + Item *row_cond= make_cond_remainder(tab->condition(), TRUE);
>>> DBUG_EXECUTE("where", print_where(row_cond, "remainder cond",
>>> QT_ORDINARY););
>>>
>>> if (row_cond)
>>> {
>>> if (!idx_remainder_cond)
>>> - tab->set_select_cond(row_cond, __LINE__);
>>> + tab->set_condition(row_cond, __LINE__);
>>> else
>>> {
>>> Item *new_cond= new Item_cond_and(row_cond, idx_remainder_cond);
>>
>> Missing error check here. Can you use the add_cond...() function here as well?
>
> This function does nothing to handle errors. I'd prefer not to rewrite this
> function and all usages of it.

OK.

>
> Utility function can be used and looks good - done.
>
>>> @@ -10897,19 +10885,16 @@ static bool is_cond_sj_in_equality(Item
>>> }
>>>
>>>
>>> -void remove_sj_conds(Item **tree)
>>> +Item *remove_sj_conds(Item *tree)
>>
>> Please document this function with a header.
>>
>
> done
>
>>> @@ -11078,10 +11063,12 @@ bool setup_sj_materialization(JOIN_TAB *
>>> */
>>> for (i= 0; i< sjm->table_count; i++)
>>> {
>>> - remove_sj_conds(&tab[i].select_cond);
>>> + Item *new_cond= remove_sj_conds(tab[i].condition());
>>> + tab[i].set_condition(new_cond, __LINE__);
>>
>> "new_cond" may be replaced with the function call here (for symmetry :)
>>
>
> Rewrote to:
> tab[i].set_condition(remove_sj_conds(tab[i].condition()), __LINE__);
>
>>> @@ -11470,6 +11457,58 @@ uint JOIN_TAB::get_sj_strategy() const
>>> return s;
>>> }
>>>
>>> +/**
>>> + 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
>>> + @param line Code line this method was called from
>>> +
>>> + @retval true if there was a memory allocation error, in which
>>> + case the conditions remain unchanged
>>> + @retval false otherwise
>>
>> I think there is unnecessary information here. We should ALWAYS leave things
>> unchanged when there is an error, thus we only need to document if there is an
>> exception to this rule.
>
> ok
>
>>
>>> +
>>> +*/
>>> +bool JOIN_TAB::extend_jt_and_sel_cond(Item *add_cond, uint line)
>>> +{
>>> + if (extend_cond(add_cond, line))
>>> + return true;
>>> +
>>> + if (select)
>>> + {
>>> + DBUG_PRINT("info",
>>> + ("select::cond extended. Change %p -> %p "
>>> + "at line %u tab %p select %p",
>>> + select->cond, cond, line, this, 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
>>> + @param line Code line this method was called from
>>> +
>>> + @retval true if there was a memory allocation error, in which case
>>> + JOIN_TAB::cond remains unchanged
>>> + @retval false otherwise
>>> +*/
>>> +bool JOIN_TAB::extend_cond(Item *add_cond, uint line)
>>> +{
>>> + Item *old_cond= cond;
>>> + if (add_cond_and_fix(&cond, add_cond))
>>> + return true;
>>> + DBUG_PRINT("info", ("JOIN_TAB::cond extended. Change %p -> %p "
>>> + "at line %u tab %p",
>>> + old_cond, cond, line, this));
>>> + return false;
>>> +}
>>> +
>>> +
>>> +
>>
>> I do not grasp the difference between extend_cond and extend_jt_and_sel_cond().
>> Is it possible to document the difference and insert an advice on which function
>> to use in which condition?
>
> I do not know why we have different conditions in join_tab and join_tab->select.
> I consider it outside the scope finding out why and fixing it. It would be great
> if someone did this!
>
>>
>> Besides, we should make up our mind on whether to "extend" a condition or "add"
>> something to a condition, cf. add_cond_and_fix().
>
> How about this:
>
> and_conditions()
> J_T::and_with_condition()
> J_T::and_with_jt_and_sel_condition()

It's OK.

>
>>> @@ -17689,14 +17728,14 @@ evaluate_join_record(JOIN *join, JOIN_TA
>>> {
>>> bool not_used_in_distinct=join_tab->not_used_in_distinct;
>>> ha_rows found_records=join->found_records;
>>> - Item *select_cond= join_tab->select_cond;
>>> + Item *jointab_cond= join_tab->condition();
>>
>> SImply "condition" would do well. It may also be const.
>
> Unfortunately not const since I'll have to fix val_int() as well.

I thought of Item const* condition;

>
>>> @@ -17921,7 +17960,7 @@ evaluate_null_complemented_join_record(J
>>> */
>>> JOIN_TAB *last_inner_tab= join_tab->last_inner;
>>> /* Cache variables for faster loop */
>>> - Item *select_cond;
>>> + Item *jointab_cond;
>>
>> This variable is better removed.
>
> Hmm... is the above comment just crap then?

No, inside this function, the variable is defined outside of the loop, and 
referenced inside it in two places. I'd say it is not useful.
>
>>> @@ -18661,8 +18700,8 @@ end_send(JOIN *join, JOIN_TAB *join_tab
>>> {
>>> JOIN_TAB *jt=join->join_tab;
>>> if ((join->tables == 1)&& !join->tmp_table&&
> !join->sort_and_group
>>> - && !join->send_group_parts&& !join->having&&
> !jt->select_cond&&
>>> - !(jt->select&& jt->select->quick)&&
>>> +&& !join->send_group_parts&& !join->having&&
>>> + !jt->condition()&& !(jt->select&&
> jt->select->quick)&&
>>> (jt->table->file->ha_table_flags()&
> HA_STATS_RECORDS_IS_EXACT)&&
>>> (jt->ref.key< 0))
>>
>> What about spelling this if test out with one predicate per line?
>
> Good idea.
>
>>> @@ -258,7 +258,9 @@ typedef struct st_join_table : public Sq
>>> TABLE *table;
>>> Key_use *keyuse; /**< pointer to first used key */
>>> SQL_SELECT *select;
>>> - Item *select_cond;
>>> +private:
>>> + Item *cond; /**< condition for this JOIN_TAB */
>>
>> Four-letter member names are too brief in large class definitions (albeit this
>> is pretty well-defined). I suggest m_condition instead. (The prefix is needed
>> because of the clash with condition()).
>
> m_condition it is (but to keep focus I didn't change the name of
> join_tab->select->cond)
>

Thanks,
Roy
Thread
bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327Jorgen Loland6 Apr
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327Guilhem Bichot7 Apr
  • Re: bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327Roy Lyseng29 Apr
    • Re: bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327Jorgen Loland2 May
      • Re: bzr commit into mysql-trunk branch (jorgen.loland:3333) Bug#11766327Roy Lyseng2 May