From: Roy Lyseng Date: April 4 2011 2:23pm Subject: Re: bzr commit into mysql-trunk branch (guilhem.bichot:3358) List-Archive: http://lists.mysql.com/commits/134629 Message-Id: <4D99D453.8040807@oracle.com> MIME-Version: 1.0 Content-Type: multipart/alternative; boundary="------------010202040107090506000605" --------------010202040107090506000605 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Hi Guilhem, the fix is OK to push. I have one suggestion below, and generally I have a couple of dislikes, but they can be removed later if we agree on them: 1. The need for the local variable thd everywhere 2. Accessing LEX data through item->unit->thd->lex. Thanks, Roy ... On 04.04.11 15.45, Guilhem Bichot wrote: > #At file:///home/mysql_src/bzrrepos_new/59852/ based on revid:tor.didriksen@stripped > > 3358 Guilhem Bichot 2011-04-04 > Code cleanup: getting rid of two redundant THD pointers in subquery-related classes > @ sql/item_subselect.cc > Item_subselect::thd and subselect_engine::thd are deleted. > The first is replaced with the existing Item_subselect::unit::thd. > The second one is replaced with the existing subselect_engine::item::unit::thd > (same object as for the first one, actually: item points to the containing > Item_subselect); for subselect_union_engine it's additionally possible to use > subselect_union_engine::unit::thd. > > Before this change, Item_subselect::thd and subselect_engine::thd > were set to NULL in constructors, and Item_subselect::fix_fields() > corrected them. > The advantage of using SELECT_LEX_UNIT::thd pointers is that they are > always kept correct by reinit_stmt_before_use(). > We also add assertions that various pointers-to-THD are equal. > > modified: > sql/item_subselect.cc > sql/item_subselect.h > === modified file 'sql/item_subselect.cc' > --- a/sql/item_subselect.cc 2011-03-29 07:30:44 +0000 > +++ b/sql/item_subselect.cc 2011-04-04 13:45:39 +0000 > @@ -42,7 +42,7 @@ inline Item * and_items(Item* cond, Item > } > > Item_subselect::Item_subselect(): > - Item_result_field(), value_assigned(0), thd(0), substitution(0), > + Item_result_field(), value_assigned(0), substitution(0), > engine(0), old_engine(0), used_tables_cache(0), have_to_be_excluded(0), > const_item_cache(1), engine_changed(0), changed(0), > is_correlated(FALSE) > @@ -169,14 +169,25 @@ Item_subselect::select_transformer(JOIN > } > > > -bool Item_subselect::fix_fields(THD *thd_param, Item **ref) > +bool Item_subselect::fix_fields(THD *thd, Item **ref) > { > - char const *save_where= thd_param->where; > + char const *save_where= thd->where; > uint8 uncacheable; > bool res; > > DBUG_ASSERT(fixed == 0); > - engine->set_thd((thd= thd_param)); > + /* > + Pointers to THD must match. unit::thd may vary over the lifetime of the > + item (for example triggers, and thus their Item-s, are in a cache shared > + by all connections), but reinit_stmt_before_use() keeps it up-to-date, > + which we check here. subselect_union_engine functions also do sanity > + checks. > + */ > + DBUG_ASSERT(thd == unit->thd); > + // Engine accesses THD via its 'item' pointer, check it: > + engine->assert_item(this); I dislike having a function just for asserting. What about DBUG_ASSERT(this == engine->get_item()); ? get_item() may even be defined within #if defined(DEBUG) - #endif > + > + engine->set_thd_for_result(); > > if (check_stack_overrun(thd, STACK_MIN_SIZE, (uchar*)&res)) > return TRUE; > @@ -285,6 +296,7 @@ bool Item_subselect::exec() > Do not execute subselect in case of a fatal error > or if the query has been killed. > */ > + THD * const thd= unit->thd; > if (thd->is_error() || thd->killed) > DBUG_RETURN(true); > > @@ -437,7 +449,7 @@ table_map Item_subselect::used_tables() > > bool Item_subselect::const_item() const > { > - return thd->lex->context_analysis_only ? FALSE : const_item_cache; > + return unit->thd->lex->context_analysis_only ? FALSE : const_item_cache; > } > > Item *Item_subselect::get_tmp_table_item(THD *thd_arg) > @@ -521,12 +533,6 @@ Item_maxmin_subselect::Item_maxmin_subse > used_tables_cache= parent->get_used_tables_cache(); > const_item_cache= parent->get_const_item_cache(); > > - /* > - this subquery always creates during preparation, so we can assign > - thd here > - */ > - thd= thd_param; > - > DBUG_VOID_RETURN; > } > > @@ -581,6 +587,7 @@ Item_singlerow_subselect::select_transfo > DBUG_RETURN(RES_OK); > > SELECT_LEX *select_lex= join->select_lex; > + THD * const thd= unit->thd; > Query_arena *arena= thd->stmt_arena; > > if (!select_lex->master_unit()->is_union()&& > @@ -1091,6 +1098,8 @@ Item_in_subselect::single_value_transfor > DBUG_RETURN(RES_ERROR); > } > > + THD * const thd= unit->thd; > + > /* > If this is an ALL/ANY single-value subselect, try to rewrite it with > a MIN/MAX subselect. We can do that if a possible NULL result of the > @@ -1264,6 +1273,7 @@ Item_subselect::trans_res > Item_in_subselect::single_value_in_to_exists_transformer(JOIN * join, Comp_creator *func) > { > SELECT_LEX *select_lex= join->select_lex; > + THD * const thd= unit->thd; > DBUG_ENTER("Item_in_subselect::single_value_in_to_exists_transformer"); > > select_lex->uncacheable|= UNCACHEABLE_DEPENDENT; > @@ -1467,6 +1477,7 @@ Item_in_subselect::row_value_transformer > SELECT_LEX_UNIT *master_unit= select_lex->master_unit(); > substitution= optimizer; > > + THD * const thd= unit->thd; > SELECT_LEX *current= thd->lex->current_select; > thd->lex->current_select= current->outer_select(); > //optimizer never use Item **ref => we can pass 0 as parameter > @@ -1526,6 +1537,7 @@ Item_subselect::trans_res > Item_in_subselect::row_value_in_to_exists_transformer(JOIN * join) > { > SELECT_LEX *select_lex= join->select_lex; > + THD * const thd= unit->thd; > Item *having_item= 0; > uint cols_num= left_expr->cols(); > bool is_having_used= (join->having || select_lex->with_sum_func || > @@ -1756,6 +1768,7 @@ Item_subselect::trans_res > Item_in_subselect::select_in_like_transformer(JOIN *join, Comp_creator *func) > { > Query_arena *arena, backup; > + THD * const thd= unit->thd; > SELECT_LEX *current= thd->lex->current_select; > const char *save_where= thd->where; > Item_subselect::trans_res res= RES_ERROR; > @@ -1911,7 +1924,7 @@ bool Item_in_subselect::setup_engine() > > old_engine= engine; > > - if (!(engine= new subselect_hash_sj_engine(thd, this, > + if (!(engine= new subselect_hash_sj_engine(unit->thd, this, > static_cast(old_engine)))) > DBUG_RETURN(true); > if (((subselect_hash_sj_engine *) engine) > @@ -1987,7 +2000,7 @@ bool Item_in_subselect::init_left_expr_c > > for (uint i= 0; i< left_expr->cols(); i++) > { > - Cached_item *cur_item_cache= new_Cached_item(thd, > + Cached_item *cur_item_cache= new_Cached_item(unit->thd, > left_expr->element_index(i), > use_result_field); > if (!cur_item_cache || left_expr_cache->push_front(cur_item_cache)) > @@ -2043,11 +2056,14 @@ void Item_allany_subselect::print(String > } > > > -void subselect_engine::set_thd(THD *thd_arg) > +void subselect_engine::set_thd_for_result() > { > - thd= thd_arg; > + /* > + select_result's constructor sets neither select_result::thd nor > + select_result::unit. > + */ > if (result) > - result->set_thd(thd_arg); > + result->set_thd(item->unit->thd); > } > > > @@ -2148,6 +2164,7 @@ bool subselect_single_select_engine::pre > { > if (prepared) > return 0; > + THD * const thd= item->unit->thd; > join= new JOIN(thd, select_lex->item_list, > select_lex->options | SELECT_NO_UNLOCK, result); > if (!join || !result) > @@ -2174,6 +2191,9 @@ bool subselect_single_select_engine::pre > > bool subselect_union_engine::prepare() > { > + THD * const thd= unit->thd; > + // We can access THD as above, or via 'item', verify equality: > + DBUG_ASSERT(thd == item->unit->thd); > return unit->prepare(thd, result, SELECT_NO_UNLOCK); > } > > @@ -2275,6 +2295,7 @@ bool subselect_single_select_engine::exe > { > DBUG_ENTER("subselect_single_select_engine::exec"); > int rc= 0; > + THD * const thd= item->unit->thd; > char const *save_where= thd->where; > SELECT_LEX *save_select= thd->lex->current_select; > thd->lex->current_select= select_lex; > @@ -2372,6 +2393,8 @@ exit: > > bool subselect_union_engine::exec() > { > + THD * const thd= unit->thd; > + DBUG_ASSERT(thd == item->unit->thd); > char const *save_where= thd->where; > const bool res= unit->exec(); > thd->where= save_where; > @@ -2830,7 +2853,7 @@ subselect_single_select_engine::save_joi > plan to save will not be replaced anyway. > 4) The Item_subselect is cacheable > */ > - if (thd->lex->describe&& // 1 > + if (item->unit->thd->lex->describe&& // 1 > !select_lex->uncacheable&& // 2 > !(join->select_options& SELECT_DESCRIBE)) // 3 > { > @@ -2873,7 +2896,7 @@ table_map subselect_union_engine::upper_ > void subselect_single_select_engine::print(String *str, > enum_query_type query_type) > { > - select_lex->print(thd, str, query_type); > + select_lex->print(item->unit->thd, str, query_type); > } > > > @@ -3134,6 +3157,7 @@ bool subselect_hash_sj_engine::setup(Lis > */ > if (!(tmp_result_sink= new select_union)) > DBUG_RETURN(TRUE); > + THD * const thd= item->unit->thd; > if (tmp_result_sink->create_result_table( > thd, tmp_columns, TRUE, > thd->variables.option_bits | TMP_TABLE_ALL_COLUMNS, > @@ -3298,6 +3322,7 @@ void subselect_hash_sj_engine::cleanup() > DBUG_ENTER("subselect_hash_sj_engine::cleanup"); > is_materialized= false; > result->cleanup(); /* Resets the temp table as well. */ > + THD * const thd= item->unit->thd; > DEBUG_SYNC(thd, "before_index_end_in_subselect"); > if (tab->table->file->inited) > tab->table->file->ha_index_end(); // Close the scan over the index > @@ -3332,6 +3357,7 @@ bool subselect_hash_sj_engine::exec() > if (!is_materialized) > { > bool res= false; > + THD * const thd= item->unit->thd; > SELECT_LEX *save_select= thd->lex->current_select; > thd->lex->current_select= materialize_engine->select_lex; > if ((res= materialize_engine->join->optimize())) > > === modified file 'sql/item_subselect.h' > --- a/sql/item_subselect.h 2011-03-29 07:30:44 +0000 > +++ b/sql/item_subselect.h 2011-04-04 13:45:39 +0000 > @@ -43,8 +43,6 @@ class Item_subselect :public Item_result > private: > bool value_assigned; /* value already assigned to subselect */ > public: > - /* thread handler, will be assigned in fix_fields only */ > - THD *thd; > /* > Used inside Item_subselect::fix_fields() according to this scenario: > > Item_subselect::fix_fields > @@ -440,7 +438,6 @@ class subselect_engine: public Sql_alloc > { > protected: > select_result_interceptor *result; /* results storage class */ > - THD *thd; /* pointer to current THD */ > Item_subselect *item; /* item, that use this engine */ > enum Item_result res_type; /* type of results */ > enum_field_types res_field_type; /* column type of the results */ > @@ -452,7 +449,7 @@ public: > INDEXSUBQUERY_ENGINE, HASH_SJ_ENGINE}; > > subselect_engine(Item_subselect *si, select_result_interceptor *res) > - :result(res), thd(NULL), item(si), res_type(STRING_RESULT), > + :result(res), item(si), res_type(STRING_RESULT), > res_field_type(MYSQL_TYPE_VAR_STRING), maybe_null(false) > {} > virtual ~subselect_engine() {}; // to satisfy compiler > @@ -461,12 +458,8 @@ public: > */ > virtual void cleanup()= 0; > > - /* > - Also sets "thd" for subselect_engine::result. > - Should be called before prepare(). > - */ > - void set_thd(THD *thd_arg); > - THD * get_thd() const { return thd; } > + /// Sets "thd" for 'result'. Should be called before prepare() > + void set_thd_for_result(); > virtual bool prepare()= 0; > virtual void fix_length_and_dec(Item_cache** row)= 0; > /* > @@ -506,6 +499,8 @@ public: > /* Check if subquery produced any rows during last query execution */ > virtual bool no_rows() const = 0; > virtual enum_engine_type engine_type() const { return ABSTRACT_ENGINE; } > + /// Verifies that the internal item matches the argument > + void assert_item(const Item_subselect *i) const { DBUG_ASSERT(i == item); } > > protected: > void set_row(List &item_list, Item_cache **row); > @@ -609,10 +604,7 @@ public: > // constructor can assign THD because it will be called after JOIN::prepare > subselect_uniquesubquery_engine(THD *thd_arg, st_join_table *tab_arg, > Item_subselect *subs, Item *where) > - :subselect_engine(subs, 0), tab(tab_arg), cond(where) > - { > - set_thd(thd_arg); > - } > + :subselect_engine(subs, 0), tab(tab_arg), cond(where) {} > virtual void cleanup() {} > virtual bool prepare(); > virtual void fix_length_and_dec(Item_cache** row); > > > > --------------010202040107090506000605--