List:Commits« Previous MessageNext Message »
From:Øystein Grøvlen Date:January 3 2011 1:03pm
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3280)
Bug#56080
View as plain text  
Hi Roy,

Thanks for the patch. Looks good. See in-line for comments.

I also miss a test case for this bug.  Why not add the test case from 
the bug report?

--
Øystein



On 11/ 9/10 04:29 PM, Roy Lyseng wrote:
> #At file:///home/rl136806/mysql/repo/mysql-work3/ based on
> revid:olav.sandstaa@stripped
>
>   3280 Roy Lyseng	2010-11-09
>        Bug#56080: Assertion 'inited=INDEX in sql/handler.h
>
>        The problem occurs because some TABLE objects are associated with
>        the optimizer execution structures even after close_thread_tables()
>        has been called. In turn, this means that final cleanup of some
>        TABLE objects is done from within THD::cleanup_after_query().
>        At this point in time, the TABLE objects may already have been
>        given to another session, meaning that we may end another session's
>        index or table scan.
>
>        The safest solution seems to be to always perform a thorough cleanup
>        of all execution data structures, including releasing the TABLE
>        objects, before close_thread_tables() is called. In other words,
>        this will have to be done at the end of each execution.
>
>        However, this is currently not possible in the implementation of
>        subquery materialization. This part of the optimizer assigns some
>        objects, including a TABLE object, for the lifetime of the
>        statement object, and not for each execution. This gives an odd
>        situation, where some parts of the data structures are set up for
>        reuse and others are not. Combine this with the fact that the
>        user may decide to use another strategy for a subsequent
>        subquery execution, the "permanent" data structures may not even
>        be used later.
>
>        It is therefore reasonable to allocate materialization execution
>        structures for the lifetime of one execution. It means that the
>        functions init_permanent() and init_runtime() of class
>        subselect_hash_sj_engine are combined into one setup() function.
>        In addition, the function Item_subselect::cleanup() will be called
>        when the cleanup() function for the underlying query expression
>        is called, and this function will cleanup and destroy the
>        materialization data structures, so that the subquery item object
>        is ready for a new optimization process in the next round of
>        execution.
>
>        However, the EXPLAIN functionality relies on these data
>        structures being present when explaining a query containing
>        a materialized subquery. Thus, we have two conflicting
>        requirements for query cleanup:
>         - For maximum efficiency we should release resources as early as
>           possible after execution.
>         - We cannot release resources that are needed for EXPLAIN queries.

I do not quite understand this.  Is this related to the issue that 
subqueries are executed as part of EXPLAIN?

...

