List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:September 1 2010 8:52am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)
Bug#48213
View as plain text  
Hello Oystein,

Øystein Grøvlen a écrit, Le 01.09.2010 00:56:
> Hi Guilhem,
> 
> Thanks for the quick and thorough (as always) review.  See my answers 
> inline.
> 
> Guilhem Bichot wrote:
>> Hello Oystein,
>>
>>  > #At 
>> file:///home/oysteing/mysql-new/mysql-next-mr-opt-backporting-2/ based 
>> on revid:epotemkin@stripped
>>  >
>>  >  3232 oystein.grovlen@stripped    2010-08-29
>>  >       Bug#48213 Materialized subselect crashes if using GEOMETRY type
>>  >
>>  >       The problem occurred because during execution of 
>> MaterializationLookup
>>  >       strategy for semi-join, a materialized table was created which
>>  >       contained a GEOMETRY column, a specialized BLOB column.  This 
>> caused
>>  >       an segmentation fault because such tables will have extra 
>> columns,
>>
>> What extra columns were those?
> 
> AFAIU, there will be an extra column which records null/not-null 
> information, for each BLOB column.

ok, I think it's worth mentioning in the comment

>>  >       In addition to adding a new test case for this bug, some 
>> existing
>>  >       tests for subqueries involving BLOB columns have been moved from
>>  >       subquery_mat.inc to subquery_sj.inc.  This way, these tests 
>> will also
>>  >       be run for more cases involving semijoin materialization.
>>
>> Ok. Though some moved tests, which have GROUP BY in the subquery,
>> don't match the textbook's definition of a semijoin. But I see the
>> value of the move, so you can keep it.
> 
> Do you think I should split the tests and only move the queries without 
> aggregation?

don't bother

>> If field_type() is more than a simple inline function, or is virtual,
>> I would cache its value on the stack.
> 
> field_type is a simple inline function, but it calls a virtual function 
> so I guess I should cache it.  However, I guess a good compiler should 
> be able to figure out that by itself.

yes. But in case, I would cache it...

>>
>>  > -      exec_method= EXEC_UNSPECIFIED;
>>  > -      if (left_expr->cols() == 1)
>>  > -        trans_res= 
>> single_value_in_to_exists_transformer(old_engine->join,
>>  > -                                                         
>> &eq_creator);
>>  > -      else
>>  > -        trans_res= 
>> row_value_in_to_exists_transformer(old_engine->join);
>>  > -      res= (trans_res != Item_subselect::RES_OK);
>>  > +      DBUG_RETURN(TRUE);
>>
>> you need to restore the arena
> 
> This is Greek to me (or French for that matter), but I guess you mean:
> 
>     if (arena)
>       thd->restore_active_arena(arena, &backup);

yes. This is for prepared statements.

>> Regarding testing, this in subquery_sj*.result:
>> +SELECT pk FROM t1 WHERE (b, c) IN (SELECT b, c FROM t2 WHERE pk > 0);
>> +pk
>> +1
>> +1
>> looks weird: t1 has only one row with pk=1, here we get two. There are
>> several occurences of this, and in several result files.
> 
> This is, as mentioned in the comment above these test cases, due to 
> Bug#52068.

ok

Ok to push.
Thread
bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213oystein.grovlen29 Aug
  • Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Guilhem Bichot31 Aug
    • Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Øystein Grøvlen1 Sep
      • Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Guilhem Bichot1 Sep
Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Roy Lyseng1 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (oystein.grovlen:3232)Bug#48213Øystein Grøvlen2 Sep