From: Jorgen Loland Date: January 13 2011 10:19am Subject: Re: bzr commit into mysql-trunk branch (olav.sandstaa:3451) Bug#58838 List-Archive: http://lists.mysql.com/commits/128618 Message-Id: <4D2ED1C0.2000304@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 8bit 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