List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:August 6 2010 10:57am
Subject:Re: bzr commit into mysql-next-mr-opt-backporting branch (roy.lyseng:3211)
Bug#43768
View as plain text  
On 02.08.10 13.58, Øystein Grøvlen wrote:
> 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.

I guess any change is undesirable, but these two "plans" are equivalent, and the 
new one with only one line is probably the best description of the plan (the 
first table is empty, hence all following tables will be skipped).

The reason for the change is rather complex: I no longer delete LEFT JOIN 
dependencies if the right-hand table is empty (make_join_statistics, line 4430). 
The reasoning behind this is that this is a functional dependency, and I try to 
retain all functional dependencies because they persist any row count variations.

The continue statement after clearing s->dependent is executed, and hence the 
table is not added to the outer_join map.

In JOIN::optimize, line 1862, there is a test: if (!conds && outer_join), which 
creates an "always true" condition if this is an outer join without any WHERE 
clause. The new code generates this condition.

Later we have the test if (conds && const_table_map != found_const_table_map
&&
(select_options & SELECT_DESCRIBE) that replaces the "always true" conds with 
"always false" (sic).

Then we call make_join_select, which identifies this as a "const" cond, returns 
with 1 and the caller sets zero_result_cause.

JOIN::exec then sees zero_result_cause, which calls return_zero_rows, which 
calls select_describe to write only a single line.

In the existing version, make_join_select is called with conds=NULL, which means 
that the "if (cond)" block is not entered, and it fails to notice that this 
query is "always false". The function returns 0, and zero_result_cause is not set.

I would say that the old behaviour is a flaw, because execution will terminate 
after first table when that table contains zero rows in any case. It means that 
if you really want to see the full plan, you need to populate your tables with 
some real data, of course...

There is still a flaw here. Unless conds=NULL to start with, a full plan will 
always be reported.
>
> 2. Since this patch changes how constant tables are handled with
> prepared statements, I think you should add some tests cases with that
> scenario.

Proposal made after private discussions.
>
> 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.

OK
>
>  >
>  > 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.
>
>

Will try to clarify.

Thanks!
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