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;
tmp_cond= and_conds(tmp_cond, orig_select_cond);
/* orig_select_cond was merged, no need to restore original one. */
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
orig_select_cond= tab->set_cond(tab->pre_idx_push_select_cond, __LINE__);
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
> 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.