List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:August 2 2010 11:58am
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch
(roy.lyseng:3211) Bug#43768
View as plain text  
Roy,

Thanks for fixing most of my previous comments.  As mentioned, I have 
two issues with the current patch:

1. The changes to explain output in subquery_sj_mat_nosj.result does
    not seem desirable. Do you know why this happens.

2. Since this patch changes how constant tables are handled with
    prepared statements, I think you should add some tests cases with that
    scenario.

I also have some comments to the commit message, see below

On 07/ 9/10 03:36 PM, Roy Lyseng wrote:
 > #At file:///home/rl136806/mysql/repo/mysql-work4/ based on 
revid:guilhem@stripped
 >
 >   3211 Roy Lyseng	2010-07-09
 >        Bug#43768: Prepared query with nested subqueries core dump on 
second execution
 >
 >        The problem here is that pullout of semijoin tables is 
attempted for
 >        each execution, but because those tables are not "pushed back" 
again
 >        after each execution, the pullout fails on second attempt.
 >
 >        The solution chosen here is to pullout only those semijoin 
tables that are
 >        functionally dependent upon the outer tables. This pullout 
operation
 >        need to be performed only once, and, unlike the current 
procedure, is not
 >        dependent upon the row count of the involved tables.
 >        We still pullout tables that are classified as const tables 
based on row count
 >        if this is a direct execution, though.
 >
 >        The practical implication of this is as follows:
 >
 >         - Only outer tables containing zero or one rows will now be 
extracted
 >           as "const tables". Thus, such tables from a semijoin nest 
are no
 >           longer accessed during optimization, and some (rare) 
optimizations
 >           are no longer possible.
 >
 >         - In the majority of cases, there is no performance impact. 
Often,
 >           the new strategy chosen is Materialization, meaning that 
the row
 >           of these table is accessed only once and saved in local memory.
 >
 >         - Const table analysis now has to be done in two phases:
 >           1) Pullout tables based on dependencies. Both outer and 
inner tables
 >           may apply, and
 >           2) Pullout tables based on row count. Outer tables are 
always pulled out,
 >           inner tables only if inside a non-prepared statement.
 >
 >        In order to implement the latter point above, and assure that 
pullout
 >        of semijoin tables occurs only once, make_join_statistics() 
has been
 >        restructured slightly. The conditional logic within the 
function has also
 >        been enhanced for better readability.
 >
 >        The logic of make_join_statistics() is now as follows:
 >
 >        1. Initialize JOIN data structures
 >           (major part of first loop in existing implementation).
 >
 >        2. Update dependencies based on join information
 >           (the Warshall algorithm).
 >
 >        3. Make key descriptions (update_ref_and_keys()).
 >
 >        4. Pull out semijoin tables, called only once.
 >
 >        5. Extract tables with zero or one rows as const tables
 >           (in prepared mode, consider outer tables only, no semijoin 
tables).
 >
 >        6. Extract dependent tables as const tables.
 >           (in prepared mode, consider outer tables only, no semijoin 
tables).
 >
 >        7. The remaining parts of the function.
 >
 >        mysql-test/r/select_found.result
 >          Possible keys changed from "PRIMARY,kid" to "kid".
 >          Happens because analysis order is slightly changed, but 
harmless as
 >          the table is identified as "const".
 >
 >        mysql-test/r/subselect_sj.result
 >          Added test case for bug#43768
 >          A number of plan changes because of extensive testing of 
semi-join
 >          tables with 0, 1 and 2 rows.
 >
 >        mysql-test/r/subselect_sj_jcl6.result
 >          Ditto.
 >
 >        mysql-test/t/subselect_sj.test
 >          Added test case for bug#43768
 >          bug#46744 now becomes a duplicate of bug#50489, and test 
case is moved.

Please, update these comments to refer to the new test/result files.

 >
 >        mysql-test/suite/optimizer_unfixed_bugs/t/bug46744.test
 >          Test for bug#46744 moved here.
 >
 >        sql/sql_select.cc
 >          pull_out_semijoin_tables()
 >            join_tab->emb_sj_nest is no longer updated. Comments updated.
 >          make_join_statistics()
 >            Removed const table pullout from first loop.
 >            Simplified testing based on inner/outer/semi-join properties.
 >            Calls pull_out_semijoin_tables() just after dependency 
analysis.
 >            Then, added loop that performs pullout based on row count
 >            but excludes semijoined tables in prepared statements.
 >            Second call of pull_out_semijoin_tables() is needed after 
this.
 >            Simplified testing based on inner/outer/semi-join properties.
 >            Semijoin tables are no longer pulled out based on row count in
 >            prepared statements.
 >            Because pull_out_semijoin_tables() is no longer called for 
each
 >            execution, emb_sj_nest is now set in this function.

Last sentence seems a bit strange.  After a bit of thinking I now
realize that "this" refers to make_join_statistics() and not
pull_out_semijoin_tables(), but that was not clear to me when I first
read this.


-- 
Øystein
Thread
bzr commit into mysql-next-mr-opt-backporting branch (roy.lyseng:3211)Bug#43768Roy Lyseng9 Jul
  • Re: bzr commit into mysql-next-mr-opt-backporting branch(roy.lyseng:3211) Bug#43768Øystein Grøvlen2 Aug
    • Re: bzr commit into mysql-next-mr-opt-backporting branch (roy.lyseng:3211)Bug#43768Roy Lyseng6 Aug