From: Martin Hansson Date: December 29 2010 10:01am Subject: Re: bzr commit into mysql-5.1-bugteam branch (sergey.glukhov:3515) Bug#48916 List-Archive: http://lists.mysql.com/commits/127653 Message-Id: <4D1B06E4.8050705@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Hi Sergey, thank you for the extra information, I replied to your commit e-mail separately. Sergey Glukhov wrote: > Hi, > > On 12/13/2010 03:26 PM, Martin Hansson wrote: >> Sergey Glukhov skrev 2010-12-10 13.06: >>> #At file:///home/gluh/MySQL/mysql-5.1-bugteam/ based on >>> revid:bjorn.munch@stripped >>> >>> 3515 Sergey Glukhov 2010-12-10 >>> 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. Extraction of HAVING part does not take into >>> account >>> the fact that some of conditions might be non-const but >>> have 'used_tables' == 0 (undependent subqueries) >>> and because of that these conditions are cut off by >>> make_cond_for_table() function. >>> The fix is to use (table_map) 0 instead of used_tables in >>> third argument for make_cond_for_table() function. >>> It allows to extract elements which belong to sorted >>> table and in addition elements which are undependend >>> subqueries. >> You might want to run a spell-checker on the comment. Misspelled >> change set comments take longer to read. >> >> I don't understand your fix. Ok, I understand that the IN condition >> is non-constant but still doesn't refer to any tables. But why is >> there any difference between sending a constant 0 and a variable >> which is 0? Actually, > The difference is that IN subquery is uncacheable that makes it > non-const. Isn't it more correct to say that it is both non-const and non-cacheable since it is transformed into a correlated EXISTS sub-query? And the fact that it doesn't reference any tables is probably because (3,2) is a constant expression. By the way, why does it have to be a row-valued predicand? What about scalar expressions, e.g. HAVING 2 IN (SELECT f2 FROM t2). I can repeat the bug with that. But I agree that row expressions should be tested as well. > Check Item_in_subselect::row_value_transformer where > we set subquery into UNCACHEABLE state and also check > Item_subselect::fix_fields where we make this subquery > non-const depending on UNCACHEABLE flag. > >> I ran some test on my machine and I get used_tables == 1. Have I >> missed something? > yes, lets look at this condition: > > (3, 2) IN (SELECT 4, 2) AND t1.f1 >= 0 > > First part '(3, 2) IN (SELECT 4, 2)' has used_tables == 0; > Second part 't1.f1 >= 0' has used_tables == 1; > > So COND1->used_tables() | COND2->used_tables() returns 1. My mistake, I confused used_tables for used_table, heh.... > > >> >> >> I ran the test with your fix and as you're saying, the condition >> ((3,2),(select 4,2 having ((((3) = 4) or >> isnull(4)) and (((2) = 2) or isnull(2)) and >> (4) and (2)))) goes missing >> without your fix, but why? > Because make_cond_for_table() with third argument not equal to zero > cuts it off. Probably Roy's comment which is added into > opt_backporting tree could help to understand the function behaviour: That comment is very good, but this one is even better: /* Ignore this condition if 1. We are extracting conditions for a specific table, and 2. that table is not referenced by the condition, and 3. [...] */ if (used_table && // 1 !(cond->used_tables() & used_table) && // 2 ... It probably explains the whole bug, huh? > >> /** >> Extract a condition that can be checked after reading given table >> >> @param cond Condition to analyze >> @param tables Tables for which "current field values" are >> available >> @param used_table Table that we're extracting the condition for (may >> also include PSEUDO_TABLE_BITS, and may be zero) >> @param exclude_expensive_cond Do not push expensive conditions >> >> @retval <>NULL Generated condition >> @retval = NULL Already checked, OR error >> >> @details >> Extract the condition that can be checked after reading the table >> specified in 'used_table', given that current-field values for >> tables >> specified in 'tables' bitmap are available. >> If 'used_table' is 0, extract conditions for all tables in 'tables'. > ^^^ > I would like to add the info that If 'used_table' is 0 the function > also returns conditions which have used_tables() == 0. Why not describe it on a higher level? No need to repeat what the code says. If used_table is 0 <=> we don't extract conditions for any particular table <=> even conditions that don't apply to one particular table are included in the produced condition. I also think you should back-port all applicable comments to 5.1. It seems it would have saved you trouble had they been there when you started out... Best Regards Martin