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

thank you for the review.

Another commit will follow in a short time.

On 03.01.11 14.03, Øystein Grøvlen wrote:
> 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?

I added a test case in a new file: optimizer_debug_sync.test. The test case 
fails within the trunk branch but not within the opt-backporting branch. The 
code path is still the same for the execution of materialized subqueries in both 
branches, so there must be something else that causes this assert not to be hit 
in the opt-backporting branch.

Obviously, the test failure will not be hit unless the debug synch point is also 
added.
>
> --
> Ø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?

EXPLAIN prints "execution" data, hence we cannot deallocate execution data 
before EXPLAIN is done. I think this is mostly related to subqueries, yes.
>
> ...
>
>> @@ -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 am reverting this change, it seems a bit half-way now.
>
> I also see that I forgot to remove the comment about reverting to IN=>EXISTS
> transformation. Good if you could remove that, too.

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

The original patch was written before the change in procedure :(
>
>>
>> - 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);

Done.
>
> ...
>
>> 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.

Done.
>
> ...
>
>> @@ -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.

Bummer. Reverted.
>
> ...
>
>> @@ -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.

Done.
>
> ...
>
>> @@ -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.

Not much effort, actually :)
>
>> 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?

Yes, and so can uncacheable(), no_tables(), may_be_null(), 
upper_select_const_tables() and engine_type()...
>
>>
>> === 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.

Yes, some debug code that I did not want to delete yet...
>
>> }
>>
>> 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?

It will be deleted in final commit ;)
>
>>
>> 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?

It may be possible, but messing with this may require quite a lot of effort.
>
> ...
>
>> === 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.

Done.
>
>> }
>> }
>> 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.

Done (for the whole function).
>
> ...
>
Thanks,
Roy
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