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<subselect_single_select_engine*>(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> &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);
>
>
>
>
