Review of MDEV-30073 Wrong result on 2nd execution of PS for query
with NOT EXISTS
Bye the way, nice to see that this patch close so many different MDEVs!
Please add a list of all the MDEV's closed by this patch in the commit message!
MDEV-23828: query with EXISTS over subquery using mergeable derived
table / view executed in PS/SP for the second time
MDEV-30396: 2nd execution of query with EXISTS subquery over view / DT
MDEV-31175: 2nd execution of SELECT with mergeable view / DT
and aggregate function
MDEV-33081: 2-nd execution of query with forced materialization
of a mergeable view because of too many base tables
MDEV-31269: 2nd execution of query with HAVING containing IN predicand
over subquery with EXISTS in WHERE clause
MDEV-31281: 2nd execution of query with IN subquery whose left operand
is aggregate function or aggregate function
MDEV-31178: 2 execution of query with conversion from
IN predicate to IN subquery applied
> index 446154e517b..d474a39e228 100644
> --- a/mysql-test/main/func_group.test
> +++ b/mysql-test/main/func_group.test
> @@ -1268,12 +1268,14 @@ set @@optimizer_switch='materialization=on,in_to_exists=off,semijoin=off';
> --echo # 1) Test that subquery materialization is setup for query with
> --echo # premature optimize() exit due to "Impossible WHERE"
> --echo #
> +--disable_ps2_protocol
> SELECT MIN(t2.pk)
> FROM t2 JOIN t1 ON t1.pk=t2.pk
> WHERE 'j'
> HAVING ('m') IN (
> SELECT v
> FROM t2);
> +--enable_ps2_protocol
Why this change?
Does this produce wrong results after your patch?
At least I cannot see anything in the query that would disallow double
execution.
If double execution does not work anymore for this one, please add a
note about this in
the commit message or in the test case.
> diff --git a/mysql-test/main/mysqltest_ps.test b/mysql-test/main/mysqltest_ps.test
> index c5a332f691f..853ce8bd38e 100644
> --- a/mysql-test/main/mysqltest_ps.test
> +++ b/mysql-test/main/mysqltest_ps.test
> @@ -32,3 +32,4 @@ select 1 + "2 a";
> create table t (a int primary key, b blob default '');
> select a, (2*a) AS a from t group by a;
> drop table t;
> +
>
Remove the change to the above. No need to touch this test case in this commit
> diff --git a/mysql-test/main/order_by.test b/mysql-test/main/order_by.test
> index c1e0c318283..7ca9a8f8def 100644
> --- a/mysql-test/main/order_by.test
> +++ b/mysql-test/main/order_by.test
> @@ -90,7 +90,9 @@ drop table t1;
> create table t1 (i int);
> insert into t1 values(1),(2),(1),(2),(1),(2),(3);
> select distinct i from t1;
> +--disable_ps2_protocol
> select distinct i from t1 order by rand(5);
> +--enable_ps2_protocol
>
Why disable ps2_protocol here?
> diff --git a/mysql-test/main/type_date.test b/mysql-test/main/type_date.test
> index 3566e427d50..5f17a893d53 100644
> --- a/mysql-test/main/type_date.test
> +++ b/mysql-test/main/type_date.test
> @@ -601,7 +601,7 @@ SHOW CREATE TABLE t2;
> DROP TABLE t2;
> DROP VIEW v1;
> DROP TABLE t1;
> -
> +# --enable_ps2_protocol
The above looks wrong. Probably leftover from some testing.
Please revert the change to this file
> diff --git a/mysql-test/main/type_newdecimal.test b/mysql-test/main/type_newdecimal.test
> index 366199e8aa8..7223b7b44f0 100644
> --- a/mysql-test/main/type_newdecimal.test
> +++ b/mysql-test/main/type_newdecimal.test
> @@ -1929,7 +1929,9 @@ drop table t1;
> SELECT AVG(DISTINCT 0.00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001);
> --enable_view_protocol
> CREATE TABLE t1 AS SELECT NULL AS v1;
> +--disable_ps2_protocol
> SELECT 1 FROM t1 GROUP BY v1 ORDER BY AVG ( from_unixtime ( '' ) ) ;
> +--enable_ps2_protocol
> DROP TABLE t1;
Why the change?
> diff --git a/mysql-test/main/win.test b/mysql-test/main/win.test
> index cba6b0fd7af..00c79425fc0 100644
> --- a/mysql-test/main/win.test
> +++ b/mysql-test/main/win.test
> @@ -2770,6 +2770,7 @@ drop table t1;
> CREATE TABLE t1 ( a char(25), b text);
> INSERT INTO t1 VALUES ('foo','bar');
>
> +--disable_ps2_protocol
> SELECT
> SUM(b) OVER (PARTITION BY a),
> ROW_NUMBER() OVER (PARTITION BY b)
> @@ -2777,6 +2778,7 @@ FROM t1
> GROUP BY
> LEFT((SYSDATE()), 'foo')
> WITH ROLLUP;
> +--enable_ps2_protocol
> drop table t1;
Why ?
> diff --git a/mysql-test/suite/federated/federatedx_create_handlers.test b/mysql-test/suite/federated/federatedx_create_handlers.test
> index 8e504590282..b2884397333 100644
> --- a/mysql-test/suite/federated/federatedx_create_handlers.test
> +++ b/mysql-test/suite/federated/federatedx_create_handlers.test
> @@ -94,6 +94,7 @@ DEFAULT CHARSET=latin1;
> INSERT INTO federated.t3 VALUES
> ('yyy'), ('www'), ('yyy'), ('xxx'), ('www'), ('yyy'), ('www');
>
> +--sorted_result
> SELECT *
> FROM federated.t3, (SELECT * FROM federated.t1 WHERE id > 3) t
> WHERE federated.t3.name=t.name;
>
I assume this has nothing to do with the patch, but just a random
failure you fixed?
If that is the case, please add a comment about this file change in
the commit message or
move this change to another commit.
> --- a/sql/item.cc
> +++ b/sql/item.cc
> @@ -3027,7 +3027,11 @@ Item* Item_ref::build_clone(THD *thd)
> unlikely(!(copy->ref= (Item**) alloc_root(thd->mem_root,
> sizeof(Item*)))) ||
> unlikely(!(*copy->ref= (* ref)->build_clone(thd))))
> + {
> + if (copy)
> + copy->ref= 0;
> return 0;
> + }
> return copy;
> }
Ok code, but I would have prefered this a bit more simple approach:
Item* Item_ref::build_clone(THD *thd)
{
Item_ref *copy= (Item_ref *) get_copy(thd);
if (likely(copy))
{
if (unlikely(!(copy->ref= (Item**) alloc_root(thd->mem_root,
sizeof(Item*)))) ||
unlikely(!(*copy->ref= (* ref)->build_clone(thd))))
{
copy->ref= 0;
return 0;
}
}
return copy;
}
> @@ -5823,22 +5827,48 @@ Item_field::fix_outer_field(THD *thd, Field **from_field, Item **reference)
> select->group_list.elements &&
> (place == SELECT_LIST || place == IN_HAVING))
> {
> - Item_outer_ref *rf;
> /*
> - If an outer field is resolved in a grouping select then it
> - is replaced for an Item_outer_ref object. Otherwise an
> - Item_field object is used.
> - The new Item_outer_ref object is saved in the inner_refs_list of
> - the outer select. Here it is only created. It can be fixed only
> - after the original field has been fixed and this is done in the
> - fix_inner_refs() function.
> - */
> - ;
> - if (!(rf= new (thd->mem_root) Item_outer_ref(thd, context, this)))
> - return -1;
> - thd->change_item_tree(reference, rf);
> - select->inner_refs_list.push_back(rf, thd->mem_root);
> - rf->in_sum_func= thd->lex->in_sum_func;
> + This Item_field represents a reference to a column in some
> + outer select. Wrap this Item_field item into an Item_outer_ref
> + object if we are at the first execution of the query or at
> + processing the prepare command for it. Make this wrapping
> + permanent for the first query execution so that it could be used
> + for next executions of the query.
> + Add the wrapper item to the list st_select_lex::inner_refs_list
> + of the select against which this Item_field has been resolved.
> + */
> + bool is_first_execution= thd->is_first_query_execution();
> + if (is_first_execution ||
> + thd->stmt_arena->is_stmt_prepare())
The above is the same as:
((stmt_arena->is_conventional() ||
stmt_arena->state != STMT_EXECUTED) &&
!stmt_arena->is_stmt_prepare()) ||
stmt_arena->is_stmt_prepare())
=>
((stmt_arena->state == STMT_CONVENTIONAL_EXECUTION ||
stmt_arena->state != STMT_EXECUTED) &&
stmt_arena->state != STMT_INITIALIZED) ||
stmt_arena->state == STMT_INITIALIZED))
=>
(stmt_arena->state != Query_arena::STMT_EXECUTED &&
stmt_arena->state != Query_arena::STMT_INITIALIZED) ||
stmt_arena->state == Query_arena::STMT_INITIALIZED
=>
stmt_arena->state != Query_arena::STMT_EXECUTED
Is it not better to use the above as it is shorter and easier to
understand as there are less hidden abstractions?
Is STMT_EXECUTED safe to use?
We don't set state to STMT_EXECUTE in case of an error during execute.
If the error is temporary (depending on data or out of space during
execution) then state will be left in STMT_INITIALIZED. Don't we have a
problem in this case for the next execution?
In 11.5 we can trivially simulate an error in almost any query by
setting the tmp disk quota to a very low value.
Could be fixed by adding a new state STMT_OPTIMIZED that tells us that
all optimizations rewrites has been done and then we could check for
STMT_OPTIMIZED || STMT_EXECUTED or force a re-prepare if the previous
execution returned an error (may be can set state to STMT_ERROR in
this case)?
Another options is for a new prepare in the case the last query ended
with an error.
> + /*
> + If an outer field is resolved in a grouping select then it
> + is replaced for an Item_outer_ref object. Otherwise an
> + Item_field object is used.
> + The new Item_outer_ref object is saved in the inner_refs_list of
> + the outer select. Here it is only created. It can be fixed only
> + after the original field has been fixed and this is done in the
> + fix_inner_refs() function.
> + */
> + if (!(rf= new (thd->mem_root) Item_outer_ref(thd, context, this)))
> + rc= -1;
> + select->inner_refs_list.push_back(rf, thd->mem_root);
> + if (is_first_execution)
> + *reference= rf;
> + else
> + thd->change_item_tree(reference, rf);
> + if (arena)
> + thd->restore_active_arena(arena, &backup);
> + if (rc)
> + return rc;
> + rf->in_sum_func= thd->lex->in_sum_func;
> + }
> }
I don't like that we use 'rf' when we know it is 0. We can fix that
with the following code change that also removes the 'rc' variable
(shorter):
{
Query_arena *arena=0, backup;
Item_outer_ref *rf;
bool is_first_execution;
if ((is_first_execution= thd->is_first_query_execution()) &&
!thd->is_conventional())
arena= thd->activate_stmt_arena_if_needed(&backup);
/*
If an outer field is resolved in a grouping select then it
is replaced for an Item_outer_ref object. Otherwise an
Item_field object is used.
The new Item_outer_ref object is saved in the inner_refs_list of
the outer select. Here it is only created. It can be fixed only
after the original field has been fixed and this is done in the
fix_inner_refs() function.
*/
if (!(rf= new (thd->mem_root) Item_outer_ref(thd, context, this)))
{
select->inner_refs_list.push_back(rf, thd->mem_root);
if (is_first_execution)
*reference= rf;
else
thd->change_item_tree(reference, rf);
}
if (arena)
thd->restore_active_arena(arena, &backup);
if (!rf) // Memory allocation failed
return -1
rf->in_sum_func= thd->lex->in_sum_func;
}
Now code has exactly the same amount of jump instructions as your
code, but it is shorter, avoids the usage of 'ref' and does not need 'rc'.
Note that in comparing to the original code, we are creating an arena for the
states:
STMT_INITIALIZED, STMT_INITIALIZED_FOR_SP= 1, STMT_PREPARED= 2, STMT_ERROR= -1
This is probably ok, just wanted to ensure you are aware of this.
Would it be enough/safer to just test for SMT_PREPARED to see if an
arena should be used? (Just wondering)
> /*
> A reference is resolved to a nest level that's outer or the same as
> @@ -5938,7 +5968,7 @@ Item_field::fix_outer_field(THD *thd, Field **from_field, Item **reference)
> else if (ref != not_found_item)
> {
> Item *save;
> - Item_ref *rf;
> + Item_ref *rf= NULL;;
remove extra ;
> /* Should have been checked in resolve_ref_in_select_and_group(). */
> DBUG_ASSERT(*ref && (*ref)->is_fixed());
> @@ -5950,35 +5980,64 @@ Item_field::fix_outer_field(THD *thd, Field **from_field, Item **reference)
<cut>
> + This Item_field represents a reference to a column in some outer select.
> + Wrap this Item_field item into an object of the Item_ref class or a class
> + derived from Item_ref we are at the first execution of the query or at
> + processing the prepare command for it. Make this wrapping permanent for
> + the first query execution so that it could be used for next executions
> + of the query. The type of the wrapper depends on the place where this
> + item field occurs or on type of the query. If it is in the the having clause
> + we use a wrapper of the Item_ref type. Otherwise we use a wrapper either
> + of Item_direct_ref type or of Item_outer_ref. The latter is used for
> + grouping queries. Such a wrapper is always added to the list inner_refs_list
> + for the select against which the outer reference has been resolved.
> */
> + bool is_first_execution= thd->is_first_query_execution();
> + if (is_first_execution ||
> + thd->stmt_arena->is_stmt_prepare())
> + {
Replace with
if (thd->stmt_arena->state != Query_arena::STMT_EXECUTED)
{
bool is_first_execution= thd->is_first_query_execution();
> + int rc= 0;
> + Query_arena *arena, backup;
> + arena= 0;
> + if (is_first_execution)
> + arena= thd->activate_stmt_arena_if_needed(&backup);
> + rf= (place == IN_HAVING ?
> + new (thd->mem_root)
> + Item_ref(thd, context, ref, table_name,
> + field_name, alias_name_used) :
> + (!select->group_list.elements ?
> + new (thd->mem_root)
> + Item_direct_ref(thd, context, ref, table_name,
> + field_name, alias_name_used) :
> + new (thd->mem_root)
> + Item_outer_ref(thd, context, ref, table_name,
> + field_name, alias_name_used)));
> + *ref= save;
> + if (!rf)
> + rc= -1;
> + if (!rc && place != IN_HAVING && select->group_list.elements)
> + {
> + outer_context->select_lex->inner_refs_list.push_back(
> + (Item_outer_ref*)rf, thd->mem_root);
> + ((Item_outer_ref*)rf)->in_sum_func= thd->lex->in_sum_func;
> + }
> + if (is_first_execution)
> + *reference= rf;
> + else
> + thd->change_item_tree(reference, rf);
> + if (arena)
> + thd->restore_active_arena(arena, &backup);
> + if (rc)
> + return rc;
Pleace do same change as in previous case by using an 'if' instead of
setting rc.
> We can not "move" aggregate function in the place where
> @@ -6003,22 +6062,46 @@ Item_field::fix_outer_field(THD *thd, Field **from_field, Item **reference)
> this, (Item_ident*)*reference, false);
> if (last_checked_context->select_lex->having_fix_field)
> {
> + This Item_field represents a reference to a column in having clause
> + of some outer select. Wrap this Item_field item into an Item_ref object
> + if we are at the first execution of the query or at processing the
> + prepare command for it. Make this wrapping permanent for the first query
> + execution so that it could be used for next executions of the query.
> */
> - DBUG_ASSERT(!rf->fixed); // Assured by Item_ref()
> - if (rf->fix_fields(thd, reference) || rf->check_cols(1))
> - return -1;
> + Item_ref *rf;
> + bool is_first_execution= thd->is_first_query_execution();
> + if (is_first_execution ||
> + thd->stmt_arena->is_stmt_prepare())
Fix test, as in other cases.
> + {
> + int rc= 0;
> + Query_arena *arena, backup;
> + arena= 0;
> + if (is_first_execution)
> + arena= thd->activate_stmt_arena_if_needed(&backup);
> + rf= new (thd->mem_root) Item_ref(thd, context,
> + (*from_field)->table->s->db,
> + Lex_cstring_strlen((*from_field)->
> + table->alias.c_ptr()),
> + field_name);
> + if (!rf)
> + rc= -1;
> + if (is_first_execution)
> + *reference= rf;
> + else
> + thd->change_item_tree(reference, rf);
> + if (arena)
> + thd->restore_active_arena(arena, &backup);
> + if (rc)
> + return rc;
> + /*
> + rf is Item_ref => never substitute other items (in this case)
> + during fix_fields() => we can use rf after fix_fields()
> + */
> + DBUG_ASSERT(!rf->fixed); // Assured by Item_ref()
> + if (rf->fix_fields(thd, reference) || rf->check_cols(1))
> + return -1;
> + }
same here.
Change
if (!rf)
rc= -1;
...
if (arena)
thd->restore_active_arena(arena, &backup);
to
if (rc)
{
....
}
if (arena)
thd->restore_active_arena(arena, &backup);
> @@ -8321,6 +8404,9 @@ bool Item_ref::fix_fields(THD *thd, Item **reference)
> goto error;
> }
> + if ((*ref)->fix_fields_if_needed(thd, reference))
> + goto error;
> +
> set_properties();
Add a comment when this is needed (ie, when reference <> 0 and it is not fixed.
I tried to come up with a simple comment but failed :(
I assume this is at least true for the prepare phase of a prepared statement.
>
> if ((*ref)->check_cols(1))
> @@ -9282,8 +9368,21 @@ bool Item_direct_view_ref::fix_fields(THD *thd, Item **reference)
> bitmap_set_bit(fld->table->read_set, fld->field_index);
> }
> }
> - else if ((*ref)->fix_fields_if_needed(thd, ref))
> - return TRUE;
> + else
> + {
> + if (thd->is_noninitial_query_execution())
replace
(thd->is_noninitial_query_execution())
with
thd->is_stmt_executed()
See later comments in definition of is_noninitial_query_execution()
> + {
> + bool rc= false;
> + bool save_wrapper= thd->lex->current_select->no_wrap_view_item;
> + thd->lex->current_select->no_wrap_view_item= TRUE;
> + rc= (*ref)->fix_fields_if_needed(thd, ref);
> + thd->lex->current_select->no_wrap_view_item= save_wrapper;
> + if (rc)
> + return TRUE;
> + }
> + else if ((*ref)->fix_fields_if_needed(thd, ref))
> + return TRUE;
> + }
Do we need no_wrap_view_item anymore (we probably do, but it is not obvious)?
The original code that introduced no_wrap_view_item used it to test
things in sql_base.cc
but that code does not exist anymore.
The only usage we have of it now is in item.cc:
if (!thd->lex->current_select->no_wrap_view_item &&
thd->lex->in_sum_func &&
thd->lex->in_sum_func->nest_level ==
select->nest_level)
set_if_bigger(thd->lex->in_sum_func->max_arg_level,
select->nest_level);
There is no code comment what the above code is supposed to do.
Could you PLEASE add a comment about what is the purpose o the above
code, especially
why the nest_level is not applicable to no_wrap_view_items ?
I have verified that we need
!thd->lex->current_select->no_wrap_view_item here. We
get crashes in main.derived_view if remove the test.
> diff --git a/sql/item_cmpfunc.cc b/sql/item_cmpfunc.cc
> index f8959cfd2bf..cc9c81608ca 100644
> --- a/sql/item_cmpfunc.cc
> +++ b/sql/item_cmpfunc.cc
> @@ -4588,7 +4588,9 @@ bool Item_func_in::value_list_convert_const_to_int(THD *thd)
> */
> if (arg[0]->type() != Item::NULL_ITEM &&
> !convert_const_to_int(thd, field_item, &arg[0]))
> - all_converted= false;
> + all_converted= false;
> + else
> + block_transform_into_subq= true;
> }
if (all_converted)
m_comparator.set_handler(&type_handler_slonglong);
The above looks strange.
You are setting block_transform_into_subq to true in the case of
IN (1,1)
but not setting it for ("abb",NULL);
I assume the idea is that if if convert_const_to_int() changes
anything, then we block conversion of the IN to sub queries as our code
cannot convert back to use the original code in this case?
(as the translation to use subqueries is permanent).
Here is a test cases the sets block_transform_into_subq to true:
create table t1 (a bigint);
insert into t1 select seq from seq_1_to_100;
select benchmark(1,1);
select * from t1 where a in ('1','2','3');
drop table t1;
Is there not a better way to do this ?
- Make all translations of strings to integers permantently in the
statement arena
- Keep the derived table around for next execution (which would be a
nice speedup).
If nothing can be done to improve things, we should ae least move
setting block_transform_into_subq to the end of the function and fix that we
only block the transformation for prepared statements:
> if (all_converted)
> m_comparator.set_handler(&type_handler_slonglong);
->
if (all_converted)
{
m_comparator.set_handler(&type_handler_slonglong);
if (thd->is_first_query_execution())
block_transform_into_subq= 1;
}
----
> diff --git a/sql/item_cmpfunc.h b/sql/item_cmpfunc.h
> index 465625f69bf..1404fa30d6b 100644
> --- a/sql/item_cmpfunc.h
> +++ b/sql/item_cmpfunc.h
> @@ -2401,6 +2401,7 @@ class Item_func_in :public Item_func_opt_neg,
> {
> return check_argument_types_like_args0();
> }
> + bool block_transform_into_subq;
Please add a comment about the purpose of this variable, when it
should be used and how it is used.
As far as I can see, this set in the case where an IN() requires a
item conversion
in which case we have to block it for prepared statements as we cannot
roll it back.
> diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc
> index ff84e6b7bf9..4dcd1f3ae9d 100644
> --- a/sql/item_subselect.cc
> +++ b/sql/item_subselect.cc
> @@ -104,6 +104,16 @@ void Item_subselect::init(st_select_lex *select_lex,
> unit->item= this;
> engine->change_result(this, result, FALSE);
> }
> + else if ((unit->item->substype() == ALL_SUBS ||
> + unit->item->substype() == ANY_SUBS) &&
> + (((Item_in_subselect*) unit->item)->
> + test_set_strategy(SUBS_MAXMIN_INJECTED) ||
> + ((Item_in_subselect*) unit->item)->
> + test_set_strategy(SUBS_MAXMIN_ENGINE)))
> + {
> + unit->item= this;
> + engine->change_result(this, result, FALSE);
> + }
> else
> {
> /*
Better to combine the test with the previous test as the code inside
the {} is identical?
Note that I understand the above code to 100%, so adding a comment
what the purpose of
this code would be great.
Also change the function 'test_set_strategy()' to
'test_choosen_strategy(SUBS_MAXMIN_INJECTED | SUBS_MAXMIN_INJECTED)'
This will make the code shorter and easier to read.
Later in the review there is a new definition for test_choosen_strategy().
This is very similar to test_strategy(), but also ensure that the
strategy is chosen.
> @@ -200,15 +211,6 @@ void Item_in_subselect::cleanup()
>
> void Item_allany_subselect::cleanup()
> {
> - /*
> - The MAX/MIN transformation through injection is reverted through the
> - change_item_tree() mechanism. Revert the select_lex object of the
> - query to its initial state.
> - */
> - for (SELECT_LEX *sl= unit->first_select();
> - sl; sl= sl->next_select())
> - if (test_set_strategy(SUBS_MAXMIN_INJECTED))
> - sl->with_sum_func= false;
> Item_in_subselect::cleanup();
> }
I assume the above is now removed becasue MAX/MIN optimization changes are now
permanent in Item_allany_subselect::transform_into_max_min(JOIN *join).
Please add an explanation for the above change to the commit message
and also add a note that thanks to this change we don't need to
cleanup for MAX/MIN optimization.
> @@ -497,6 +505,9 @@ void Item_subselect::recalc_used_tables(st_select_lex *new_parent,
> Ref_to_outside *upper;
> DBUG_ENTER("recalc_used_tables");
>
> + if (!unit->thd->is_first_query_execution())
> + DBUG_VOID_RETURN;
With prepared statements, the optimizer plan can change for each query.
How come we don't need to recalc_used_tables() just because we have
done one execution before?
> @@ -2143,11 +2155,11 @@ bool Item_allany_subselect::transform_into_max_min(JOIN *join)
> }
> if (upper_item)
> upper_item->set_sum_test(item);
> - thd->change_item_tree(&select_lex->ref_pointer_array[0], item);
> + select_lex->ref_pointer_array[0]= item;
> {
> List_iterator<Item> it(select_lex->item_list);
> it++;
> - thd->change_item_tree(it.ref(), item);
> + it.replace(item);
> }
Looks like we are now are doing the MAX/MIN transformation permanently.
> +uint st_select_lex_unit::ub_eq_for_exists2_in()
> +{
> + if (derived && derived->is_materialized_derived())
> + return 0;
> + st_select_lex *sl= first_select();
> + if (sl->next_select())
> + return 0;
> + uint n= sl->select_n_eq;
> + for (st_select_lex_unit *unit= sl->first_inner_unit();
> + unit;
> + unit= unit->next_unit())
> + {
> + n+= unit->ub_eq_for_exists2_in();
> + }
> + return n;
> +}
Please use a more clear function name. 'ub' is not clear.
Please also add a function comment to explain the purpose if this function
as I do not understand the purpose of it.
The function is is used here:
bool Item_exists_subselect::fix_fields(THD *thd, Item **ref)
{
DBUG_ENTER("Item_exists_subselect::fix_fields");
st_select_lex *sel= unit->first_select();
sel->select_n_reserved= unit->ub_eq_for_exists2_in();
But it's purpose is not that clear or what it trying to calculate.
For example, if you have a query:
SELECT (a=1) + (b=2) + (c=3)
select_n_eq will be 3.
In case of
SELECT (a = 1 or b = 1) or (a=2 or b=4)
select_n_eq will be 4
I don't see how either value can be of any use for the optimizer.
Why not instead calculate the number of = operations in the top level
Item_comp_and() ? You could have the counter in Item_comp_and of
how many '=' comparisons there are inside it.
This would give you an exact number of = that may be needed, compared
to the current code.
Note find_inner_outer_equalities() is only appending items from the top
and level or alternatively 0 or 1 element for other items.
> +bool Item_singlerow_subselect::test_set_strategy(uchar strategy_arg)
> +{
> + DBUG_ASSERT(strategy_arg == SUBS_MAXMIN_INJECTED ||
> + strategy_arg == SUBS_MAXMIN_ENGINE);
> + return ((strategy & SUBS_STRATEGY_CHOSEN) &&
> + (strategy & ~SUBS_STRATEGY_CHOSEN) == strategy_arg);
> +}
Change to the following so that we can check all bits at once, like
in test_strategy().
bool Item_singlerow_subselect::test_chosen_strategy(uint strategy_arg)
{
DBUG_ASSERT(strategy_arg &&
!(strategy_arg & ~(SUBS_MAXMIN_INJECTED | SUBS_MAXMIN_ENGINE)));
return ((strategy & SUBS_STRATEGY_CHOSEN) &&
(strategy & strategy_arg);
}
> diff --git a/sql/item_subselect.h b/sql/item_subselect.h
> index d3572e1abe7..9e318e74bab 100644
> --- a/sql/item_subselect.h
> +++ b/sql/item_subselect.h
> @@ -302,6 +302,7 @@ class Item_singlerow_subselect :public Item_subselect
> {
> protected:
> Item_cache *value, **row;
> + uchar strategy;
Change to uint16 to ensure that we can add more bits later without having
to change a lot of code (thanks to alignment, the storage required
does not change if we use uint16)
> diff --git a/sql/item_sum.cc b/sql/item_sum.cc
> index 4cf403c1618..e1a32185ad1 100644
> --- a/sql/item_sum.cc
> +++ b/sql/item_sum.cc
> @@ -93,10 +93,15 @@ bool Item_sum::init_sum_func_check(THD *thd)
> thd->lex->in_sum_func= this;
> nest_level= thd->lex->current_select->nest_level;
> ref_by= 0;
> + if (!thd->is_noninitial_query_execution() ||
> + (thd->active_stmt_arena_to_use()->state ==
> + Query_arena::STMT_SP_QUERY_ARGUMENTS))
> + {
> aggr_level= -1;
> aggr_sel= NULL;
> max_arg_level= -1;
> max_sum_func_level= -1;
> + }
Please fix the indentation.
Please add a comment for the above test. Here is a suggestion:
/*
Item_sum objects are persistent and does not have to be updated
for pre-executed prepared statements (stmt_arena->state == EXECUTED) except
when setting variables for stored procedures
(state == Query_arena::STMT_SP_QUERY_ARGUMENTS))
*/
> outer_fields.empty();
> return FALSE;
> }
> @@ -409,7 +414,7 @@ bool Item_sum::register_sum_func(THD *thd, Item **ref)
> sl= sl->master_unit()->outer_select() )
> sl->master_unit()->item->get_with_sum_func_cache()->set_with_sum_func();
> }
> - if (aggr_sel)
> + if (aggr_sel && aggr_sel != thd->lex->current_select )
> thd->lex->current_select->mark_as_dependent(thd, aggr_sel, NULL);
Remove extra space before && and before )
Please add comment under which circumstances this is true:
aggr_sel != thd->lex->current_select
I would like to understand why this was not needed before.
> +++ b/sql/opt_subselect.cc
> @@ -962,8 +962,13 @@ bool JOIN::transform_max_min_subquery()
> if (!subselect || (subselect->substype() != Item_subselect::ALL_SUBS &&
> subselect->substype() != Item_subselect::ANY_SUBS))
> DBUG_RETURN(0);
> - DBUG_RETURN(((Item_allany_subselect *) subselect)->
> - transform_into_max_min(this));
> + bool rc= false;
> + Query_arena *arena, backup;
> + arena= thd->activate_stmt_arena_if_needed(&backup);
> + rc= (((Item_allany_subselect *) subselect)->transform_into_max_min(this));
> + if (arena)
> + thd->restore_active_arena(arena, &backup);
> + DBUG_RETURN(rc);
> }
Please update the comment for this function to make it clear that the changes
to use min/max are now permanent.
>
> @@ -1889,7 +1894,7 @@ static bool convert_subq_to_sj(JOIN *parent_join, Item_in_subselect *subq_pred)
> with thd->change_item_tree
> */
> Item_func_eq *item_eq=
> - new (thd->mem_root) Item_func_eq(thd, left_exp_orig,
> + new (thd->mem_root) Item_func_eq(thd, left_exp,
> subq_lex->ref_pointer_array[0]);
> if (!item_eq)
> goto restore_tl_and_exit;
I assume this was a bug in the old code?
I checked the history and this line has changed back and between
left_exp_orig and left_exp over time.
The documentation for left_expr_orig is:
/*
Important for PS/SP: left_expr_orig is the item that left_expr originally
pointed at. That item is allocated on the statement arena, while
left_expr could later be changed to something on the execution arena.
*/
Item *left_expr_orig;
As an experiment I added left_exp_orig back and did run the test that you
had changed in your commit with valgrind. All test passed.
Do you have any test that shows that left_exp is the right value here or do
you have any other proof why left_exp is right ?
> @@ -6535,6 +6540,10 @@ bool JOIN::choose_subquery_plan(table_map join_tables)
> if (is_in_subquery())
> {
> in_subs= unit->item->get_IN_subquery();
> + if (in_subs->test_set_strategy(SUBS_MAXMIN_INJECTED))
> + return false;
> + if (in_subs->test_set_strategy(SUBS_MAXMIN_ENGINE))
> + return false;
> if (in_subs->create_in_to_exists_cond(this))
> return true;
You can replace the above two changed lines with:
if (in_subs->test_chosen_strategy(SUBS_MAXMIN_INJECTED | SUBS_MAXMIN_INJECTED))
> diff --git a/sql/sql_array.h b/sql/sql_array.h
> index b6de1b18d78..b9fdef0623c 100644
> --- a/sql/sql_array.h
> +++ b/sql/sql_array.h
> @@ -39,7 +39,10 @@ template <typename Element_type> class Bounds_checked_array
>
> Bounds_checked_array(Element_type *el, size_t size_arg)
> : m_array(el), m_size(size_arg)
> - {}
> + {
> + if (!m_size)
> + m_array= NULL;
> + }
Strange that one would call it with m_size() with el != 0 and m_size == 0.
Sounds like a bug in the caller. I would change the above to be a DBUG_ASSERT
instead.
> void reset() { m_array= NULL; m_size= 0; }
>
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index f1fd0053a82..a6cdd218c44 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -5783,10 +5783,7 @@ find_field_in_view(THD *thd, TABLE_LIST *table_list,
> {
> if (!my_strcasecmp(system_charset_info, field_it.name()->str, name))
> {
> - // in PS use own arena or data will be freed after prepare
> - if (register_tree_change &&
> - thd->stmt_arena->is_stmt_prepare_or_first_stmt_execute())
> - arena= thd->activate_stmt_arena_if_needed(&backup);
> + arena= thd->activate_stmt_arena_if_needed(&backup);
Why this change?
Will it not use statement area for a lot of cases where it did not before?
like for state == STMT_EXECUTED ?
- This was discussed on slack, but we did not have time to reach a conclusion.
> @@ -5805,11 +5802,23 @@ find_field_in_view(THD *thd, TABLE_LIST *table_list,
> the replacing item.
> */
> if (*ref && !(*ref)->is_autogenerated_name())
> + {
> + arena=0;
> + if (thd->is_first_query_execution())
> + arena= thd->activate_stmt_arena_if_needed(&backup);
> item->set_name(thd, (*ref)->name);
> + if (arena)
> + thd->restore_active_arena(arena, &backup);
Item::set_name() does never allocate any memory, so we don't need arena here.
(I checked all code on the sql level).
> + }
> + if (item != *ref)
> + {
> + if (register_tree_change &&
> + !thd->is_first_query_execution())
The above is same as:
register_tree_change &&
!(stmp_arena->state != STMT_INITIALIZED &&
stmt_arena->state != Query_arena::STMT_EXECUTED)
=>
register_tree_change &&
(stmp_arena->state == STMT_INITIALIZED ||
stmt_arena->state == STMT_EXECUTED))
Is the above correct and the intention of this cde?
> + thd->change_item_tree(ref, item);
> + else
> + *ref= item;
> + }
You can remove the else before *ref. It is faster to update *ref than handle
the jump.
I am still a bit concerned that we set *ref=item when
register_tree_change != 0 as our code comments says that if
register_tree_change is set, then *ref will be removed before next
execution and any tries to acces it can result in a core dump.
> @@ -7639,10 +7648,14 @@ bool setup_fields(THD *thd, Ref_ptr_array ref_pointer_array,
> TODO: remove it when (if) we made one list for allfields and
> ref_pointer_array
> */
> - if (!ref_pointer_array.is_null())
> + if (thd->is_first_query_execution() ||
> + thd->stmt_arena->is_stmt_prepare())
The new test is same as (see earlier examples):
thd->stmt_arena->state != Query_arena::STMT_EXECUTED
Please replace the test with the above or replace it with is_stmt_exceuted()
<cut>
> /*
> @@ -7682,17 +7695,6 @@ bool setup_fields(THD *thd, Ref_ptr_array ref_pointer_array,
> ref[0]= item;
> ref.pop_front();
> }
> - /*
> - split_sum_func() must be called for Window Function items, see
> - Item_window_func::split_sum_func.
> - */
> - if (sum_func_list &&
> - ((item->with_sum_func() && item->type() != Item::SUM_FUNC_ITEM) ||
> - item->with_window_func))
> - {
> - item->split_sum_func(thd, ref_pointer_array, *sum_func_list,
> - SPLIT_SUM_SELECT);
> - }
I assume this is moved part of this from setup_fields to JOIN::prepare().
Any particular reason why this move was required ?
Just curious. Still, it would be good to explain this in the commit message.
(One reason is that it will allow us to remove sum_func_list from the
arguments to setup_fields)
I don't see any big problem with the code as it is JOIN::prepare() that is
calling both setup_fields() and split_sum_func().
It will be a bit slower as we have to traverse the field_list twice, but
that is not a high cost.
Note that if we go with the above code change, you should also remove
sum_func_list from the arguments as the above code was the only place it
was used.
Also, please add back the comment to the moved code:
/*
split_sum_func() must be called for Window Function items, see
Item_window_func::split_sum_func.
*/
> diff --git a/sql/sql_class.cc b/sql/sql_class.cc
> index 7913f1a40d2..19f1ec9728e 100644
> --- a/sql/sql_class.cc
> +++ b/sql/sql_class.cc
> @@ -3959,12 +3962,16 @@ void Query_arena::free_items()
> Item *next;
> DBUG_ENTER("Query_arena::free_items");
> /* This works because items are allocated on THD::mem_root */
> + for (next= free_list; next; next= next->next)
> + {
> + next->cleanup();
> + }
Why not call cleanup in the following loop instead of looping over
the elements twice (as we did before)?
Is there some cases when we cannot do cleanup as part of delete_self()?
If yes, please add comment in the code why we have to traverse the list twice
as otherwise it is very likely some developer will try to optimize the
first loop away while looking at the code.
If this is the case, is this not a problem also for free_items() ?
> diff --git a/sql/sql_class.h b/sql/sql_class.h
> index 7e5d9ac96e3..2f05b2dbd03 100644
> --- a/sql/sql_class.h
> +++ b/sql/sql_class.h
> @@ -2569,6 +2569,20 @@ class THD: public THD_count, /* this must be first */
> return static_cast<PSI_thread*>(my_atomic_loadptr((void*volatile*)&m_psi));
> }
>
> + inline bool is_first_query_execution()
> + {
> + return (stmt_arena->is_conventional() ||
> + stmt_arena->state != Query_arena::STMT_EXECUTED) &&
> + !stmt_arena->is_stmt_prepare();
(stmt_arena->is_conventional() ||
stmt_arena->state != Query_arena::STMT_EXECUTED) &&
!stmt_arena->is_stmt_prepare()
=>
(stmt_arena->state == STMT_CONVENTIONAL_EXECUTION ||
stmt_arena->state != STMT_EXECUTED) &&
stmp_arena->state != STMT_INITIALIZED)
=>
stmt_arena->state != STMT_INITIALIZED &&
stmt_arena->state != STMT_EXECUTED
Simpler and easier to understand.
> + }
> +
> + inline bool is_noninitial_query_execution()
> + {
> + return !stmt_arena->is_conventional() &&
> + stmt_arena->state == Query_arena::STMT_EXECUTED &&
> + !stmt_arena->is_stmt_prepare() ;
Please add comment to explain this function and when to use it.
(name of is_first_query_execution() is clear, but this one is not)
Anyway:
!stmt_arena->is_conventional() &&
stmt_arena->state == Query_arena::STMT_EXECUTED &&
!stmt_arena->is_stmt_prepare() ;
=>
stmt_arena->state != STMT_CONVENTIONAL_EXECUTION &&
stmt_arena->state == STMT_EXECUTED &&
stmt_arena->state != STMT_INITIALIZED
=>
stmt_arena->state == STMT_EXECUTED
Better to renamed it to is_stmt_executed() and move it to where we
define is_stmt_prepare() in class THD and class Query_arena
> diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc
> index b669925a263..d7b7e98a6e4 100644
> --- a/sql/sql_lex.cc
> +++ b/sql/sql_lex.cc
> @@ -3601,6 +3607,8 @@ ulong st_select_lex::get_table_join_options()
>
> uint st_select_lex::get_cardinality_of_ref_ptrs_slice(uint order_group_num_arg)
> {
> + if (card_of_ref_ptrs_slice)
> + return card_of_ref_ptrs_slice;
I assume this is just a cache and the code should work also without it?
If yes, please add a comment about that.
> if (!((options & SELECT_DISTINCT) && !group_list.elements))
> hidden_bit_fields= 0;
>
> @@ -3620,20 +3628,23 @@ uint st_select_lex::get_cardinality_of_ref_ptrs_slice(uint order_group_num_arg)
> order_group_num * 2 +
> hidden_bit_fields +
> fields_in_window_functions;
> + card_of_ref_ptrs_slice= n;
> return n;
> }
Does caching card_of_ref_ptrs_slice really help ?
(The function is quite trivial)
We call the function in one place, setup_ref_array()
It is called once per query (For selectr in JOIN::prepare())
I think having the variable and testing and updating clearing it for
each normal query will cost us more than not having it.
This assumes we have more normal queries than we have prepared
statements.
I would suggest we remove the variable.
> -bool st_select_lex::setup_ref_array(THD *thd, uint order_group_num)
> +static
> +bool setup_ref_ptrs_array(THD *thd, Ref_ptr_array *ref_ptrs, uint n_elems)
> {
> + if (!ref_ptrs->is_null())
> + {
> + if (ref_ptrs->size() >= n_elems)
> + return false;
> + }
> Item **array= static_cast<Item**>(
> thd->active_stmt_arena_to_use()->alloc(sizeof(Item*) * n_elems));
> if (likely(array != NULL))
> + *ref_ptrs= Ref_ptr_array(array, n_elems);
> return array == NULL;
> }
I assume the change is that the new code will alloocate a new array
if the old array was not big enough.
That is good. Howevever it would be good to know when this can happen.
For ref_pointer_array this should be impossible as we cache the value
and it cannot change for two iterations.
Is this reallocate needed for the save_ref_ptrs array?
Please add a comment to explain in which case the old array was not
big enough
It would also be good to have the code for setup_ref_ptr_array,
setup_ref_array and save_ref_ptrs_after_persistent_rewrites and
save_ref_ptrs_if_needed close to each other as these are very similar
and very related.
> +
> +bool st_select_lex::setup_ref_array(THD *thd, uint order_group_num)
> +{
> + /*
> + We have to create array in prepared statement memory if it is a
> + prepared statement
> + */
> + uint slice_card= !thd->is_noninitial_query_execution() ?
> + get_cardinality_of_ref_ptrs_slice(order_group_num) :
> + card_of_ref_ptrs_slice;
> + uint n_elems= slice_card * 5;
> + DBUG_ASSERT(n_elems % 5 == 0);
You can remove the DBUG_ASSERT. It is obvious that this is always
correct (for this code).
> + return setup_ref_ptrs_array(thd, &ref_pointer_array, n_elems);
> +}
With the cache, why don't have to test for
!thd->is_noninitial_query_execution(). This should be good enough:
uint slice_card= get_cardinality_of_ref_ptrs_slice(order_group_num);
With the cache, it will return at once after the first call.
As the cache is updated on first run, the answer should always be the
same is in your original code.
> +bool st_select_lex::save_ref_ptrs_after_persistent_rewrites(THD *thd)
> +{
> + uint n_elems= card_of_ref_ptrs_slice;
> + if (setup_ref_ptrs_array(thd, &save_ref_ptrs, n_elems))
> + return true;
> + if (!ref_pointer_array.is_null())
> + join->copy_ref_ptr_array(save_ref_ptrs, join->ref_ptr_array_slice(0));
> + return false;
> +}
Please add to function start:
/* Ensure that setup_ref_array has been called */
DBUG_ASSERT(card_of_ref_ptrs_slice);
It looks like all calls to setup_ref_ptrs_array() are using the same
value for all future calls. If this is the case, why did you change
setup_ref_ptrs_array() to check if number of elements has grown?
If this is not needed, remove the check and add an assert()
to ensure that the elements does not grow over time.
> +bool st_select_lex::save_ref_ptrs_if_needed(THD *thd)
> +{
> + if (thd->is_first_query_execution())
> + {
> + if (save_ref_ptrs_after_persistent_rewrites(thd))
> + return true;
> + }
> + return false;
> +}
The above if is true in case of
stmt_arena->state != STMT_INITIALIZED &&
stmt_arena->state != STMT_EXECUTED
We call save_ref_prts_if_needed in JOIN::prepare() and
subselect_single_select_engine::prepare(), mysql_select() and
st_select_lex_unit::prepare_join
I assume this is ok.
However, I did find some inconsistently in how this functioin is called.
JOIN::prepare() calls it on errors before returning (for most cases but not
all).
subselect_single_select_engine::prepare() & mysql_select()
calls it if join->prepare() returns error, but not otherwise.
st_select_lex_unit::prepare_join() calls it always after prepare.
I would like to know when we MUST call save_ref_prts_if_needed().
Do we have to call it in case JOIN::prepare returns 0 ?
It would be easier if things would be done this way:
- JOIN::prepare() would always call it in case of error if there is any
need to call it.
- We would not have to call it in
subselect_single_select_engine::prepare() or mysql_select()
- We would remove the call in st_select_lex_unit::prepare_join() or
alternative call it only if join->prepare() returns 0.
(Depending on what is correct here)
> void st_select_lex_unit::print(String *str, enum_query_type query_type)
> {
> if (with_clause)
> @@ -4958,7 +5007,7 @@ bool st_select_lex::optimize_unflattened_subqueries(bool const_only)
> }
> if ((res= inner_join->optimize()))
> return TRUE;
> - if (!inner_join->cleaned)
> + if (inner_join->thd->is_first_query_execution())
> sl->update_used_tables();
> sl->update_correlated_cache();
> is_correlated_unit|= sl->is_correlated;
Fix intendation.
Why is cleaned not good enough here?
cleaned means that the join structure cannot be used anymore.
Why not do:
if (!inner_join->cleaned && inner_join->thd->is_first_query_execution())
Should be safer.
> diff --git a/sql/sql_lex.h b/sql/sql_lex.h
> index 3ab50d4aaa8..59029057a23 100644
> --- a/sql/sql_lex.h
> +++ b/sql/sql_lex.h
> @@ -1045,6 +1045,8 @@ class st_select_lex_unit: public st_select_lex_node {
>
> bool can_be_merged();
>
> + uint ub_eq_for_exists2_in();
> +
> friend class st_select_lex;
> };
>
> @@ -1176,6 +1178,7 @@ class st_select_lex: public st_select_lex_node
> uint curr_tvc_name;
> /* true <=> select has been created a TVC wrapper */
> bool is_tvc_wrapper;
> + uint fields_added_by_fix_inner_refs;
You can remove the above variable as it is not used anywhere.
It is set at:
sql_select.cc:1499: select_lex->fields_added_by_fix_inner_refs=
sql_lex.cc:3119: fields_added_by_fix_inner_refs= 0;
but not used anywhere.
> /*
> Needed to correctly generate 'PRIMARY' or 'SIMPLE' for select_type column
> of EXPLAIN
> @@ -1201,6 +1204,8 @@ class st_select_lex: public st_select_lex_node
>
> /// Array of pointers to top elements of all_fields list
> Ref_ptr_array ref_pointer_array;
> + Ref_ptr_array save_ref_ptrs;
> + uint card_of_ref_ptrs_slice;
Please add uint variables together with other uint variables.
Otherwise we may loose 4 byte for each variable because of alignment.
>
> /*
> number of items in select_list and HAVING clause used to get number
> @@ -1220,6 +1225,7 @@ class st_select_lex: public st_select_lex_node
> uint order_group_num;
> /* reserved for exists 2 in */
> uint select_n_reserved;
> + uint select_n_eq;
Please add a descripton for the above variable.
(Name does not tell me what it is used for).
> /*
> it counts the number of bit fields in the SELECT list. These are used when DISTINCT is
> converted to a GROUP BY involving BIT fields.
> @@ -1295,6 +1301,8 @@ class st_select_lex: public st_select_lex_node
> case of an error during prepare the PS is not created.
> */
> uint8 changed_elements; // see TOUCHED_SEL_*
> + uint8 save_uncacheable;
> + uint8 save_master_uncacheable;
The above two variables can be removed as these are only assigned to
but never used for anything.
> /* TODO: add foloowing first_* to bitmap above */
> bool first_natural_join_processing;
> bool first_cond_optimization;
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index 5db53b6f2b8..83569207a0c 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -4956,11 +4956,24 @@ mysql_execute_command(THD *thd)
> if ((res= multi_delete_precheck(thd, all_tables)))
> break;
>
> + if (thd->stmt_arena->is_stmt_prepare())
> + {
> + if (add_item_to_list(thd, new (thd->mem_root) Item_null(thd)))
> + goto error;
> + }
> + else if (thd->is_first_query_execution())
> + {
The above tests for
stmt_arena->state != STMT_INITIALIZED &&
stmt_arena->state != STMT_EXECUTED
While .. != STMT_EXECUTED would be enough as it is tested on if above.
No big deal, just and observarion
> + if (select_lex->item_list.elements != 0)
> + select_lex->item_list.empty();
> + bool rc= false;
You don't have to set it to false here.
> + Query_arena *arena= 0, backup;
> + arena= thd->activate_stmt_arena_if_needed(&backup);
> + rc= add_item_to_list(thd, new (thd->stmt_arena->mem_root) Item_null(thd));
> + if (arena)
> + thd->restore_active_arena(arena, &backup);
> + if (rc)
> + goto error;
> + }
> diff --git a/sql/sql_select.cc b/sql/sql_select.cc
> index a7b84bbfe3b..27eb466b47c 100644
> --- a/sql/sql_select.cc
> +++ b/sql/sql_select.cc
> @@ -551,6 +551,19 @@ fix_inner_refs(THD *thd, List<Item> &all_fields, SELECT_LEX *select,
> the found_in_group_by field of the references from the list.
> */
> List_iterator_fast <Item_outer_ref> ref_it(select->inner_refs_list);
> +
> + if (thd->is_noninitial_query_execution())
> + {
> + while ((ref= ref_it++))
> + {
> + if (ref->found_in_select_list)
> + continue;
> + int el= all_fields.elements;
> + all_fields.push_front(ref_pointer_array[el], thd->mem_root);
> + }
> + return false;
> + }
Please move the comment before this loop after the loop as it refers
to the next loop.
Also please add a short comment why the purpose of this loop is.
Will this not cause the ref_pointer_array to grow for each
prepared statement call (after the first one)?
Looking at the other parts of your patch, I thought that the size and content
of ref_pointer_array[] will be fixed after the first call.
(or saved in save_ref_ptrs).
Adding something to ref_pointer_array in case of STMT_EXECUTED sounds
like it is going against the above principles.
> + bool is_first_execution= thd->is_first_query_execution();
> + if (is_first_execution ||
> + thd->stmt_arena->is_stmt_prepare())
The above can be replaced with
if (thd->stmt_area->state != STMT_EXECUTED)
It a bit confusing that we have defined:
inline bool is_stmt_execute() const
{ return state == STMT_PREPARED || state == STMT_EXECUTED; }
as it would be been logical to have
inline bool is_stmt_execute() const
{ return state == STMT_EXECUTED; }
Adding a function is_stmt_executed()
would easily be confused with is_stmt_execute().
Maybe we should rename is_stmt_execute to is_stmt_preared_or_executed()
and add is_stmt_executed() ?
> @@ -1336,18 +1367,13 @@ JOIN::prepare(TABLE_LIST *tables_init, COND *conds_init, uint og_num,
> DBUG_RETURN(-1);
> }
>
> - if (thd->lex->current_select->first_cond_optimization)
> - {
> - if ( conds && ! thd->lex->current_select->merged_into)
> - select_lex->select_n_reserved= conds->exists2in_reserved_items();
> - else
> - select_lex->select_n_reserved= 0;
> - }
I asume this is now what is calcualated in Item_exists_subselect::fix_fields()
and not needed here anymore?
> @@ -1463,6 +1489,38 @@ JOIN::prepare(TABLE_LIST *tables_init, COND *conds_init, uint og_num,
> if (res)
> DBUG_RETURN(res);
>
> + if (thd->is_first_query_execution() ||
> + thd->lex->is_ps_or_view_context_analysis())
> + {
> + if (fix_inner_refs(thd, all_fields, select_lex, ref_ptrs))
> + DBUG_RETURN(-1);
Remove the above outside the if as the code is identical also for the else
part.
> + if (thd->is_first_query_execution() &&
> + all_fields.elements > select_lex->item_list.elements)
> + select_lex->fields_added_by_fix_inner_refs=
> + select_lex->inner_refs_list.elements;
You can remove the above 4 lines as fields_added_by_fix_inner_refs is not
used anywhere.
> + }
> + else
> + {
> + if (fix_inner_refs(thd, all_fields, select_lex, ref_ptrs))
> + DBUG_RETURN(-1);
> + select_lex->uncacheable= select_lex->save_uncacheable;
> + select_lex->master_unit()->uncacheable= select_lex->save_master_uncacheable;
you can remove the above as neither variable is used.
> + }
> +
The above means you can replace the above query block with:
if (fix_inner_refs(thd, all_fields, select_lex, ref_ptrs))
DBUG_RETURN(-1);
> @@ -1624,6 +1681,7 @@ JOIN::prepare(TABLE_LIST *tables_init, COND *conds_init, uint og_num,
> err:
> delete procedure; /* purecov: inspected */
> procedure= 0;
> + select_lex->save_ref_ptrs_if_needed(thd);
> DBUG_RETURN(-1); /* purecov: inspected */
> }
ok. Please check if there is any other case in JOIN::prepare where we
return <> 0 where we should call save_ref_ptrs_if_needed(thd).
By the way, I am not sure why we would save_ref_ptrs_if_needed() at
all in case of errors. The state will in this case not be set to
STMT_EXECUTED and thus the state should not be used by the next queryt.
Please add a comment why we need the result in case of an error.
> // Update used tables after all handling derived table procedures
> - select_lex->update_used_tables();
> + if (select_lex->first_cond_optimization)
> + select_lex->update_used_tables();
Why first_cond_optimization() instead of state != STMT_EXECUTED ?
> - if (transform_max_min_subquery())
> + if (select_lex->first_cond_optimization && transform_max_min_subquery())
> DBUG_RETURN(1); /* purecov: inspected */
>
> if (select_lex->first_cond_optimization)
Please combine the above two ifs
> @@ -2042,6 +2100,11 @@ JOIN::optimize_inner()
<cut>
> + select_lex->save_uncacheable= select_lex->uncacheable;
> + select_lex->save_master_uncacheable= select_lex->master_unit()->uncacheable;
Remove as not used.
> @@ -4832,6 +4895,7 @@ mysql_select(THD *thd, TABLE_LIST *tables, List<Item> &fields, COND *conds,
> if ((err= join->prepare(tables, conds, og_num, order, false, group,
> having, proc_param, select_lex, unit)))
> {
> + select_lex->save_ref_ptrs_if_needed(thd);
Can probably be removed. See previous comment
> @@ -4858,6 +4922,7 @@ mysql_select(THD *thd, TABLE_LIST *tables, List<Item> &fields, COND *conds,
> if ((err= join->prepare(tables, conds, og_num, order, false, group, having,
> proc_param, select_lex, unit)))
> {
> + select_lex->save_ref_ptrs_if_needed(thd);
> goto err;
Can probably be removed. See previous comment
> @@ -25172,14 +25253,33 @@ find_order_in_list(THD *thd, Ref_ptr_array ref_pointer_array,
<cut>
> - from_field= find_field_in_tables(thd, (Item_ident*) order_item, tables,
> - NULL, &view_ref, IGNORE_ERRORS, FALSE,
> - FALSE);
> + if (!thd->is_noninitial_query_execution())
> + {
> + from_field= find_field_in_tables(thd, (Item_ident*) order_item, tables,
> + NULL, &view_ref, IGNORE_ERRORS, FALSE,
> + FALSE);
> + order->view_ref= view_ref;
> + order->from_field= from_field;
> + }
> + else
> + {
> + if (order->view_ref)
> + {
Fix indentation
> + view_ref= order->view_ref;
> + from_field= order->from_field;
> + }
> + else
> + {
Fix indentation
> + from_field= find_field_in_tables(thd, (Item_ident*) order_item,
> + tables, NULL, &view_ref,
> + IGNORE_ERRORS, FALSE, FALSE);
> + }
> + }
> if (!from_field)
> from_field= (Field*) not_found_field;
> }
> @@ -25232,6 +25332,12 @@ find_order_in_list(THD *thd, Ref_ptr_array ref_pointer_array,
> if (found_item != not_found_item)
> {
> order->item= &ref_pointer_array[all_fields.elements-1-counter];
> + if (!thd->is_first_query_execution() &&
> + !thd->lex->is_ps_or_view_context_analysis())
> + {
> + if ((*order->item)->fix_fields_if_needed_for_order_by(thd, order->item))
> + return TRUE;
> + }
The above code is new. Why do need this when we did not need it before?
Please add a comment for the block above.
> order->in_field_list= 0;
> return FALSE;
> }
> diff --git a/sql/sql_union.cc b/sql/sql_union.cc
> index c3c4198439a..5fb31ccbe46 100644
> --- a/sql/sql_union.cc
> +++ b/sql/sql_union.cc
> @@ -1114,6 +1114,8 @@ bool st_select_lex_unit::prepare_join(THD *thd_arg, SELECT_LEX *sl,
> thd_arg->lex->proc_list.first),
> sl, this);
>
> + sl->save_ref_ptrs_if_needed(thd);
You may be able to remove the above, look at other comments.
> @@ -2784,6 +2786,8 @@ bool st_select_lex::cleanup()
>
> if (join)
> {
> + if (join->thd->stmt_arena->is_stmt_prepare())
> + inner_refs_list.empty();
> List_iterator<TABLE_LIST> ti(leaf_tables);
> TABLE_LIST *tbl;
> while ((tbl= ti++))
> @@ -2813,7 +2817,6 @@ bool st_select_lex::cleanup()
> continue;
> error= (bool) ((uint) error | (uint) lex_unit->cleanup());
> }
> - inner_refs_list.empty();
> exclude_from_table_unique_test= FALSE;
> hidden_bit_fields= 0;
> DBUG_RETURN(error);
The above patch looks a bit strange.
We are deleting the join in this function. Isn't the inner_refs_list
depending on the join struct in any way?
One effect of the above is that we are resetting inner_refs_list
only later when excecuting the next query in
st_select_lex::init_select() for anything else than STMT_INITIALIZED.
I tried reverting the fix and I did get errors.
I assume this is because we now only update inner_refs_list once and
permantently during first execution?
It would be good to add some kind of comment in the commit message
about this change.
Why only is_stmt_prepare and not also for STMT_CONVENTIONAL_EXECUTION ?
Regards,
Monty
