List:Commits« Previous MessageNext Message »
From:Martin Hansson Date:December 29 2010 10:01am
Subject:Re: bzr commit into mysql-5.1-bugteam branch (sergey.glukhov:3515)
Bug#48916
View as plain text  
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
>> <in_optimizer>((3,2),<exists>(select 4,2 having (((<cache>(3) =
> 4) or
>> isnull(4)) and ((<cache>(2) = 2) or isnull(2)) and
>> <is_not_null_test>(4) and <is_not_null_test>(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

Thread
bzr commit into mysql-5.1-bugteam branch (sergey.glukhov:3515) Bug#48916Sergey Glukhov10 Dec
  • Re: bzr commit into mysql-5.1-bugteam branch (sergey.glukhov:3515)Bug#48916Martin Hansson13 Dec
  • Re: bzr commit into mysql-5.1-bugteam branch (sergey.glukhov:3515)Bug#48916Martin Hansson29 Dec
Re: bzr commit into mysql-5.1-bugteam branch (sergey.glukhov:3515)Bug#48916Martin Hansson29 Dec