List:Commits« Previous MessageNext Message »
From:Roy Lyseng Date:April 4 2011 2:23pm
Subject:Re: bzr commit into mysql-trunk branch (guilhem.bichot:3358)
View as plain text  
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);
>
>
>
>


Thread
bzr commit into mysql-trunk branch (guilhem.bichot:3358) Guilhem Bichot4 Apr
  • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3358)Roy Lyseng4 Apr
    • Re: bzr commit into mysql-trunk branch (guilhem.bichot:3358)Guilhem Bichot4 Apr