Hi! Here is part 1 of the review; part 2 should come later today.

...

...

...

...

"Sergey" == Sergey Petrunya <psergey@askmonty.org> writes:

Sergey> On Tue, Feb 22, 2011 at 05:24:37PM +0300, Sergey Petrunya wrote:

...

Hi Monty,

Please find attached the complete patch of MWL#90 for review. The tree with the code is on launchpad/buildbot: https://code.launchpad.net/~maria-captains/maria/5.3-subqueries-mwl90 http://buildbot.askmonty.org/buildbot/grid?branch=5.3-subqueries-mwl90

<cut>

+++ maria-5.3-subqueries-r36-noc/mysql-test/include/mix1.inc 2011-02-16 14:42:52.000000000 +0300 @@ -1532,6 +1532,12 @@

SELECT 1 FROM (SELECT COUNT(DISTINCT c1) FROM t1 WHERE c2 IN (1, 1) AND c3 = 2 GROUP BY c2) x; +--echo # MariaDB note: +--echo # This will show 2 for table which has 5 rows. +--echo # This is because the access method employed is actually range access +--echo # which scans 2 records (yes, EXPLAIN displays it incorrectly). +--echo # our correct printing is an artifact of changing in select_describe() +--echo # from printing table->starts.records() to tab->records.

Please remove comment as this is not an issue anymore.

diff -urN --exclude='.*' 5.3-noc/sql/item_subselect.cc maria-5.3-subqueries-r36-noc/sql/item_subselect.cc

@@ -4067,6 +4078,17 @@ }

+int subselect_hash_sj_engine::optimize() +{ + int res= 0;

Don't reset res

+ SELECT_LEX *save_select= thd->lex->current_select; + thd->lex->current_select= materialize_join->select_lex; + res= materialize_join->optimize(); + thd->lex->current_select= save_select; + + return res; +} +

diff -urN --exclude='.*' 5.3-noc/sql/item_subselect.h maria-5.3-subqueries-r36-noc/sql/item_subselect.h --- 5.3-noc/sql/item_subselect.h 2011-01-27 21:39:33.000000000 +0300 +++ maria-5.3-subqueries-r36-noc/sql/item_subselect.h 2011-02-16 14:42:52.000000000 +0300 @@ -46,16 +46,17 @@ < child_join->prepare < engine->prepare *ref= substitution; + substitution= NULL; < Item_subselect::fix_fields */ - Item *substitution; public: + Item *substitution; /* unit of subquery */ st_select_lex_unit *unit; -protected: Item *expr_cache; /* engine that perform execution of subselect (single select or union) */ subselect_engine *engine; +protected: /* old engine if engine was changed */ subselect_engine *old_engine; /* cache of used external tables */ @@ -148,6 +149,7 @@ bool mark_as_dependent(THD *thd, st_select_lex *select, Item *item); void fix_after_pullout(st_select_lex *new_parent, Item **ref); void recalc_used_tables(st_select_lex *new_parent, bool after_pullout); + virtual int optimize(); virtual bool exec(); virtual void fix_length_and_dec(); table_map used_tables() const; @@ -351,7 +353,9 @@ all JOIN in UNION */ Item *expr; +public: Item_in_optimizer *optimizer; +protected:

Move the above to after Item *substitions; No reason to have many :public :protected blocks

@@ -397,6 +401,16 @@ }; enum_exec_method exec_method;

+ /* + TRUE<=>this is a flattenable semi-join, false overwise. + */ + bool is_flattenable_semijoin;

You should set this to zero in Item_subselect::Item_subselect() !

+ + /* + Cost to populate the temporary table (set on if-needed basis). + */ + //double startup_cost; +

Remove startup_cost as it's not used <cut>

@@ -752,7 +767,7 @@

