From: Roy Lyseng
Date: January 25 2011 12:07pm
Subject: Re: bzr commit into mysql-trunk branch (olav.sandstaa:3451) Bug#58838
List-Archive: http://lists.mysql.com/commits/129551
Message-Id: <4D3EBD0D.9060602@oracle.com>
MIME-Version: 1.0
Content-Type: multipart/alternative;
 boundary="------------020806040807050201030307"

--------------020806040807050201030307
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi Olav,

thank you for the fix, the patch is approved.

Roy

On 03.01.11 15.50, 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 'mysql-test/include/icp_tests.inc'
> --- a/mysql-test/include/icp_tests.inc	2010-12-13 15:22:45 +0000
> +++ b/mysql-test/include/icp_tests.inc	2011-01-03 14:50:49 +0000
> @@ -653,3 +653,28 @@
>   insert into t1 values ('',1);
>   select 1 from t1 where b<= 1 and a<>  '';
>   drop table t1;
> +
> +--echo #
> +--echo # Bug#58838 "Wrong results with HAVING + LIMIT without GROUP BY when
> +--echo #            ICP is enabled"
> +--echo #
> +
> +CREATE TABLE t1 (
> +  pk INT NOT NULL,
> +  c1 INT,
> +  PRIMARY KEY (pk),
> +  KEY col_int_key (c1)
> +);
> +
> +INSERT INTO t1 VALUES (1,37);
> +INSERT INTO t1 VALUES (2,8);
> +INSERT INTO t1 VALUES (3,-25);
> +INSERT INTO t1 VALUES (4,NULL);
> +INSERT INTO t1 VALUES (5,55);
> +
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 0;
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 1;
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 2;
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 5;
> +
> +DROP TABLE t1;
>
> === modified file 'mysql-test/r/innodb_icp.result'
> --- a/mysql-test/r/innodb_icp.result	2010-12-13 15:22:45 +0000
> +++ b/mysql-test/r/innodb_icp.result	2011-01-03 14:50:49 +0000
> @@ -598,5 +598,32 @@
>   select 1 from t1 where b<= 1 and a<>  '';
>   1
>   drop table t1;
> +#
> +# Bug#58838 "Wrong results with HAVING + LIMIT without GROUP BY when
> +#            ICP is enabled"
> +#
> +CREATE TABLE t1 (
> +pk INT NOT NULL,
> +c1 INT,
> +PRIMARY KEY (pk),
> +KEY col_int_key (c1)
> +);
> +INSERT INTO t1 VALUES (1,37);
> +INSERT INTO t1 VALUES (2,8);
> +INSERT INTO t1 VALUES (3,-25);
> +INSERT INTO t1 VALUES (4,NULL);
> +INSERT INTO t1 VALUES (5,55);
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 0;
> +pk
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 1;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 2;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 5;
> +pk
> +3
> +DROP TABLE t1;
>   set default_storage_engine= @save_storage_engine;
>   set optimizer_switch=default;
>
> === modified file 'mysql-test/r/innodb_icp_all.result'
> --- a/mysql-test/r/innodb_icp_all.result	2010-12-13 15:22:45 +0000
> +++ b/mysql-test/r/innodb_icp_all.result	2011-01-03 14:50:49 +0000
> @@ -598,5 +598,32 @@
>   select 1 from t1 where b<= 1 and a<>  '';
>   1
>   drop table t1;
> +#
> +# Bug#58838 "Wrong results with HAVING + LIMIT without GROUP BY when
> +#            ICP is enabled"
> +#
> +CREATE TABLE t1 (
> +pk INT NOT NULL,
> +c1 INT,
> +PRIMARY KEY (pk),
> +KEY col_int_key (c1)
> +);
> +INSERT INTO t1 VALUES (1,37);
> +INSERT INTO t1 VALUES (2,8);
> +INSERT INTO t1 VALUES (3,-25);
> +INSERT INTO t1 VALUES (4,NULL);
> +INSERT INTO t1 VALUES (5,55);
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 0;
> +pk
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 1;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 2;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 5;
> +pk
> +3
> +DROP TABLE t1;
>   set default_storage_engine= @save_storage_engine;
>   set optimizer_switch=default;
>
> === modified file 'mysql-test/r/innodb_icp_none.result'
> --- a/mysql-test/r/innodb_icp_none.result	2010-12-13 15:22:45 +0000
> +++ b/mysql-test/r/innodb_icp_none.result	2011-01-03 14:50:49 +0000
> @@ -597,5 +597,32 @@
>   select 1 from t1 where b<= 1 and a<>  '';
>   1
>   drop table t1;
> +#
> +# Bug#58838 "Wrong results with HAVING + LIMIT without GROUP BY when
> +#            ICP is enabled"
> +#
> +CREATE TABLE t1 (
> +pk INT NOT NULL,
> +c1 INT,
> +PRIMARY KEY (pk),
> +KEY col_int_key (c1)
> +);
> +INSERT INTO t1 VALUES (1,37);
> +INSERT INTO t1 VALUES (2,8);
> +INSERT INTO t1 VALUES (3,-25);
> +INSERT INTO t1 VALUES (4,NULL);
> +INSERT INTO t1 VALUES (5,55);
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 0;
> +pk
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 1;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 2;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 5;
> +pk
> +3
> +DROP TABLE t1;
>   set default_storage_engine= @save_storage_engine;
>   set optimizer_switch=default;
>
> === modified file 'mysql-test/r/myisam_icp.result'
> --- a/mysql-test/r/myisam_icp.result	2010-12-13 15:22:45 +0000
> +++ b/mysql-test/r/myisam_icp.result	2011-01-03 14:50:49 +0000
> @@ -596,4 +596,31 @@
>   select 1 from t1 where b<= 1 and a<>  '';
>   1
>   drop table t1;
> +#
> +# Bug#58838 "Wrong results with HAVING + LIMIT without GROUP BY when
> +#            ICP is enabled"
> +#
> +CREATE TABLE t1 (
> +pk INT NOT NULL,
> +c1 INT,
> +PRIMARY KEY (pk),
> +KEY col_int_key (c1)
> +);
> +INSERT INTO t1 VALUES (1,37);
> +INSERT INTO t1 VALUES (2,8);
> +INSERT INTO t1 VALUES (3,-25);
> +INSERT INTO t1 VALUES (4,NULL);
> +INSERT INTO t1 VALUES (5,55);
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 0;
> +pk
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 1;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 2;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 5;
> +pk
> +3
> +DROP TABLE t1;
>   set optimizer_switch=default;
>
> === modified file 'mysql-test/r/myisam_icp_all.result'
> --- a/mysql-test/r/myisam_icp_all.result	2010-12-13 15:22:45 +0000
> +++ b/mysql-test/r/myisam_icp_all.result	2011-01-03 14:50:49 +0000
> @@ -596,4 +596,31 @@
>   select 1 from t1 where b<= 1 and a<>  '';
>   1
>   drop table t1;
> +#
> +# Bug#58838 "Wrong results with HAVING + LIMIT without GROUP BY when
> +#            ICP is enabled"
> +#
> +CREATE TABLE t1 (
> +pk INT NOT NULL,
> +c1 INT,
> +PRIMARY KEY (pk),
> +KEY col_int_key (c1)
> +);
> +INSERT INTO t1 VALUES (1,37);
> +INSERT INTO t1 VALUES (2,8);
> +INSERT INTO t1 VALUES (3,-25);
> +INSERT INTO t1 VALUES (4,NULL);
> +INSERT INTO t1 VALUES (5,55);
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 0;
> +pk
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 1;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 2;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 5;
> +pk
> +3
> +DROP TABLE t1;
>   set optimizer_switch=default;
>
> === modified file 'mysql-test/r/myisam_icp_none.result'
> --- a/mysql-test/r/myisam_icp_none.result	2010-12-13 15:22:45 +0000
> +++ b/mysql-test/r/myisam_icp_none.result	2011-01-03 14:50:49 +0000
> @@ -595,4 +595,31 @@
>   select 1 from t1 where b<= 1 and a<>  '';
>   1
>   drop table t1;
> +#
> +# Bug#58838 "Wrong results with HAVING + LIMIT without GROUP BY when
> +#            ICP is enabled"
> +#
> +CREATE TABLE t1 (
> +pk INT NOT NULL,
> +c1 INT,
> +PRIMARY KEY (pk),
> +KEY col_int_key (c1)
> +);
> +INSERT INTO t1 VALUES (1,37);
> +INSERT INTO t1 VALUES (2,8);
> +INSERT INTO t1 VALUES (3,-25);
> +INSERT INTO t1 VALUES (4,NULL);
> +INSERT INTO t1 VALUES (5,55);
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 0;
> +pk
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 1;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 2;
> +pk
> +3
> +SELECT pk FROM t1 WHERE c1<>  1 HAVING pk = 3 ORDER BY pk LIMIT 5;
> +pk
> +3
> +DROP TABLE t1;
>   set optimizer_switch=default;
>
> === 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);
>
>
>
>


--------------020806040807050201030307--