List:Commits« Previous MessageNext Message »
From:Jørgen Løland Date:July 2 2010 8:47am
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch (olav:3203)
View as plain text  

Compared to the previous patch I reviewed, you now leave the and-ing of
pre_idx_push_select_cond and original_select_cond unchanged around line 19451:

COND *tmp_cond= tab->pre_idx_push_select_cond;
if (orig_select_cond)
  tmp_cond= and_conds(tmp_cond, orig_select_cond);
tab->set_cond(tmp_cond, __LINE__);
/* orig_select_cond was merged, no need to restore original one. */
orig_select_cond= 0;

I understand that this is because you have found cases where some conds have 
been removed before pre_idx_push_select_cond was assigned but were later added 
to select_conds. But if that is the case, don't we need the same and-ing around 
line 19163?

if (tab->pre_idx_push_select_cond)
   orig_select_cond= tab->set_cond(tab->pre_idx_push_select_cond, __LINE__);
   orig_select_cond_saved= true;

As you mention, it is probable that pre_idx_push_select_cond was designed to 
contain every condition required to get a correct result for the query. We know 
now that this is not the case as some conditions have disappeared. We have at 
least three ways to handle this:

1) Do not do ICP until we know for certain which index will be used. 
Conceptually the best option by far, but may require a huge amount of work since 
we may currently change index during exec()
2) Make sure pre_idx_push_select_cond does contain every condition required to 
get a correct result, meaning that the conditions that "disappeared" in your 
test must be appended to it
3) Make sure we restore the missing conditions in test_if_skip_sort_order(), 
either by a) and-ing pre_idx... with orig_select_cond or by b) and-ing 
pre_idx... with the missing conditions (not sure if this is possible).

You should make the two cases above consistent by one of these three methods (or 
one I didn't think about).

Apart from this I'm fine with the patch :)

Olav Sandstaa wrote:
> #At file:///export/home/tmp/mysql/opt-bug52605-v2/ based on
> revid:epotemkin@stripped
>  3203 Olav Sandstaa	2010-06-30
>       Fix for Bug#52605 Adding LIMIT 1 clause to query with complex range predicate
> causes wrong results
>       The cause for the wrong result returned when adding the LIMIT 1 to
>       this query was:
>       * during the optimize phase ICP code pushed part of the WHERE clause
>         down to the storage engine based on that the query should be
>         executed as a range query using the primary key.            
>       * in the start of the execution phase the LIMIT clause is considered
>         and alternative access strategies are evaluated in order to reduce
>         the cost for reading records. This resulted in the switching to a
>         different index for reading the data from the storage engine.
>       As a consequence of this change of access strategy (switching index)
>       the part of the WHERE clause that had been pushed down to the storage
>       engine was never evaluated.
>       The fix for this is to detect that we have switched index and in that
>       case include the condition that was previously been pushed to the
>       storage engine into the condition that is evaluated by the server.
>       Note that this patch also fixes another minor (performance) issue:
>       if the entire where condition was pushed down to the storage engine 
>       then tab->select_cond would be NULL when calling
>       test_if_skip_sort_order(). If this was replaced by the pre-pushed index
>       condition it would never be restored back to NULL. This would result 
>       in that the where condition would be evaluated both by the storage 
>       engine and in the server.

Jørgen Løland
bzr commit into mysql-next-mr-opt-backporting branch (olav:3203) Bug#52605Olav Sandstaa30 Jun
  • Re: bzr commit into mysql-next-mr-opt-backporting branch (olav:3203)Bug#52605Jørgen Løland2 Jul
    • Re: bzr commit into mysql-next-mr-opt-backporting branch (olav:3203)Bug#52605Olav Sandstaa9 Jul