List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:April 11 2011 11:31am
Subject:Re: bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517
View as plain text  
Hi Øystein,

On 11.04.11 11.36, Øystein Grøvlen wrote:
> Hi Roy,
>
> I think changing the meaning of remaining_tables is very good since it makes the
> interface more general. This is also illustrated by the patch since you are able
> to remove semi-join specific code from best_extension_by_limited search().
> AFAICT, you should also be able to simplify the code that compute the constant
> n_tables in greedy_search(), and I suggest you include that in this patch.

It seems that I lost that part when splitting the patch in two. Thank you for 
catching.
>
> However, you are also adding more semi-join dependent code into
> choose_table_order(), and I do not think that is a good idea. In my opinion,
> excluded_tables and join_tables are general concepts that should rather be
> computed by the caller, which knows the context, and passed as parameters. The
> same also holds for jtab_sort_func. This will, in my opinion, give a better
> separation of concerns.

I think there are arguments for both approaches. In either case, I think that it 
is impossible to remove semijoin-related handling from greedy_search. Actually, 
I fear that we need to add more code, in addition to what is already in 
advance_sj_state() and other functions.

I do think that the encapsulation approach is good: The new class is fed with a 
JOIN pointer and a semi-join nest pointer (which may be NULL), and the class 
calculates the additional information needed from these classes.

Now, there are exactly two places where this class is called, one with a 
non-NULL and another with a NULL argument to emb_sjm_nest. Thus, it is possible 
to calculate information derived from emb_sjm_nest externally in exactly one 
place, and thus avoid the if test inside choose_table_order(). Nevertheless, I 
think that it is good to encapsulate this calculation also, and thus hide it 
from the caller. But this is probably a matter of taste.
>
> See also inline for some comments to the commit comments.
>
> On 04/ 6/11 03:27 PM, Roy Lyseng wrote:
>> #At file:///home/rl136806/mysql/repo/mysql-work5/ based on
>> revid:roy.lyseng@stripped
>>
>> 3359 Roy Lyseng 2011-04-06
>> Bug#11822517: Refactor call interface of choose_plan()
>>
>> Part 2 - Change meaning of greedy_search() argument 'remaining_tables'.
>>
>> The purpose of this patch is to make the interface of greedy_search()
>> and related functions simpler.
>> It is also a preparatory step for a simpler test for dependencies when
>> doing semi join materialization together with outer join (WL#5561).
>>
>> The meaning of the argument remaining_tables to greedy_search() is changed:
>> Before, it meant all tables not added to the current join plan,
>> regardless of whether the tables were to be optimized or not.
>> Now, it means all tables remaining to be added.
>
> I think the last sentence needs to be qualified with "by this. The way it since
> now, I do not see what has changes since what is described in the previous
> sentence.

I am not sure that I understand what you mean.
>
>>
>> The difference is important when making a plan for a materialized
>> semi join nest. Before, remaining_tables included also the tables
>> belonging to the query, but not part of the semi join nest being
>> optimized. This makes for simpler implementation of
>> best_extension_by_limited_search() and easier maintenance.
>>
>> However, best_access_path() requires that information about all tables
>> not yet added to the plan be available. We implement this by adding
>> a member variable excluded_tables in the Optimize_join_tables class,
>
> Never heard of this class. I guess you mean Optimize_table_order ...

Oh, names :(
>
>> which contains the set of all tables in the query block that are not
>> being optimized in the current operation.
>>
>> sql/sql_select.cc
>> New member variable excluded_tables is added to Optimize_join_tables.
>>
>> In choose_table_order(), join_tables is calculated as the set of
>> tables to be optimized.
>>
>> In best_extension_by_limited_search(), use of allowed_tables is
>> eliminated, and logic has been slightly simplified.
>>
>> modified:
>> sql/sql_select.cc
>
> ...
>
Thanks,
Roy
Thread
bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517Roy Lyseng6 Apr
  • Re: bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517Guilhem Bichot8 Apr
  • Re: bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517Øystein Grøvlen11 Apr
    • Re: bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517Roy Lyseng11 Apr
      • Re: bzr commit into mysql-trunk branch (roy.lyseng:3359) Bug#11822517Øystein Grøvlen11 Apr