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 ?
> @ sql/sql_select.cc
> JOIN_TAB::cond made private, rename JOIN_TAB functions for getting and
> setting condition to better reflect reality, use extend_*_cond() where possible to
> simplify code.
> @ sql/sql_select.h
> Fix naming of JOIN_TAB variables and functions to better reflect reality.
> Make JOIN_TAB::cond (formerly select_cond) private.
> @ sql/sql_show.cc
> JOIN_TAB::cond is made private
> === 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.
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?
> }
> }
> }
> @@ -9440,24 +9438,6 @@ make_outerjoin_info(JOIN *join)
> DBUG_VOID_RETURN;
> }
>
> -static bool extend_select_cond(JOIN_TAB *cond_tab, Item *tmp_cond)
> -{
> - DBUG_ENTER("extend_select_cond");
> -
> - Item *new_cond= !cond_tab->select_cond ? tmp_cond :
> - new Item_cond_and(cond_tab->select_cond, tmp_cond);
> - cond_tab->set_select_cond(new_cond, __LINE__);
> - if (!cond_tab->select_cond)
> - DBUG_RETURN(1);
> - cond_tab->select_cond->update_used_tables();
> - cond_tab->select_cond->quick_fix_field();
> - if (cond_tab->select)
> - cond_tab->select->cond= cond_tab->select_cond;
> -
> - DBUG_RETURN(0);
> -}
> -
> -
> @@ -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.
> return;
> }
> +/**
> + 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).
> +*/
> +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.
> + false otherwise
> +*/
> +bool JOIN_TAB::extend_jt_cond(Item *add_cond)
> +{
> + DBUG_ENTER("JOIN_TAB::extend_jt_cond");
> +
> + Item *new_cond= cond ?
> + new Item_cond_and(cond, add_cond) :
> + add_cond;
> +
> + if (!new_cond)
> + DBUG_RETURN(true);
> +
> + set_jt_cond(new_cond, __LINE__);
> +
> + cond->update_used_tables();
> + cond->quick_fix_field();
> +
> + DBUG_RETURN(false);
> +}
> @@ -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.
> @@ -416,21 +418,27 @@ typedef struct st_join_table : public Sq
> {
> return first_inner && first_inner == this;
> }
> - void set_select_cond(Item *to, uint line)
> + Item *get_jt_cond()
> + {
> + return cond;
> + }
Good that you didn't declare it const, because logically it's not const
(assuming there exists plenty of code which calls "modifying" functions
on the pointer returned by get_jt_cond(); which you can check by
changing the return type to "const Item*", for fun...).
> + void set_jt_cond(Item *to, uint line)
> {
> - DBUG_PRINT("info", ("select_cond changes %p -> %p at line %u tab %p",
> - select_cond, to, line, this));
> - select_cond= to;
> + DBUG_PRINT("info", ("JOIN_TAB::cond changes %p -> %p at line %u tab %p",
> + cond, to, line, this));
> + cond= to;
> }
> - Item *set_cond(Item *new_cond, uint line)
> + Item *set_jt_and_sel_cond(Item *new_cond, uint line)
I'd put a function comment to say this returns the old cond.
> {
> - Item *tmp_select_cond= select_cond;
> - set_select_cond(new_cond, line);
> + Item *tmp_cond= cond;
> + set_jt_cond(new_cond, line);
> if (select)
> select->cond= new_cond;
> - return tmp_select_cond;
> + return tmp_cond;
> }
