On 12.08.10 11.07, Guilhem Bichot wrote:
> Roy Lyseng a écrit, Le 12.08.2010 10:32:
>>> Minor comments below, nothing critical, ok to push anyway.
>>>> @@ -1362,7 +1360,7 @@ static bool sj_table_is_included(JOIN *j
>>>> It may be possible to consolidate the materialization strategies into
>>>> The choice between the strategies is made by the join optimizer (see
>>>> - advance_sj_state() and
>>>> + advance_sj_state() and fix_semijoin_strategies()).
>>>> This function sets up all fields/structures/etc needed for execution
>>>> for setup/initialization of semi-join materialization which is done in
>>>> setup_sj_materialization() (todo: can't we move that to here also?)
>>>> @@ -4873,6 +4871,17 @@ static bool optimize_semijoin_nests(JOIN
>>>> TABLE_LIST *sj_nest;
>>>> + /*
>>>> + Perform necessary cleanup in case semijoin nests have been used in
>>>> + executions.
>>>> + */
>>> ideally cleanup should be done by the prior executions when they end (that's
>>> supposed to be the "clean house" principle in MySQL code: don't leave it to
>>> inhabitants to clean up after you). I don't know whether it's safely
>> I like that principle too... In this case, I just went for the simple
>> solution, as I wanted a quick fix to see if the solution worked :)
>> Will investigate more...
I am adding reset of sjm.positions to fix_semijoin_strategies...().
sjm-positions is not used in the optimization process after this function.
The sj_mat_info pointer is reset in JOIN::destroy().
But I keep the reset code in optimize_semijoin_nests(), just in case the cleanup
code is never called ;)
>>>> + SJ_MATERIALIZATION_INFO* sjm;
>>>> + if (!(sjm= new SJ_MATERIALIZATION_INFO))
>>>> + DBUG_RETURN(TRUE);
>>> I find "sjm" is confusing: here it's used for a SJ_MATERIALIZATION_INFO
>>> whereas in structure st_semijoin_mat "sjm" is a st_semijoin_mat; and one goal
>>> the patch is to separate both structs.
>> Yes, standalone names must have more expressive names...
>> How about sjm_exec? (sjm_info is just as bad as sjm...)
> yes, sjm_exec is good. Maybe sjm_opt for the optimization-related counterpart?
There is no standalone variable for optimizer sjm struct ;)
>>>> else if (sjm_strategy == SJ_OPT_MATERIALIZE_LOOKUP)
>>>> COST_VECT prefix_cost; - SJ_MATERIALIZATION_INFO *mat_info=
>>>> - signed int first_tab= (int)idx - mat_info->tables;
>>>> + signed int first_tab=
>>> this is not your code, but I wonder whether 'signed' is useful here.
>> I inserted DBUG_ASSERT(first_tab >= 0); and got verification that first_tab
>> may become -1.
> right; it should not be unsigned, and plain "int" would do.