List:Commits« Previous MessageNext Message »
From:Jorgen Loland Date:January 13 2011 10:19am
Subject:Re: bzr commit into mysql-trunk branch (olav.sandstaa:3451) Bug#58838
View as plain text  
Hi Olav,

Thank you for the patch. Since it's not safe to use the Item* generated by 
make_cond_for_table() in both the where condition and pre_idx..., the code is OK.

On 01/03/2011 03:50 PM, Olav Sandstaa wrote:
> #At file:///export/home/tmp/mysql2/opt-bug58838-trunk/ based on
> revid:guilhem.bichot@stripped
>
>   3451 Olav Sandstaa	2011-01-03
>        Fix for Bug#58838 Wrong results with HAVING + LIMIT without GROUP BY
>                          when ICP is enabled.
>
>        The wrong result was caused due to "loosing" the HAVING condition
>        when changing to use a different index than originally selected by the
>        optimizer.
>
>        During the optimization phase it was decided to use a secondary index
>        for retrieving data from the table. Since ICP was enabled the WHERE
>        condition was pushed down to the storage engine. A copy of the WHERE
>        condition was kept in the JOINTAB's pre_idx_push_select_cond
>        member. In the start of the execution phase it was determined that the
>        HAVING condition could be included in the JOINTAB's select
>        condition. Then in test_if_skip_sort_order() we (since part of the
>        select condition has been pushed down to the storage engine) replace
>        the JOINTAB's select condition with the original select condition
>        (found in the pre_idx_push_select_cond member). Since
>        test_if_skip_sort_order() determines that is will be less costly to
>        use the primary key for doing the retrieval of data (due to LIMIT) it
>        changes to use the primary key. If it had not changed index then
>        test_if_skip_sort_order() would have restored the select condition to
>        what it was when it was called. But due to changing index the part of
>        the select condition that has been pushed down must also be included
>        in the select condition. To achieve this the condition in
>        pre_idx_push_select_cond is kept as the JOINTAB's select
>        condition. This does not include the HAVING condition and as a result
>        the HAVING condition is no longer part of the JOINTAB's select
>        condition.
>
>        The fix for this problem: When adding the HAVING condition to the
>        JOINTAB's select condition also add it to the pre_idx_push_select_cond
>        (if this exists).
>       @ mysql-test/include/icp_tests.inc
>          Test case for Bug#58838 "Wrong results with HAVING + LIMIT without
>          GROUP BY when ICP is enabled".
>       @ mysql-test/r/innodb_icp.result
>          Test case for Bug#58838 "Wrong results with HAVING + LIMIT without
>          GROUP BY when ICP is enabled".
>       @ mysql-test/r/innodb_icp_all.result
>          Test case for Bug#58838 "Wrong results with HAVING + LIMIT without
>          GROUP BY when ICP is enabled".
>       @ mysql-test/r/innodb_icp_none.result
>          Test case for Bug#58838 "Wrong results with HAVING + LIMIT without
>          GROUP BY when ICP is enabled".
>       @ mysql-test/r/myisam_icp.result
>          Test case for Bug#58838 "Wrong results with HAVING + LIMIT without
>          GROUP BY when ICP is enabled".
>       @ mysql-test/r/myisam_icp_all.result
>          Test case for Bug#58838 "Wrong results with HAVING + LIMIT without
>          GROUP BY when ICP is enabled".
>       @ mysql-test/r/myisam_icp_none.result
>          Test case for Bug#58838 "Wrong results with HAVING + LIMIT without
>          GROUP BY when ICP is enabled".
>       @ sql/sql_select.cc
>          When including the HAVING condition as part of the JOINTAB's
>          select condition it should also be added to the original
>          select condition (stored in pre_idx_push_select_cond) if
>          parts of the select condition has been pushed down to the storage
>          engine. This is necessary since the pre_idx_push_select_cond
>          is used in test_if_skip_sort_order() and might replace the
>          select condition in the cases where test_if_skip_sort_order()
>          decides to change to use a different index for retrieving data.
>
>      modified:
>        mysql-test/include/icp_tests.inc
>        mysql-test/r/innodb_icp.result
>        mysql-test/r/innodb_icp_all.result
>        mysql-test/r/innodb_icp_none.result
>        mysql-test/r/myisam_icp.result
>        mysql-test/r/myisam_icp_all.result
>        mysql-test/r/myisam_icp_none.result
>        sql/sql_select.cc
> === modified file 'mysql-test/include/icp_tests.inc'
(...)
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-12-29 00:38:59 +0000
> +++ b/sql/sql_select.cc	2011-01-03 14:50:49 +0000
> @@ -3219,6 +3219,29 @@
>   	DBUG_EXECUTE("where",print_where(curr_table->select->cond,
>   					 "select and having",
>                                            QT_ORDINARY););
> +
> +        /*
> +          If we have pushed parts of the condition down to the handler
> +          then we need to add this to the original pre-ICP select
> +          condition since the original select condition may be used in
> +          test_if_skip_sort_order().
> +        */
> +        if (curr_table->pre_idx_push_select_cond)
> +        {
> +          sort_table_cond= make_cond_for_table(curr_join->tmp_having,
> +                                               used_tables, used_tables, 0);
> +          if (!sort_table_cond)
> +            DBUG_VOID_RETURN;
> +          Item* new_pre_idx_push_select_cond=
> +            new Item_cond_and(curr_table->pre_idx_push_select_cond,
> +                              sort_table_cond);
> +          if (!new_pre_idx_push_select_cond)
> +            DBUG_VOID_RETURN;
> +          if (new_pre_idx_push_select_cond->fix_fields(thd, 0))
> +            DBUG_VOID_RETURN;
> +          curr_table->pre_idx_push_select_cond= new_pre_idx_push_select_cond;
> +        }
> +
>   	curr_join->tmp_having= make_cond_for_table(curr_join->tmp_having,
>   						   ~ (table_map) 0,
>   						   ~used_tables, 0);

-- 
Jørgen Løland | Senior Software Engineer | +47 73842138
Oracle MySQL
Trondheim, Norway
Thread
bzr commit into mysql-trunk branch (olav.sandstaa:3451) Bug#58838Olav Sandstaa3 Jan
  • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3451) Bug#58838Jorgen Loland13 Jan
  • Re: bzr commit into mysql-trunk branch (olav.sandstaa:3451) Bug#58838Roy Lyseng25 Jan