class subselect_hash_sj_engine : public subselect_engine { -protected: +public: /* The table into which the subquery is materialized. */ TABLE *tmp_table; /* TRUE if the subquery was materialized into a temp table. */ @@ -764,14 +779,16 @@ of subselect_single_select_engine::[prepare | cols]. */ subselect_single_select_engine *materialize_engine; +protected: /* The engine used to compute the IN predicate. */ subselect_engine *lookup_engine; /* QEP to execute the subquery and materialize its result into a temporary table. Created during the first call to exec(). */ +public: JOIN *materialize_join; - +protected: /* Keyparts of the only non-NULL composite index in a rowid merge. */ MY_BITMAP non_null_key_parts; /* Keyparts of the single column indexes with NULL, one keypart per index. */ @@ -784,7 +801,9 @@ IN results because index lookups sometimes match values that are actually not equal to the search key in SQL terms. */ +public: Item_cond_and *semi_join_conds; +protected: /* Possible execution strategies that can be used to compute hash semi-join.*/

Please move all public variables to one block.

+++ maria-5.3-subqueries-r36-noc/sql/opt_subselect.cc 2011-02-16 14:42:52.000000000 +0300 @@ -166,6 +328,7 @@ (void)subquery_types_allow_materialization(in_subs);

in_subs->emb_on_expr_nest= thd->thd_marker.emb_on_expr_nest; + in_subs->is_flattenable_semijoin= TRUE;

/* Register the subquery for further processing in flatten_subqueries() */ select_lex-> @@ -220,10 +383,24 @@ (in_subs->is_top_level_item() || optimizer_flag(thd, OPTIMIZER_SWITCH_PARTIAL_MATCH_ROWID_MERGE) || optimizer_flag(thd, OPTIMIZER_SWITCH_PARTIAL_MATCH_TABLE_SCAN)) &&//4 - !in_subs->is_correlated && // 5 - in_subs->exec_method == Item_in_subselect::NOT_TRANSFORMED) // 6 + !in_subs->is_correlated) // 5 { + if (in_subs->exec_method == Item_in_subselect::NOT_TRANSFORMED) in_subs->exec_method= Item_in_subselect::MATERIALIZATION;

You removed //6 comment without updating the comment for the block that describes 6. Please update comment!

+ + /* + If the subquery is an AND-part of WHERE register for being processed + with jtbm strategy + */ + if (in_subs->exec_method == Item_in_subselect::MATERIALIZATION && + thd->thd_marker.emb_on_expr_nest == (TABLE_LIST*)0x1 && + optimizer_flag(thd, OPTIMIZER_SWITCH_SEMIJOIN))

Instead of doing: thd->thd_marker.emb_on_expr_nest == (TABLE_LIST*)0x1 please create an inline function with a meningfull name to test test or at least replace (TABLE_LIST*) 0x with a define or both.

+ { + in_subs->emb_on_expr_nest= thd->thd_marker.emb_on_expr_nest;

Here you could make things cleared and set in_subs->emb_on_expr_nest to a define. <cut>

@@ -339,6 +516,69 @@

/* + Finalize IN->EXISTS conversion in case we couldn't use materialization. + + DESCRIPTION Invoke the IN->EXISTS converter + Replace the Item_in_subselect with its wrapper Item_in_optimizer in WHERE. + + RETURN + FALSE - Ok + TRUE - Fatal error +*/ + +static +bool make_in_exists_conversion(THD *thd, JOIN *join, Item_in_subselect *item) +{ + DBUG_ENTER("make_in_exists_conversion"); + JOIN *child_join= item->unit->first_select()->join; + Item_subselect::trans_res res; + item->changed= 0;

Please add a comment why you set item->fixed to 0 as this is not obvious

+ item->fixed= 0; + + SELECT_LEX *save_select_lex= thd->lex->current_select; + thd->lex->current_select= item->unit->first_select(); + + res= item->select_transformer(child_join); + + thd->lex->current_select= save_select_lex; + + if (res == Item_subselect::RES_ERROR) + DBUG_RETURN(TRUE); + + item->changed= 1; + item->fixed= 1; + + Item *substitute= item->substitution; + bool do_fix_fields= !item->substitution->fixed;

item->substitution -> substitute

+ /* + The Item_subselect has already been wrapped with Item_in_optimizer, so we + should search for item->optimizer, not 'item'. + */ + Item *replace_me= item->optimizer; + DBUG_ASSERT(replace_me==substitute); + + Item **tree= (item->emb_on_expr_nest == (TABLE_LIST*)1)? + &join->conds : &(item->emb_on_expr_nest->on_expr); + if (replace_where_subcondition(join, tree, replace_me, substitute, + do_fix_fields)) + DBUG_RETURN(TRUE); + item->substitution= NULL; +

The code belove makes a permanent change to the prepared statement tree. Please add a comment about this! Also please add a test where you do a prepared statement that hits his code and excecute it twice with different parameters.

+ if (!thd->stmt_arena->is_conventional()) + { + tree= (item->emb_on_expr_nest == (TABLE_LIST*)1)? + &join->select_lex->prep_where : + &(item->emb_on_expr_nest->prep_on_expr); + + if (replace_where_subcondition(join, tree, replace_me, substitute, + FALSE)) + DBUG_RETURN(TRUE); + } + DBUG_RETURN(FALSE); +} + + +/* Convert semi-join subquery predicates into semi-join join nests

SYNOPSIS @@ -445,25 +685,41 @@ // #tables-in-parent-query + #tables-in-subquery < MAX_TABLES /* Replace all subqueries to be flattened with Item_int(1) */ arena= thd->activate_stmt_arena_if_needed(&backup); - for (in_subq= join->sj_subselects.front(); - in_subq != in_subq_end && - join->tables + (*in_subq)->unit->first_select()->join->tables < MAX_TABLES; - in_subq++) - { - Item **tree= ((*in_subq)->emb_on_expr_nest == (TABLE_LIST*)1)? - &join->conds : &((*in_subq)->emb_on_expr_nest->on_expr); - if (replace_where_subcondition(join, tree, *in_subq, new Item_int(1), - FALSE)) - DBUG_RETURN(TRUE); /* purecov: inspected */ - }

for (in_subq= join->sj_subselects.front(); in_subq != in_subq_end && join->tables + (*in_subq)->unit->first_select()->join->tables < MAX_TABLES; in_subq++) { - if (convert_subq_to_sj(join, *in_subq)) - DBUG_RETURN(TRUE); + bool remove_item= TRUE; + if ((*in_subq)->is_flattenable_semijoin) + { + if (convert_subq_to_sj(join, *in_subq)) + DBUG_RETURN(TRUE); + } + else + { + if (convert_subq_to_jtbm(join, *in_subq, &remove_item)) + DBUG_RETURN(TRUE); + } + if (remove_item) + { + Item **tree= ((*in_subq)->emb_on_expr_nest == (TABLE_LIST*)1)? + &join->conds : &((*in_subq)->emb_on_expr_nest->on_expr); + Item *replace_me= *in_subq; + /* + JTBM: the subquery was already mapped with Item_in_optimizer, so we + should search for that, not for original Item_in_subselect. + TODO: what about delaying that rewrite until here? + */ + if (!(*in_subq)->is_flattenable_semijoin) + { + replace_me= (*in_subq)->optimizer; + }

Consider doing a function: Item_in_subselect::original_item() { return is_flattenable_semijoin ? self : item->optimizer; } This would make the above code a bit easier to understand by doing: Item *replace_me= (*in_subq)->original_item() This would thus be a bit like real_item()

+ if (replace_where_subcondition(join, tree, replace_me, new Item_int(1), + FALSE)) + DBUG_RETURN(TRUE); /* purecov: inspected */ + } } skip_conversion: /* @@ -494,7 +750,19 @@ bool do_fix_fields= !(*in_subq)->substitution->fixed; Item **tree= ((*in_subq)->emb_on_expr_nest == (TABLE_LIST*)1)? &join->conds : &((*in_subq)->emb_on_expr_nest->on_expr); - if (replace_where_subcondition(join, tree, *in_subq, substitute, + + Item *replace_me= *in_subq; + /* + JTBM: the subquery was already mapped with Item_in_optimizer, so we + should search for that, not for original Item_in_subselect. + TODO: what about delaying that rewrite until here? + */ + if (!(*in_subq)->is_flattenable_semijoin) + { + replace_me= (*in_subq)->optimizer; + }

The above code can also be made easier with (*in_subq)->original_item(). The above comment is a good one for original_item()!

+ +/* + Get #output_rows and scan_time estimates for a "delayed" table. + + SYNOPSIS + get_delayed_table_estimates() + table IN Table to get estimates for + out_rows OUT E(#rows in the table) + scan_time OUT E(scan_time). + startup_cost OUT cost to populate the table. + + DESCRIPTION + Get #output_rows and scan_time estimates for a "delayed" table. By + "delayed" here we mean that the table is filled at the start of query + execution. This means that the optimizer can't use table statistics to + get #rows estimate for it, it has to call this function instead. + + This function is expected to make different actions depending on the nature + of the table. At the moment there is only one kind of delayed tables, + non-flattenable semi-joins. +*/ + +void get_delayed_table_estimates(TABLE *table, + ha_rows *out_rows, + double *scan_time, + double *startup_cost) +{ + Item_in_subselect *item= table->pos_in_table_list->jtbm_subselect; + item->optimize(); + + DBUG_ASSERT(item->engine->engine_type() == + subselect_engine::HASH_SJ_ENGINE); + + subselect_hash_sj_engine *hash_sj_engine= + ((subselect_hash_sj_engine*)item->engine); + JOIN *join= hash_sj_engine->materialize_join; + + double rows= 1; + double read_time= 0.0; + + /* Calculate #rows and cost of join execution */ + for (uint i= join->const_tables; i < join->tables; i++) + { + rows *= join->best_positions[i].records_read; + read_time += join->best_positions[i].read_time; + } + *out_rows= (ha_rows)rows; + *startup_cost= read_time;

The above loop is identical to doing: get_partial_join_cost(join, join->table, startup_cost, &rows); *out_rows= (ha_rows) rows;

+ /* Calculate cost of scanning the temptable */ + double data_size= rows * hash_sj_engine->tmp_table->s->reclength; + /* Do like in handler::read_time */ + *scan_time= data_size/IO_SIZE + 2; +}

+ +/* + Convert subquery predicate into non-mergeable semi-join nest. + + TODO: + why does this do IN-EXISTS conversion? Can't we unify it with mergeable + semi-joins? currently, convert_subq_to_sj() cannot fail to convert (unless + fatal errors) + + + RETURN + FALSE - Ok + TRUE - Fatal error +*/ + +static bool convert_subq_to_jtbm(JOIN *parent_join, + Item_in_subselect *subq_pred, + bool *remove_item) +{ + SELECT_LEX *parent_lex= parent_join->select_lex; + List<TABLE_LIST> *emb_join_list= &parent_lex->top_join_list; + TABLE_LIST *emb_tbl_nest= NULL; // will change when we learn to handle outer joins + TABLE_LIST *tl; + DBUG_ENTER("convert_subq_to_jtbm"); + + if (subq_pred->setup_engine(TRUE)) + DBUG_RETURN(TRUE); +

/* Please ensure that the code below is covered with some test case !*/

+ if (subq_pred->engine->engine_type() != subselect_engine::HASH_SJ_ENGINE) + { + *remove_item= FALSE; + make_in_exists_conversion(parent_join->thd, parent_join, subq_pred);

You should return value of make_in_exists_conversion() as this may fail...

+ DBUG_RETURN(FALSE); + }

+ *remove_item= TRUE; + + TABLE_LIST *jtbm; + char *tbl_alias; + const char alias_mask[]="<subquery%d>"; + if (!(tbl_alias= (char*)parent_join->thd->calloc(sizeof(alias_mask)+5)) || + !(jtbm= alloc_join_nest(parent_join->thd))) //todo: this is not a join nest! + { + DBUG_RETURN(TRUE); + } + + jtbm->join_list= emb_join_list; + jtbm->embedding= emb_tbl_nest; + jtbm->jtbm_subselect= subq_pred; + jtbm->nested_join= NULL; + + /* Nests do not participate in those 'chains', so: */ + /* jtbm->next_leaf= jtbm->next_local= jtbm->next_global == NULL*/ + emb_join_list->push_back(jtbm); + + /* + Inject the jtbm table into TABLE_LIST::next_leaf list, so that + make_join_statistics() and co. can find it. + */ + for (tl= parent_lex->leaf_tables; tl->next_leaf; tl= tl->next_leaf) ;

Add an explicit {} on an empty row, as otherwise you may get compiler warnings from some compilers

+ tl->next_leaf= jtbm; + + /* + Same as above for TABLE_LIST::next_local chain + (a theory: a next_local chain always starts with ::leaf_tables + because view's tables are inserted after the view) + */ + for (tl= parent_lex->leaf_tables; tl->next_local; tl= tl->next_local) ;

Add an explicit {} on an empty row...

+ tl->next_local= jtbm;

Do you really have to put the table last ? It would be much esier to put it first...

+ + /* A theory: no need to re-connect the next_global chain */ + + subselect_hash_sj_engine *hash_sj_engine= + ((subselect_hash_sj_engine*)subq_pred->engine); + jtbm->table= hash_sj_engine->tmp_table; + + jtbm->table->tablenr= parent_join->tables; + jtbm->table->map= table_map(1) << (parent_join->tables); + + parent_join->tables++; +

Where is it checked that this doesn't exceed MAX_TABLES? Add a comment for the function that guarantees this and an DBUG_ASSERT

+ Item *conds= hash_sj_engine->semi_join_conds; + conds->fix_after_pullout(parent_lex, &conds); + + DBUG_EXECUTE("where", print_where(conds,"SJ-EXPR", QT_ORDINARY);); + + my_snprintf(tbl_alias, sizeof(alias_mask)+5, alias_mask, + hash_sj_engine->materialize_join->select_lex->select_number);

A fast way to generate the name would be to use the following function: #define subquery_table_name_length= sizeof("<subquery9999>"); void create_temporary_subquery_table_name(char *to, uint number) { DBUG_ASSERT(number < 10000); to= strmov(to, "<subquery"); to= int10_to_str((int) number, to, 10); to[0]= '>'; to[1]= 0; } This is significantly faster than my_snprintf(). This would also simplify the other code you have when you generate the name.

+ jtbm->alias= tbl_alias; + + /* Inject sj_on_expr into the parent's WHERE or ON */ + if (emb_tbl_nest) + { + DBUG_ASSERT(0); + /*emb_tbl_nest->on_expr= and_items(emb_tbl_nest->on_expr, + sj_nest->sj_on_expr); + emb_tbl_nest->on_expr->fix_fields(parent_join->thd, &emb_tbl_nest->on_expr); + */

Please remove the comment and change the above to: DBUG_ASSERT(!emb_tbl_nest);

+enum_nested_loop_state join_tab_execution_startup(JOIN_TAB *tab) +{

<cut>

+ else if (tab->bush_children) + { + /* It's a merged SJM nest */ + enum_nested_loop_state rc; + JOIN *join= tab->join; + SJ_MATERIALIZATION_INFO *sjm= tab->bush_children->start->emb_sj_nest->sj_mat_info; + JOIN_TAB *join_tab= tab->bush_children->start; + JOIN_TAB *save_return_tab= join->return_tab;

move setting of the last 2 variables inside the loop belpw

--- 5.3-noc/sql/sql_base.cc 2011-01-27 21:39:33.000000000 +0300 +++ maria-5.3-subqueries-r36-noc/sql/sql_base.cc 2011-02-21 18:15:06.000000000 +0300 @@ -7778,6 +7778,18 @@ if (res) DBUG_RETURN(1); } + if (table_list->jtbm_subselect) + { + Item *item= table_list->jtbm_subselect; + if (item->fix_fields(thd, &item)) + { + my_error(ER_TOO_MANY_TABLES,MYF(0),MAX_TABLES); + DBUG_RETURN(1); + } + DBUG_ASSERT(item == table_list->jtbm_subselect); + if (table_list->jtbm_subselect->setup_engine(FALSE)) + DBUG_RETURN(1); + } }

Regards, Monty

Hi! Here is the third and last part of the review.

...

...

...

...

"Sergey" == Sergey Petrunya <psergey@askmonty.org> writes:

<cut>

@@ -7433,8 +7718,18 @@

table_map used_tables2= (join->const_table_map | OUTER_REF_TABLE_BIT | RAND_TABLE_BIT); - for (tab= join->join_tab+join->const_tables; tab <= last_tab ; tab++) + for (JOIN_TAB *tab= first_linear_tab(join, TRUE); + tab; + tab= next_linear_tab(join, tab, TRUE)) { + if (!tab->table) + { + /* + psergey-todo: this is probably incorrect, fix this when we get + correct processing for outer joins + semi joins + */ + continue; + }

When is the above fix going to happen ? (Which worklog). What is the effect if we execute the continue? a) Wrong result b) Crash 3) Slower code ? Please add a comment about this if it's 3) If it's 1 or 2 we should fix it before pushing to trunk.

@@ -7480,37 +7775,12 @@ (*sel_cond_ref)->update_used_tables(); if (cond_tab->select) cond_tab->select->cond= cond_tab->select_cond; - } + } + if (tab == last_tab) + break;

