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.
