List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:September 20 2010 12:16pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3244)
Bug#55955
View as plain text  
Buenos dias Roy,

Roy Lyseng a écrit, Le 20.09.2010 14:14:
> Bon Jour Guilhem,
> 
> please see whether my answers are acceptable, or you need more details!
> 
> On 20.09.10 13.47, Guilhem Bichot wrote:
>> God Dag Roy,
>>
>> Roy Lyseng a écrit, Le 16.09.2010 13:16:
>>> #At file:///home/rl136806/mysql/repo/mysql-work5/ based on
>>> revid:jorgen.loland@stripped
>>>
>>> 3244 Roy Lyseng 2010-09-16
>>> Bug#55955: crash in MEMORY engine with IN(LEFT JOIN (JOIN))
>>> As indicated in the "Suggested fix", the problem is that not all
>>> tables in a semijoin nest has the emb_sj_nest pointer set.
>>> The pointer is only set if the table is contained immediately within
>>> the semijoin nest, but according to the documentation in sql_select.h,
>>> it should be set for all tables within a semijoin nest.
>>> This is also consistent, as a table can never be included in more than
>>> one semijoin nest (semijoin nests cannot be nested).
>>> I have also manually inspected all uses of emb_sj_nest, and it seems 
>>> that
>>> all uses are still consistent (previously there was an implicit assert
>>> that emb_sj_nest == embedding, which no longer holds).
>>> mysql-test/include/subquery_sj.inc
>>> Added test case for bug#55955.
>>> mysql-test/r/subquery_sj_all.result
>>> Semijoin plan changed from DuplicateWeedout to Materialize.
>>
>> Do you know what caused this?
> 
> Not really... The original plan was decided on a wrong basis, as not all 
> semijoin tables were identified as such. Hence I did not think it made 
> sense to investigate why that plan was chosen. However, both the 
> FirstMatch and Materialization choices after the fix seem plausible to me.

ok

>>> === modified file 'sql/sql_select.cc'
>>> --- a/sql/sql_select.cc 2010-09-15 08:53:08 +0000
>>> +++ b/sql/sql_select.cc 2010-09-16 11:16:22 +0000
>>> @@ -4219,9 +4219,6 @@ bool find_eq_ref_candidate(TABLE *table,
>>> - It is accessed via eq_ref(outer_tables)
>>>
>>> POSTCONDITIONS
>>> - * Tables that were pulled out have JOIN_TAB::emb_sj_nest == NULL
>>> - * Tables that were not pulled out have JOIN_TAB::emb_sj_nest 
>>> pointing - to
>>> semi-join nest they are in.
>>> * Semi-join nests' TABLE_LIST::sj_inner_tables is updated accordingly
>>>
>>> This operation is (and should be) performed at each PS execution since
>>> @@ -4263,7 +4260,6 @@ int pull_out_semijoin_tables(JOIN *join)
>>> {
>>> if (tbl->table)
>>> {
>>> - tbl->table->reginfo.join_tab->emb_sj_nest= sj_nest;
>>> if (tbl->table->map & join->const_table_map)
>>> {
>>> pulled_tables |= tbl->table->map;
>>> @@ -4325,15 +4321,8 @@ int pull_out_semijoin_tables(JOIN *join)
>>> {
>>> if (tbl->table)
>>> {
>>> - if (inner_tables & tbl->table->map)
>>> + if (!(inner_tables & tbl->table->map))
>>
>> this if() and the upper if() can be merged in one with &&
> 
> The above two while loops have (almost) the same if()-style. If I change 
> this one, I should probably change the other two as well, and if you 
> consider the diffs generated by this change to be worthwhile, I will do 
> it...

you decide.

>>> + /*
>>> + Set pointer to embedding semijoin nest for all semijoined tables.
>>> + Note that this must be done for every table inside all semijoin nests,
>>> + even for tables within outer join nests embedded in semijoin nests.
>>> + A table can never be part of multiple semijoin nests, hence no
>>> + ambiguities can ever occur.
>>> + Note also that the pointer is not set for TABLE_LIST objects that
>>> + are outer join nests within semijoin nests.
>>> + */
>>> + for (s= stat; s < stat_end; s++)
>>> + {
>>> + TABLE_LIST *tables= s->table->pos_in_table_list;
>>> + while (tables->embedding)
>>> + {
>>> + if (tables->embedding->sj_on_expr)
>>> + {
>>> + s->emb_sj_nest= tables->embedding;
>>> + break;
>>> + }
>>> + tables= tables->embedding;
>>> + }
>>
>> this while() could be, at your option:
>> for(TABLE_LIST *tables= s->table->pos_in_table_list ;
>> tables->embedding ;
>> tables= tables->embedding)
>> { if() etc }
>> The entire block could be a function, at your option.
> 
> Yes, the for loop is better than the while, will do.
> 
> While your suggestion on making this into a function is great, I think I 
> will leave the loop where it is for now, and rather wait until a 
> complete refactoring of make_join_statistics() is performed.

ok

>>> + }
>>> +
>>> join->join_tab=stat;
>>> join->map2table=stat_ref;
>>> join->all_tables= table_vector;
>>
>> By reading code I could not find a case which will break now that we set
>> emb_sj_nest for nested joins too.
>> Ok to push.
>>
> Merci beaucoup,

De nada.
Ok to push again.
Thread
bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3244) Bug#55955Roy Lyseng16 Sep
  • Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3244)Bug#55955Guilhem Bichot20 Sep
    • Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3244)Bug#55955Roy Lyseng20 Sep
      • Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3244)Bug#55955Guilhem Bichot20 Sep