> @@ -1917,43 +1914,30 @@ void Item_in_subselect::fix_after_pullou
>
>   bool Item_in_subselect::setup_engine()
>   {
> -  subselect_hash_sj_engine *hash_engine;
>     DBUG_ENTER("Item_in_subselect::setup_engine");
>
> -  if (engine->engine_type() == subselect_engine::SINGLE_SELECT_ENGINE)
> -  {
> -    /* Create/initialize objects in permanent memory. */
> -    Query_arena *arena= thd->stmt_arena;
> -    Query_arena backup;
> -    if (arena->is_conventional())
> -      arena= 0;
> -    else
> -      thd->set_n_backup_active_arena(arena,&backup);
> +  /* Currently, the decision whether to materialize is already taken. */
> +  if (exec_method != Item_exists_subselect::EXEC_MATERIALIZATION)
> +    DBUG_RETURN(FALSE);

Maybe it should be specified in the function documentation that 
setup_engine() does nothing if query is not to be executed with 
materialization.  As it is, it seems like it should only be called if 
materialization has been selected.

I also see that I forgot to remove the comment about reverting to 
IN=>EXISTS transformation.  Good if you could remove that, too.

By the way, it should be OK to use built-in lower case false, now.

>
> -    subselect_single_select_engine *old_engine=
> -      static_cast<subselect_single_select_engine*>(engine);
> -    if (!(hash_engine= new subselect_hash_sj_engine(thd, this,
> -                                                    old_engine)) ||
> -        hash_engine->init_permanent(unit->get_unit_column_types()))
> -    {
> -      /*
> -        For some reason we cannot use materialization for this IN predicate.
> -        Delete all materialization-related objects, and return error.
> -      */
> -      delete hash_engine;
> -      if (arena)
> -        thd->restore_active_arena(arena,&backup);
> -      DBUG_RETURN(TRUE);
> -    }
> -    engine= hash_engine;
> +  DBUG_ASSERT(engine->engine_type() == subselect_engine::SINGLE_SELECT_ENGINE);
>
> -    if (arena)
> -      thd->restore_active_arena(arena,&backup);
> -  }
> -  else
> +  old_engine= engine;
> +
> +  if (!(engine= new subselect_hash_sj_engine(thd, this,
> +                  static_cast<subselect_single_select_engine*>(old_engine))))
> +    DBUG_RETURN(TRUE);
> +  if (((subselect_hash_sj_engine *) engine)
> +        ->setup(unit->get_unit_column_types()))
>     {
> -    DBUG_ASSERT(engine->engine_type() == subselect_engine::HASH_SJ_ENGINE);
> -    hash_engine= static_cast<subselect_hash_sj_engine*>(engine);
> +    /*
> +      For some reason we cannot use materialization for this IN predicate.
> +      Delete all materialization-related objects, and return error.
> +    */
> +    delete engine;
> +    engine= old_engine;
> +    old_engine= NULL;
> +    DBUG_RETURN(TRUE);

Or DBUG_RETURN(true);

...

>   subselect_hash_sj_engine::~subselect_hash_sj_engine()
>   {
>     delete result;
> -  if (tab)
> -    free_tmp_table(thd, tab->table);
> +  DBUG_ASSERT(!tab);

I suggest adding a comment that explains that the above assert will 
verify that cleanup has been called.

...

> @@ -495,11 +494,11 @@ public:
>         1 - Either an execution error, or the engine was "changed", and the
>             caller should call exec() again for the new engine.
>     */
> -  virtual int exec()= 0;
> +  virtual bool exec()= 0;
>     virtual uint cols()= 0; /* return number of columns in select */
>     virtual uint8 uncacheable()= 0; /* query is uncacheable */
> -  enum Item_result type() { return res_type; }
> -  enum_field_types field_type() { return res_field_type; }
> +  virtual const enum Item_result type() const { return res_type; }
> +  virtual const enum_field_types field_type() const { return res_field_type; }

I do not think the first const of each line makes much sense.  You will 
copy this value anyway during assignment.  If the return value was a 
pointer of a reference, it would be a different matter.

...

> @@ -553,24 +553,26 @@ public:
>
>   class subselect_union_engine: public subselect_engine
>   {
> +private:
>     st_select_lex_unit *unit;  /* corresponding unit structure */

If private is made explicit, you might as well move it to the end of the 
class definition.

...

> @@ -615,26 +617,28 @@ public:
>     {
>       set_thd(thd_arg);
>     }
> -  void cleanup();
> -  int prepare();
> -  void fix_length_and_dec(Item_cache** row);
> -  int exec();
> -  uint cols() { return 1; }
> -  uint8 uncacheable() { return UNCACHEABLE_DEPENDENT; }
> -  void exclude();
> -  table_map upper_select_const_tables() { return 0; }
> +  virtual void cleanup() {}
> +  virtual bool prepare();
> +  virtual void fix_length_and_dec(Item_cache** row);
> +  virtual bool exec();
> +  virtual uint cols() { return 1; }
> +  virtual uint8 uncacheable() { return UNCACHEABLE_DEPENDENT; }
> +  virtual void exclude();
> +  virtual table_map upper_select_const_tables() { return 0; }
>     virtual void print (String *str, enum_query_type query_type);
> -  bool change_result(Item_subselect *si, select_result_interceptor *result);
> -  bool no_tables();
> -  int scan_table();
> +  virtual bool change_result(Item_subselect *si,
> +                             select_result_interceptor *result);
> +  virtual bool no_tables();
> +  bool scan_table();
>     bool copy_ref_key();
> -  bool no_rows() { return empty_result_set; }
> +  virtual bool no_rows() { return empty_result_set; }

I think no_rows() with some effort could be made const.

>     virtual enum_engine_type engine_type() { return UNIQUESUBQUERY_ENGINE; }
>   };
>
>
>   class subselect_indexsubquery_engine: public subselect_uniquesubquery_engine
>   {
> +private:
>     /* FALSE for 'ref', TRUE for 'ref-or-null'. */
>     bool check_null;
>     /*
> @@ -677,7 +681,7 @@ public:
>        check_null(chk_null),
>        having(having_arg)
>     {}
> -  int exec();
> +  virtual bool exec();
>     virtual void print (String *str, enum_query_type query_type);
>     virtual enum_engine_type engine_type() { return INDEXSUBQUERY_ENGINE; }
>   };
> @@ -712,21 +716,17 @@ inline bool Item_subselect::is_uncacheab
>
>   class subselect_hash_sj_engine: public subselect_uniquesubquery_engine
>   {
> -protected:
> +private:
>     /* TRUE if the subquery was materialized into a temp table. */
>     bool is_materialized;
>     /*
>       The old engine already chosen at parse time and stored in permanent memory.
> -    Through this member we can re-create and re-prepare materialize_join for
> -    each execution of a prepared statement. We also reuse the functionality
> -    of subselect_single_select_engine::[prepare | cols].
> +    Through this member we can re-create and re-prepare the join object
> +    used to materialize the subquery for each execution of a prepared
> +    statement. We also reuse the functionality of
> +    subselect_single_select_engine::[prepare | cols].
>     */
>     subselect_single_select_engine *materialize_engine;
> -  /*
> -    QEP to execute the subquery and materialize its result into a
> -    temporary table. Created during the first call to exec().
> -  */
> -  JOIN *materialize_join;
>     /* Temp table context of the outer select's JOIN. */
>     TMP_TABLE_PARAM *tmp_param;
>
> @@ -735,20 +735,19 @@ public:
>                                  subselect_single_select_engine *old_engine)
>       :subselect_uniquesubquery_engine(thd, NULL, in_predicate, NULL),
>       is_materialized(FALSE), materialize_engine(old_engine),
> -    materialize_join(NULL), tmp_param(NULL)
> +    tmp_param(NULL)
>     {}
>     ~subselect_hash_sj_engine();
>
> -  bool init_permanent(List<Item>  *tmp_columns);
> -  bool init_runtime();
> -  void cleanup();
> -  int prepare()
> +  bool setup(List<Item>  *tmp_columns);
> +  virtual void cleanup();
> +  virtual bool prepare()
>     {
>       return materialize_engine->prepare();
>     }
> -  int exec();
> -  void print (String *str, enum_query_type query_type);
> -  uint cols()
> +  virtual bool exec();
> +  virtual void print (String *str, enum_query_type query_type);
> +  virtual uint cols()
>     {
>       return materialize_engine->cols();
>     }

Can cols() be made const?

>
> === modified file 'sql/sql_lex.h'
> --- a/sql/sql_lex.h	2010-10-25 16:07:05 +0000
> +++ b/sql/sql_lex.h	2010-11-09 15:29:16 +0000
> @@ -596,7 +596,7 @@ public:
>     inline bool is_union ();
>
>     friend void lex_start(THD *thd);
> -  friend int subselect_union_engine::exec();
> +  friend bool subselect_union_engine::exec();
>
>     List<Item>  *get_unit_column_types();
>   };
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc	2010-10-27 14:46:44 +0000
> +++ b/sql/sql_parse.cc	2010-11-09 15:29:16 +0000
> @@ -5551,6 +5551,8 @@ void mysql_parse(THD *thd, char *rawbuf,
>       thd->end_statement();
>       thd->cleanup_after_query();
>       DBUG_ASSERT(thd->change_list.is_empty());
> +    //sql_print_error("Query: %*s", length, rawbuf);
> +    //report_memory_plugin();

I guess this is not intentionally included.

>     }
>
>     DBUG_VOID_RETURN;
>
> === modified file 'sql/sql_plugin.cc'
> --- a/sql/sql_plugin.cc	2010-11-05 22:14:29 +0000
> +++ b/sql/sql_plugin.cc	2010-11-09 15:29:16 +0000
> @@ -1554,6 +1554,19 @@ error:
>     DBUG_RETURN(TRUE);
>   }
>
> +void report_memory_plugin()
> +
> +{
> +  struct st_plugin_int *p;
> +
> +  for (uint i= 0; i<  plugin_array.elements; i++)
> +  {
> +    p= *dynamic_element(&plugin_array, i, struct st_plugin_int **);
> +    if (!strcmp(p->name.str, "MEMORY"))
> +      sql_print_error("Plugin '%s' has ref_count=%d",
> +                      p->name.str, p->ref_count);
> +  }
> +}

This is not mentioned in commit comments.  Included by accident?

>
>   void plugin_shutdown(void)
>   {
>
> === modified file 'sql/sql_plugin.h'
> --- a/sql/sql_plugin.h	2010-10-07 17:34:38 +0000
> +++ b/sql/sql_plugin.h	2010-11-09 15:29:16 +0000
> @@ -160,4 +160,5 @@ typedef my_bool (plugin_foreach_func)(TH
>   #define plugin_foreach(A,B,C,D) plugin_foreach_with_mask(A,B,C,PLUGIN_IS_READY,D)
>   extern bool plugin_foreach_with_mask(THD *thd, plugin_foreach_func *func,
>                                        int type, uint state_mask, void *arg);
> +void report_memory_plugin();
>   #endif
>
> === modified file 'sql/sql_select.cc'
> --- a/sql/sql_select.cc	2010-11-05 22:19:41 +0000
> +++ b/sql/sql_select.cc	2010-11-09 15:29:16 +0000
> @@ -2626,10 +2626,14 @@ void JOIN::restore_tmp()
>   }
>
>
> -int
> -JOIN::reinit()
> +/**
> +  Reset the state of this join object so that it is ready for a
> +  new execution.
> +*/
> +
> +void JOIN::reset()
>   {
> -  DBUG_ENTER("JOIN::reinit");
> +  DBUG_ENTER("JOIN::reset");
>
>     unit->offset_limit_cnt= (ha_rows)(select_lex->offset_limit ?
>                                       select_lex->offset_limit->val_uint() :
> @@ -2677,7 +2681,7 @@ JOIN::reinit()
>         func->clear();
>     }
>
> -  DBUG_RETURN(0);
> +  DBUG_VOID_RETURN;
>   }
>
>   /**
> @@ -3309,14 +3313,12 @@ JOIN::exec()
>
>
>   /**
> -  Clean up join.
> +  Clean up and destroy join object.
>
> -  @return
> -    Return error that hold JOIN.
> +  @return false if previous execution was successful, and true otherwise

Why is this return value needed?  Cannot this be picked up after execution?

...

> === modified file 'sql/sql_union.cc'
> --- a/sql/sql_union.cc	2010-10-25 09:18:21 +0000
> +++ b/sql/sql_union.cc	2010-11-09 15:29:16 +0000
> @@ -223,7 +223,7 @@ bool st_select_lex_unit::prepare(THD *th
>   	  DBUG_RETURN(TRUE);
>   	}
>   	sl->join->select_options|= SELECT_DESCRIBE;
> -	sl->join->reinit();
> +	sl->join->reset();

Please, remove TAB indentation.

>         }
>       }
>       DBUG_RETURN(FALSE);
> @@ -495,7 +495,10 @@ bool st_select_lex_unit::exec()
>         thd->lex->current_select= sl;
>
>         if (optimized)
> -	saved_error= sl->join->reinit();
> +      {
> +	saved_error= false;

TAB indentation.

...

-- 
Øystein
Thread
bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3280) Bug#56080Roy Lyseng9 Nov
  • Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3280)Bug#56080Øystein Grøvlen3 Jan
    • Re: bzr commit into mysql-next-mr-bugfixing branch (roy.lyseng:3280)Bug#56080Roy Lyseng4 Jan