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