Wouldn't it be better to fix the 'for' loop, as otherwise you may get a crash if tab->table is not true for the tab == last_tab. How about this for a better for loop for (JOIN_TAB *tab= first_linear_tab(join, TRUE), prev_tab= 0; prev_tab != last_tab; prev_tab= tab, tab = next_linear_tab(join, tab, TRUE)) Note that we don't have to test for tab == 0 as we will always find last_tab first.

@@ -7968,8 +8240,8 @@ Don't use join buffering if we're dictated not to by no_jbuf_after (this ...) */ - if (!(i <= no_jbuf_after) || tab->loosescan_match_tab || - sj_is_materialize_strategy(join->best_positions[i].sj_strategy)) + if ((!tab->bush_root_tab? !(i <= no_jbuf_after) : FALSE) || + tab->loosescan_match_tab || tab->bush_children) goto no_join_cache;

The above was a bit complex if. How about: if (((!tab->bush_root_tab && !(i <= no_jbuf_after)) || tab->loosescan_match_tab || tab->bush_children) goto no_join_cache; Don't really understand how we could use join_cache with SMJ_NEST child (ie, a tab with tab->bush_root_tab set).

for (JOIN_TAB *first_inner= tab->first_inner; first_inner; @@ -8122,16 +8394,24 @@ void check_join_cache_usage_for_tables(JOIN *join, ulonglong options, uint no_jbuf_after) { - JOIN_TAB *first_sjm_table= NULL; - JOIN_TAB *last_sjm_table= NULL; + //JOIN_TAB *first_sjm_table= NULL; + //JOIN_TAB *last_sjm_table= NULL;

Remove comments. (Please remove all these before you request a review as the deleted lines will show up nicely in the diff anyway).

+ JOIN_TAB *tab;

- for (uint i= join->const_tables; i < join->tables; i++) - join->join_tab[i].used_join_cache_level= join->max_allowed_join_cache_level; - - for (uint i= join->const_tables; i < join->tables; i++) - { - JOIN_TAB *tab= join->join_tab+i; + //for (uint i= join->const_tables; i < join->tables; i++)

Remove comment

+ for (tab= first_linear_tab(join, TRUE); + tab; + tab= next_linear_tab(join, tab, TRUE)) + { + tab->used_join_cache_level= join->max_allowed_join_cache_level; + }

+ //for (uint i= join->const_tables; i < join->tables; i++)

Remove comment

+ for (tab= first_linear_tab(join, TRUE); + tab; + tab= next_linear_tab(join, tab, TRUE)) + { +#if 0 if (sj_is_materialize_strategy(join->best_positions[i].sj_strategy)) { first_sjm_table= tab; @@ -8142,9 +8422,16 @@ } if (!(tab >= first_sjm_table && tab < last_sjm_table)) tab->first_sjm_sibling= NULL; - +#endif

Remove above ifdefed block

+ JOIN_TAB *prev_tab; +restart: tab->icp_other_tables_ok= TRUE; tab->idx_cond_fact_out= TRUE; + + prev_tab= tab - 1;

Isn't it more clear to set prev_tab as part of the for loop? (This will make it independent of how next_linear_tab() is implemented)

@@ -8154,12 +8441,22 @@

+ /* if (join->return_tab) i= join->return_tab-join->join_tab-1; // always >= 0 + */

Remove commented code

@@ -8204,10 +8501,17 @@ setup_semijoin_dups_elimination(join, options, no_jbuf_after)) DBUG_RETURN(TRUE); /* purecov: inspected */

- for (i= 0; i < join->const_tables; i++) - join->join_tab[i].partial_join_cardinality= 1; + for (tab= first_linear_tab(join, TRUE); + tab; + tab= next_linear_tab(join, tab, TRUE)) + { + tab->partial_join_cardinality= 1; + }

Isn't the above loop wrong ? It was supposed to only loop over const tables, not skip const tables.

- for (i=join->const_tables ; i < join->tables ; i++) + //for (i=join->const_tables ; i < join->tables ; i++)

Remove comment

+ for (tab= first_linear_tab(join, TRUE), i= join->const_tables; + tab; + tab= next_linear_tab(join, tab, TRUE)) { /* The approximation below for partial join cardinality is not good because @@ -8215,24 +8519,45 @@ - it does not differentiate between inner joins, outer joins and semi-joins. Later it should be improved. */ - JOIN_TAB *tab=join->join_tab+i; + JOIN_TAB *prev_tab= tab - 1;

Set prev_tab in the for loop. This allows us to get automaticly the second part of the following if done:

+ if ((tab->bush_root_tab && tab->bush_root_tab->bush_children->start == tab) || + (tab == join->join_tab + join->const_tables)) + prev_tab= NULL;

In other words use: for (tab= first_linear_tab(join, TRUE), i= join->const_tables, prev_tab=0; tab; prev_tab= tab; tab= next_linear_tab(join, tab, TRUE))

+ DBUG_ASSERT(tab->bush_children || tab->table == join->best_positions[i].table->table); tab->partial_join_cardinality= join->best_positions[i].records_read * - (i ? (tab-1)->partial_join_cardinality : 1); + (prev_tab? prev_tab->partial_join_cardinality : 1); + if (!tab->bush_children) + i++;

Why do you assign and increment i? I can't see it beeing used anymore in the code.

- for (i=join->const_tables ; i < join->tables ; i++) + //for (i=join->const_tables ; i < join->tables ; i++)

Remove comment.

+ for (tab= first_linear_tab(join, TRUE), i= join->const_tables; + tab; + tab= next_linear_tab(join, tab, TRUE), i++) { - JOIN_TAB *tab=join->join_tab+i; + //JOIN_TAB *tab=join->join_tab+i;

Remove comment.

+ if (tab->bush_children) + { + if (setup_sj_materialization(tab)) + return TRUE; + } + TABLE *table=tab->table; uint jcl= tab->used_join_cache_level; tab->read_record.table= table; tab->read_record.file=table->file; tab->read_record.unlock_row= rr_unlock_row; - tab->next_select=sub_select; /* normal select */ tab->sorted= sorted; sorted= 0; // only first must be sorted + + if (!(tab->bush_root_tab && + tab->bush_root_tab->bush_children->end == tab + 1)) + { + tab->next_select=sub_select; /* normal select */ + }

The above needs a comment like: /* We should not set tab->next_select for the last table in the SMJ-nest, as this is already set in setup_sj_materialization(). */ (It would be nicer to have next_select set to 0 by default and here only set it if it was not set before).

@@ -8395,13 +8707,16 @@ abort(); /* purecov: deadcode */ } } - join->join_tab[join->tables-1].next_select=0; /* Set by do_select */ + uint n_top_tables= join->join_tab_ranges.head()->end - + join->join_tab_ranges.head()->start; + join->join_tab[n_top_tables - 1].next_select=0; /* Set by do_select */

Isn't the above same as (join->join_tab_ranges.head()->end -1).next_select= 0 ?

-/* + /* If a join buffer is used to join a table the ordering by an index for the first non-constant table cannot be employed anymore. */ - for (i=join->const_tables ; i < join->tables ; i++) + //for (i=join->const_tables ; i < join->tables ; i++) + for (i=join->const_tables ; i < n_top_tables ; i++) {

Wouldn't it be better to also here use the normal for loop for JOIN_TAB's to hide the implementation a bit ? for (tab= first_linear_tab(join, TRUE), tab; tab= next_linear_tab(join, tab, FALSE))

@@ -8481,6 +8799,12 @@ { table->disable_keyread(); table->file->ha_index_or_rnd_end(); + + if (table->pos_in_table_list && + table->pos_in_table_list->jtbm_subselect) + { + table->pos_in_table_list->jtbm_subselect->cleanup(); + } /* We need to reset this for next select (Tested in part_of_refkey) @@ -8675,7 +8999,7 @@

if (table) { - JOIN_TAB *tab,*end; + JOIN_TAB *tab; /* Only a sorted table may be cached. This sorted table is always the first non const table in join->table @@ -8688,13 +9012,15 @@

if (full) { - for (tab= join_tab, end= tab+tables; tab != end; tab++) + for (tab= top_jtrange_tables?join_tab:NULL; tab; tab= next_linear_tab(this, tab, TRUE)) tab->cleanup(); + //psergey4: how is the above supposed to work when + //top_jtrange_tables==FALSE? It will crash right away!

Don't understand the comment. If top_jtrange_tables is 0 then tab is 0 and the for loop will never execute. anyway, it would be better to use our standard loop: for (tab= first_linear_tab(join, FALSE), tab; tab= next_linear_tab(join, tab, TRUE)) instead of having yet another way to do the loop over JOIN_TAB's

table= 0; } else { - for (tab= join_tab, end= tab+tables; tab != end; tab++) + for (tab= top_jtrange_tables?join_tab:NULL; tab; tab= next_linear_tab(this, tab, TRUE))

Same here; Change to a basic for loop

@@ -9982,7 +10343,8 @@

/* Pick the "head" item: the constant one or the first in the join order - that's not inside some SJM nest. + that's not inside some SJM nest. psergey2: out-of-date comment. It is ok + inside SJM, too.

Please update comment to be correct.

*/ if (item_const) head= item_const; @@ -11084,6 +11446,9 @@ for (i= first_tab; i <= last_tab; i++) reopt_remaining_tables |= join->positions[i].table->table->map;

+ table_map save_cur_sj_inner_tables= join->cur_sj_inner_tables; + join->cur_sj_inner_tables= 0; +

Add a comment why the above has to be saved, reset & restored. (Didn't se anything in the loop that would obviously need this)

for (i= first_tab; i <= last_tab; i++) { JOIN_TAB *rs= join->positions[i].table; @@ -11109,6 +11474,7 @@ if (!rs->emb_sj_nest) *outer_rec_count *= pos.records_read; } + join->cur_sj_inner_tables= save_cur_sj_inner_tables; *reopt_cost= cost; }

@@ -12391,6 +12757,8 @@ share->keys=1; share->uniques= test(using_unique_constraint); table->key_info= table->s->key_info= keyinfo; + table->keys_in_use_for_query.set_bit(0); + share->keys_in_use.set_bit(0); keyinfo->key_part=key_part_info; keyinfo->flags=HA_NOSAME; keyinfo->usable_key_parts=keyinfo->key_parts= param->group_parts; @@ -12406,6 +12774,8 @@ bool maybe_null=(*cur_group->item)->maybe_null; key_part_info->null_bit=0; key_part_info->field= field; + if (cur_group == group) + field->key_start.set_bit(0); key_part_info->offset= field->offset(table->record[0]); key_part_info->length= (uint16) field->key_length(); key_part_info->type= (uint8) field->key_type(); @@ -12475,6 +12845,8 @@ keyinfo->key_parts * sizeof(KEY_PART_INFO)))) goto err; bzero((void*) key_part_info, keyinfo->key_parts * sizeof(KEY_PART_INFO)); + table->keys_in_use_for_query.set_bit(0); + share->keys_in_use.set_bit(0); table->key_info= table->s->key_info= keyinfo; keyinfo->key_part=key_part_info; keyinfo->flags=HA_NOSAME | HA_NULL_ARE_EQUAL; @@ -12513,6 +12885,13 @@ { key_part_info->null_bit=0;

Remove above as you are setting it below.

key_part_info->field= *reg_field; + (*reg_field)->flags |= PART_KEY_FLAG; + if (key_part_info == keyinfo->key_part) + (*reg_field)->key_start.set_bit(0); + key_part_info->null_bit= (*reg_field)->null_bit; + key_part_info->null_offset= (uint) ((*reg_field)->null_ptr - + (uchar*) table->record[0]); +

You should only set null_offset if the field has a null_ptr. It's strange that the old code worked without setting null_bit.... Found the issue; in ha_heap.cc we corrected a wrongly set null bit value. However we should not count in this behavior (which the above fix ensures)

@@ -14603,13 +14848,24 @@ return (*tab->read_record.read_record)(&tab->read_record); }

-static int +int join_read_record_no_init(JOIN_TAB *tab) { + Copy_field *save_copy, *save_copy_end; +

Add comment: /* init_read_record resets all elements of tab->read_record(). Remember things that we don't want to have reset. */ I think it would be better to move copy_field and copy_field_end to JOIN_TAB as these don't have anything directly to do with the read_record functions.

+ save_copy= tab->read_record.copy_field; + save_copy_end= tab->read_record.copy_field_end; + + init_read_record(&tab->read_record, tab->join->thd, tab->table, + tab->select,1,1, FALSE); + + tab->read_record.copy_field= save_copy; + tab->read_record.copy_field_end= save_copy_end; + tab->read_record.read_record= rr_sequential_and_unpack; + return (*tab->read_record.read_record)(&tab->read_record); }

<cut>

@@ -19032,16 +19322,21 @@ { table_map used_tables=0;

- uchar sjm_nests[MAX_TABLES]; - uint sjm_nests_cur=0; - uint sjm_nests_end= 0; - uint end_table= join->tables; bool printing_materialize_nest= FALSE; uint select_id= join->select_lex->select_number;

- for (uint i=0 ; i < end_table ; i++) + List_iterator<JOIN_TAB_RANGE> it(join->join_tab_ranges); + JOIN_TAB_RANGE *jt_range; + while ((jt_range= it++))

Please replace with a function that goes over all ranges, so that we don't get an extra indentation level here. (Part of suggestion of review 1)

@@ -19321,12 +19550,16 @@ examined_rows= tab->limit; else { - tab->table->file->info(HA_STATUS_VARIABLE); - examined_rows= tab->table->file->stats.records; + //tab->table->file->info(HA_STATUS_VARIABLE);

Remove comment

+ if (!tab->table->pos_in_table_list || + tab->table->is_filled_at_execution()) // temporary, is_filled_at_execution + examined_rows= tab->records; + else + examined_rows= tab->table->file->stats.records;

Shouldn't you call tab->table->file->info(HA_STATUS_VARIABLE) in the last case ? If not, where is it called now?

@@ -19347,7 +19580,7 @@ */ float f= 0.0; if (examined_rows) - f= (float) (100.0 * join->best_positions[i].records_read / + f= (float) (100.0 * tab->records_read/*join->best_positions[i].records_read*/ /

Removed comment as it's not accurate anymore <cut>

diff -urN --exclude='.*' 5.3-noc/sql/sql_show.cc maria-5.3-subqueries-r36-noc/sql/sql_show.cc

--- 5.3-noc/sql/sql_show.cc 2011-01-27 21:39:33.000000000 +0300

+JOIN_TAB *first_linear_tab(JOIN *join, bool after_const_tables); +JOIN_TAB *next_linear_tab(JOIN* join, JOIN_TAB* tab, bool include_bush_roots);

Please add the prototypes in sql_select.h and include this instead of having prototypes to functions in many places.

@@ -6602,14 +6605,17 @@ bool get_schema_tables_result(JOIN *join, enum enum_schema_table_state executed_place) { - JOIN_TAB *tmp_join_tab= join->join_tab+join->tables; + //JOIN_TAB *tmp_join_tab= join->join_tab+join->tables;

Remove comment

THD *thd= join->thd; LEX *lex= thd->lex; bool result= 0; DBUG_ENTER("get_schema_tables_result");

thd->no_warnings_for_error= 1; - for (JOIN_TAB *tab= join->join_tab; tab < tmp_join_tab; tab++) + //for (JOIN_TAB *tab= join->join_tab; tab < tmp_join_tab; tab++)

Remove comment

+ for (JOIN_TAB *tab= first_linear_tab(join, FALSE); + tab; + tab= next_linear_tab(join, tab, FALSE)) { if (!tab->table || !tab->table->pos_in_table_list) break;

+++ maria-5.3-subqueries-r36-noc/sql/sql_test.cc 2011-02-16 14:42:52.000000000 +0300 @@ -166,58 +166,66 @@ void TEST_join(JOIN *join) { - uint i,ref; + uint ref; + int i; + List_iterator<JOIN_TAB_RANGE> it(join->join_tab_ranges); + JOIN_TAB_RANGE *jt_range; DBUG_ENTER("TEST_join");

- /* - Assemble results of all the calls to full_name() first, - in order not to garble the tabular output below. - */ - String ref_key_parts[MAX_TABLES]; - for (i= 0; i < join->tables; i++) - { - JOIN_TAB *tab= join->join_tab + i; - for (ref= 0; ref < tab->ref.key_parts; ref++) - { - ref_key_parts[i].append(tab->ref.items[ref]->full_name()); - ref_key_parts[i].append(" "); - } - } - DBUG_LOCK_FILE; VOID(fputs("\nInfo about JOIN\n",DBUG_FILE)); + + while ((jt_range= it++)) { + /* + Assemble results of all the calls to full_name() first, + in order not to garble the tabular output below. + */ + String ref_key_parts[MAX_TABLES]; + for (i= 0; i < (jt_range->end - jt_range->start); i++)

Store (jt_range->end - jt_range->start) in a variable and use it above and below. <cut>

+ for (i= 0; i < (jt_range->end - jt_range->start); i++)

<cut>

+++ maria-5.3-subqueries-r36-noc/sql/table.cc 2011-02-16 14:42:52.000000000 +0300 @@ -5337,6 +5337,12 @@ (parent && parent->children_attached)); }

Add also a comment here what this functions means (copy from table.h)

+ +bool st_table::is_filled_at_execution() +{ + return test(pos_in_table_list->jtbm_subselect); +} +

Regards, Monty