From: Martin Hansson
Date: June 17 2010 11:55am
Subject: Re: bzr commit into mysql-5.1-bugteam branch (Sergey.Glukhov:3432)
 Bug#48916
List-Archive: http://lists.mysql.com/commits/111426
Message-Id: <4C1A0D2F.4090904@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

Hi,
I don't quite understand how the fix works. I inlined some questions below.

Sergey Glukhov wrote:
> #At file:///home/gluh/MySQL/mysql-5.1-bugteam/ based on revid:bjorn.munch@stripped
>
>  3432 Sergey Glukhov	2010-06-16
>       Bug#48916 Server incorrectly processing HAVING clauses with an ORDER BY clause
>       Before sorting HAVING condition is splitted into two parts,
>       first past is a table related condition and the rest of is
>       HAVING part. ...
>   
What is the 'table related' condition? Do you mean the SELECT condition? 
Or related to the temporary table that results from the GROUP BY/ORDER 
BY operation?

past -> part
>       the fact that some of conditions might be non-const but
>       have 'used_tables' equal to zero(particulary IN subselect)
>       and because of that these conditions are cut off by
>       make_cond_for_table() function.
>       The fix is to split extraction of HAVING part into two steps:
>       1. extract condition which does not belong to the sorted table
>   
What is the sorted table? Is there always such a table?
>       2. extract condition which has 'used_tables' equal to zero
>       and depending on result use one or union of them.
>      @ mysql-test/r/having.result
>         test case
>      @ mysql-test/t/having.test
>         test case
>      @ sql/sql_select.cc
>         The fix is to split extraction of HAVING part into two steps:
>         1. extract condition which does not belong to sorted table
>         2. extract condition which has 'used_tables' equal to zero
>         and depending on result use one or union of them.
>
>     modified:
>       mysql-test/r/having.result
>       mysql-test/t/having.test
>       sql/sql_select.cc
> === modified file 'mysql-test/r/having.result'
> --- a/mysql-test/r/having.result	2010-04-12 10:12:20 +0000
> +++ b/mysql-test/r/having.result	2010-06-16 06:55:24 +0000
> @@ -530,3 +530,51 @@ MAX(t2.f2)
>  NULL
>  DROP TABLE t1,t2;
>  End of 5.0 tests
> +#
> +# Bug#48916 Server incorrectly processing HAVING clauses with an ORDER BY clause
> +#
> +CREATE TABLE t1 (f1 INT, f2 INT);
> +INSERT INTO t1 VALUES (1, 0), (2, 1), (3, 2);
> +CREATE TABLE t2 (f1 INT, f2 INT);
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT f1, f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +f1
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT 4, 2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +f1
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT t1.f1, t1.f1) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +f1
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT rand(), rand() FROM t2) AND
> +(3, 2) IN (SELECT f1, f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +f1
> +INSERT INTO t2 VALUES (3, 2);
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT f1, f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +f1
> +1
> +2
> +3
> +DELETE FROM t2;
> +INSERT INTO t2 VALUES (1, 2), (3, 2), (3, 4);
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT f1, f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +f1
> +1
> +2
> +3
> +DROP TABLE t1,t2;
> +End of 5.1 tests
>   
Most of these tests pass even without applying the fix. Is there a 
reason (e.g. code coverage) for having them here?
> === modified file 'mysql-test/t/having.test'
> --- a/mysql-test/t/having.test	2010-04-12 10:12:20 +0000
> +++ b/mysql-test/t/having.test	2010-06-16 06:55:24 +0000
> @@ -544,3 +544,49 @@ ORDER BY t1.f2;
>  DROP TABLE t1,t2;
>  
>  --echo End of 5.0 tests
> +
> +--echo #
> +--echo # Bug#48916 Server incorrectly processing HAVING clauses with an ORDER BY clause
> +--echo #
> +
> +CREATE TABLE t1 (f1 INT, f2 INT);
> +INSERT INTO t1 VALUES (1, 0), (2, 1), (3, 2);
> +CREATE TABLE t2 (f1 INT, f2 INT);
> +
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT f1, f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT 4, 2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT t1.f1, t1.f1) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT rand(), rand() FROM t2) AND
> +       (3, 2) IN (SELECT f1, f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +
> +INSERT INTO t2 VALUES (3, 2);
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT f1, f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +
> +DELETE FROM t2;
> +INSERT INTO t2 VALUES (1, 2), (3, 2), (3, 4);
> +SELECT t1.f1
> +FROM t1
> +HAVING (3, 2) IN (SELECT f1, f2 FROM t2) AND t1.f1  >= 0
> +ORDER BY t1.f1;
> +
> +DROP TABLE t1,t2;
> +
> +--echo End of 5.1 tests
>
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-06-10 20:45:22 +0000
> +++ b/sql/sql_select.cc	2010-06-16 06:55:24 +0000
> @@ -2183,7 +2183,7 @@ JOIN::exec()
>        JOIN_TAB *curr_table= &curr_join->join_tab[curr_join->const_tables];
>        table_map used_tables= (curr_join->const_table_map |
>  			      curr_table->table->map);
> -
> +      DBUG_ASSERT(!(used_tables & (OUTER_REF_TABLE_BIT | RAND_TABLE_BIT)));
>        Item* sort_table_cond= make_cond_for_table(curr_join->tmp_having,
>  						 used_tables,
>  						 used_tables);
> @@ -2207,9 +2207,31 @@ JOIN::exec()
>  	DBUG_EXECUTE("where",print_where(curr_table->select->cond,
>  					 "select and having",
>                                           QT_ORDINARY););
> -	curr_join->tmp_having= make_cond_for_table(curr_join->tmp_having,
> -						   ~ (table_map) 0,
> -						   ~used_tables);
> +
> +        Item* having_non_const_cond=
> +          make_cond_for_table(curr_join->tmp_having, ~(table_map) 0,
> +                              (~used_tables | RAND_TABLE_BIT |
> +                               OUTER_REF_TABLE_BIT));
> +        Item* having_const_cond=
> +          make_cond_for_table(curr_join->tmp_having,
> +                              ~(used_tables | RAND_TABLE_BIT|
> +                                OUTER_REF_TABLE_BIT), (table_map) 0);
>   
What does make_cond_for_table do actually? There is no documentation and 
the parameter names are completely useless.
> +        if (having_non_const_cond && having_const_cond)
> +        {
> +          curr_join->tmp_having= new Item_cond_and(having_non_const_cond,
> +                                                   having_const_cond);
> +          curr_join->tmp_having->fix_fields(thd, 0);
>   
What is the reason for calling fix_fields on the new expression tree? 
This was not done in previous code.
> +        }
> +        else if (having_non_const_cond)
> +          curr_join->tmp_having= having_non_const_cond;
> +        else if (having_const_cond)
> +          curr_join->tmp_having= having_const_cond;
> +        else
> +        {
> +          DBUG_ASSERT(!(curr_join->tmp_having->used_tables() & ~used_tables) &&
> +                      !having_const_cond);
> +          curr_join->tmp_having= 0;
>   
Please use NULL instead of 0 for NULL pointers.

Otherwise, the fix looks good

Best Regards,

Martin