Merge lp://qastaging/~vlad-lesin/percona-server/5.6-per_query_variables_settings into lp://qastaging/percona-server/5.6

   - How does it replicate? Would a test be needed for that?
     https://dev.mysql.com/doc/refman/5.6/en/replication-features-variables.html
   - Please add tests for invalid syntax
   - Please add tests for global-only variable use attempts
   - Since it is supported only for variables with certain show type,
     please test that variables with unsupported show types are
     rejected correctly.
   - set_stmt_reset_vars shouldn't assume that the update back to the
     original value might possibly fail. I.e. it can fail, but that
     should be a fatal assertion in a debug build. Thus instead of
     if (!var->check(thd))
       error|= var->update(thd);
     I'd do something like
     DBUG_ASSERT(var->check(thd));
     error|= var->update(thd);
     DBUG_ASSERT(!error);
   - Please add CREATE TABLE STATEMENT(a INT) test to a testcase
   - Lines 2063--2067: one_shot is removed as of 5.6.1.
   - Lines 2093--2094: can this free list swap cause that something
     non related to the per-statement vars might get freed late as
     well? If that's not the case, then a
     DBUG_ASSERT(!thd->free_list) before the set_stmt_get_reset_vars
     loop call would document this. But if it is the case, then care
     must be taken to remove only per-statement var Item instances
     from the thd->free_list.
   - Lines 2106--2112 and 2127--2135: please factor out to a common
     function.
   - Should lines 2240--2244 be sp_create_assignment_lex() call
     instead?
   - Spurious line 2260.
   Minor comments:
   - Please use the disable_warnings; DROP TABLE IF EXISTS;
     enable_warnings; idiom in the testcase.
   - The test depends on t1 being MyISAM. Please either spell it out
     by ENGINE=MyISAM, either look into future-proofing the test by
     using an InnoDB table instead (that would require adjusting Test
     4 accordingly).
   - Testcase comments: s/check/check that/g where appropriate.
   - set_stmt_get_reset_vars header comment: s/will retrieve/retrieves
   - body comment: s/ans/and
   - Diff line 2022: s/Variables list/Variable list
   - Diff line 2086: s/fot/for

> - How does it replicate? Would a test be needed for that?
> https://dev.mysql.com/doc/refman/5.6/en/replication-features-variables.html

There are the following types of variables:
1) variables that are NOT replicated correctly when using STATEMENT mode;
2) variables thar ARE replicated correctly when using STATEMENT mode;
3) sql_mode which is replicated correctly exept NO_DIR_IN_CREATE value;
4) variables that are not replicated at all:
 default_storage_engine, storage_engine, max_heap_table_size;

1)
But as we can set variables in a statements itself and that statements are written to binlog in STATEMENT mode and old values of variables are restored just after statement execution it works correctly.

2)
That variables are saved in Query_log_event on master for each event and restored on slave in Query_log_event::do_apply_event(). "Per query variables settings" code saves old values of variables that are listed in "SET STATEMENT ... FOR ..." statement before query execution and restores them after that. This code works in mysql_execute_command(). There is the following call stack on slave: Query_log_event::do_apply_event()->mysql_parse()->mysql_execute_command(). As variables are restored from mysqlbinlog event by slave thread in Query_log_event::do_apply_event() before "per query variables settings" code saves their old values the old values are lost. But in master the variables are restored to the previous values just after query execution and the next mysqlbinlog event will set them to the previous values on slave. As "SET STATEMENT ... FOR ..." changes only session values I think such behaviour is acceptable.

3)
The NO_DIR_IN_CREATE mode is reset just before event applying on slave in Query_log_event::do_apply_event(). But after that mysql_parse() is invoked and the new value of sql_mode is parsed from statement including NO_DIR_IN_CREATE. For this case we filter out sql_mode variable on slaves during query parsing.

4)
The same is for 3.

> - Please add tests for invalid syntax
Done.

> - Please add tests for global-only variable use attempts
Done

> - Since it is supported only for variables with certain show type,
> please test that variables with unsupported show types are
> rejected correctly.
Done.

> - set_stmt_reset_vars shouldn't assume that the update back to the
> original value might possibly fail. I.e. it can fail, but that
> should be a fatal assertion in a debug build. Thus instead of
> if (!var->check(thd))
> error|= var->update(thd);
> I'd do something like
> DBUG_ASSERT(var->check(thd));
> error|= var->update(thd);
> DBUG_ASSERT(!error);
Done.

> - Please add CREATE TABLE STATEMENT(a INT) test to a testcase
Done.

> - Lines 2063--2067: one_shot is removed as of 5.6.1.
But thd->one_shot and lex->one_shot are still in the code. And it's resonable to take them into account while they are still there.

> - Lines 2093--2094: can this free list swap cause that something
> non related to the per-statement vars might get freed late as
> well? If that's not the case, then a
> DBUG_ASSERT(!thd->free_list) before the set_stmt_get_reset_vars
> loop call woul...

Rebased to new code.

http://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/210/

It looks like tests have not been committed after rebasing.

  - the following assertion

+ if (lex->sql_command == SQLCOM_EXECUTE || lex->sql_command == SQLCOM_PREPARE)
+ {
+ DBUG_ASSERT(thd->free_list == NULL);
+ thd->free_list= stmt_free_list;
+ }

     would fail in the following case:

     SET global_only_var=..., normal_var=... FOR EXECUTE stmt;

     because if an error occurs in one of the set_stmt_get_reset_vars,
     we bail out before saving and resetting thd->free_list.

  - the following code will not be resetting variables to their previous
    values in release builds (i.e. var->update() will not be called at
    all):

+ while ((var = it++))
+ {
+ DBUG_ASSERT(!var->check(thd));
+ DBUG_ASSERT(!var->update(thd));
+ }

  - variable length array in set_stmt_get_reset_vars() is not portable.
    You actually don't even need another buffer for the var name, and
    another LEX_STRING variable. what's wrong with using a pointer to
    var->var->name directly?

  - switch() in set_stmt_get_reset_vars() handles only a subset of the
    options. What about SHOW_BOOL, SHOW_DOUBLE, SHOW_SIGNED_LONG, etc.?

  - please remove all changes related to ONE_SHOT

  - the following code should be wrapped into #ifndef DBUG_OFF:

+ if(thd->lex->stmt_set_list.is_empty())
+ DBUG_ASSERT(thd->free_list == NULL);

  - in general, I would implement it all differently. Just replace
    thd->variables with its deep copy before statement execution, then
    destroy it after statement execution and assign the original pointer
    to thd->variables. This way you don't have to mess with option
    types, Item instances, thd->free_list, etc. I guess the patch would
    be 50% smaller and more robust against future changes.

  - replication. No system variables are replicated except a small
    subset which is listed in the manual. But variables from that subset
    are written to the binlog event header. Which means a slave should
    ignore _all_ statement variables and we don't need
    filter_replicated_variables().

> - the following assertion
>
> + if (lex->sql_command == SQLCOM_EXECUTE || lex->sql_command ==
> SQLCOM_PREPARE)
> + {
> + DBUG_ASSERT(thd->free_list == NULL);
> + thd->free_list= stmt_free_list;
> + }
>
> would fail in the following case:
>
> SET global_only_var=..., normal_var=... FOR EXECUTE stmt;
>
> because if an error occurs in one of the set_stmt_get_reset_vars,
> we bail out before saving and resetting thd->free_list.
>
> - the following code will not be resetting variables to their previous
> values in release builds (i.e. var->update() will not be called at
> all):
>
> + while ((var = it++))
> + {
> + DBUG_ASSERT(!var->check(thd));
> + DBUG_ASSERT(!var->update(thd));
> + }
>
> - variable length array in set_stmt_get_reset_vars() is not portable.
> You actually don't even need another buffer for the var name, and
> another LEX_STRING variable. what's wrong with using a pointer to
> var->var->name directly?
>
> - switch() in set_stmt_get_reset_vars() handles only a subset of the
> options. What about SHOW_BOOL, SHOW_DOUBLE, SHOW_SIGNED_LONG, etc.?
>
The above comments do not make sense because the feature is implemented in another way now.

> - please remove all changes related to ONE_SHOT
>
Done.

> - the following code should be wrapped into #ifndef DBUG_OFF:
>
> + if(thd->lex->stmt_set_list.is_empty())
> + DBUG_ASSERT(thd->free_list == NULL);
>
Done.

> - in general, I would implement it all differently. Just replace
> thd->variables with its deep copy before statement execution, then
> destroy it after statement execution and assign the original pointer
> to thd->variables. This way you don't have to mess with option
> types, Item instances, thd->free_list, etc. I guess the patch would
> be 50% smaller and more robust against future changes.
Done.

> - replication. No system variables are replicated except a small
> subset which is listed in the manual. But variables from that subset
> are written to the binlog event header. Which means a slave should
> ignore _all_ statement variables and we don't need
> filter_replicated_variables().
Done.

http://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/268/

Debug builds fail.

> Debug builds fail.

Fixed.
http://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/271

  - the comment for LEX::set_statement says:
    “The number of recursive SET STATEMENT ... FOR ... statements“

    but the variable has the ‘bool’ type?

  - but it got me wondering what happens with recursive SET STATEMENT
    statements, and indeed they don’t work as one would expect,
    i.e. only the innermost SET STATEMENT has an effect, and the outer
    ones seem to be ignored. This doesn’t look like a serious limitation
    and I’m fine with documenting it, but are there are real reasons for
    this behavior? It looks like making in work correctly is a matter of
    not calling lex->var_list.empty() if it already has some variables?

  - the patch introduces a new Bison warning:

“...sql/sql_yacc.yy:14551.11-14567.76: warning: type clash on default
action: <NONE> != <>”

  because it doesn’t define an action after “set_stmt_option_value_following_option_type_list FOR_SYM statement“

  - do you really need set_var::stmt_update()? It is used in only one
    place in the code. It returns a value, but the only place where it is
    called doesn’t care about its return value. What gives?

  - the following change leads to server allocating 816 bytes on stack
    for all queries, even those not using per-query variables.

---
@@ -2419,6 +2419,8 @@ mysql_execute_command(THD *thd)
   /* have table map for update for multi-update statement (BUG#37051) */
   bool have_table_map_for_update= FALSE;
 #endif
+ struct system_variables per_query_variables_backup;
+
   DBUG_ENTER("mysql_execute_command");
   DBUG_ASSERT(!lex->describe || is_explainable_query(lex->sql_command));
---

   let’s allocate it on heap instead?

  - I guess changes in sql_prepare.cc are no longer required and can be
    reverted?

  - please rename rpl_* files to percona_rpl_*

On 09/22/2013 05:06 PM, Alexey Kopytov wrote:
> Review: Needs Fixing
>
> - the comment for LEX::set_statement says:
> “The number of recursive SET STATEMENT ... FOR ... statements“
>
> but the variable has the ‘bool’ type?
Yes, I forgot to change this comment, initially this variable was ulint.
But bool is enough even for recursive use. Fixed.

> - but it got me wondering what happens with recursive SET STATEMENT
> statements, and indeed they don’t work as one would expect,
> i.e. only the innermost SET STATEMENT has an effect, and the outer
> ones seem to be ignored. This doesn’t look like a serious limitation
> and I’m fine with documenting it, but are there are real reasons for
> this behavior? It looks like making in work correctly is a matter of
> not calling lex->var_list.empty() if it already has some variables?
Fixed, new test case #21 in mysql-test/t/percona_statement_set.test is
added.

But this implementation has disadvantage. In the case of "SET STATEMENT
... FOR SET SESSION ..." query lex->var_list will be cleared when SET
SESSION is parsed, so all variables settings in SET STATEMENT section
will be cleared. The solution could be in using separate list for
statement variables. But in this case all variables which was set in
"SET SESSION" part will be reset to pre-statement state as
thd->variables will be completely restored after statement execution.

> - the patch introduces a new Bison warning:
>
> “...sql/sql_yacc.yy:14551.11-14567.76: warning: type clash on default
> action: <NONE> != <>”
>
> because it doesn’t define an action after “set_stmt_option_value_following_option_type_list FOR_SYM statement“
Fixed.

> - do you really need set_var::stmt_update()? It is used in only one
> place in the code. It returns a value, but the only place where it is
> called doesn’t care about its return value. What gives?
Return type is changed to "void". But this function is still necessary
because sys_var::on_update is protected.

> - the following change leads to server allocating 816 bytes on stack
> for all queries, even those not using per-query variables.
>
> ---
> @@ -2419,6 +2419,8 @@ mysql_execute_command(THD *thd)
> /* have table map for update for multi-update statement (BUG#37051) */
> bool have_table_map_for_update= FALSE;
> #endif
> + struct system_variables per_query_variables_backup;
> +
> DBUG_ENTER("mysql_execute_command");
> DBUG_ASSERT(!lex->describe || is_explainable_query(lex->sql_command));
> ---
>
> let’s allocate it on heap instead?
Done.

> - I guess changes in sql_prepare.cc are no longer required and can be
> reverted?
No, they are still required because objects of "Item" class are
allocated during parsing and thd->free_list contains list of those objects.

> - please rename rpl_* files to percona_rpl_*
Done.

Here is new testing
http://jenkins.percona.com/view/PS%205.6/job/percona-server-5.6-param/291 .

Hi Vlad,

On Mon, 23 Sep 2013 10:27:28 +0400, Vlad Lesin wrote:

>> - I guess changes in sql_prepare.cc are no longer required and can be
>> reverted?
> No, they are still required because objects of "Item" class are
> allocated during parsing and thd->free_list contains list of those objects.
>

OK, I see it now. Shouldn't we instead of adjusting the assertion in
sql_prepare.cc just reset thd->free_list after calling
sql_set_variables() in mysql_execute_command()? It was impossible with
the previous implementation, but is possible now, right?

Vlad,

Conflicts, please rebase on trunk.

> Conflicts, please rebase on trunk.

Rebased.

> Hi Vlad,
>
> On Mon, 23 Sep 2013 10:27:28 +0400, Vlad Lesin wrote:
>
> >> - I guess changes in sql_prepare.cc are no longer required and can be
> >> reverted?
> > No, they are still required because objects of "Item" class are
> > allocated during parsing and thd->free_list contains list of those objects.
> >
>
> OK, I see it now. Shouldn't we instead of adjusting the assertion in
> sql_prepare.cc just reset thd->free_list after calling
> sql_set_variables() in mysql_execute_command()? It was impossible with
> the previous implementation, but is possible now, right?

Resetting thd->free_list after each sql_set_variables() call does not work right because thd->free_list can contain "Item" class instances not only from SET STATEMENT parsing. If fixes in sql_prepare.cc are not desirable thd->free_list can be released in SQLCOM_EXECUTE command handler in mysql_execute_command() function. As it can be seen I have done it. But I have some doubts about this because there is no common solution and the issue which is in sql_prepare.cc is fixed in sql_parse.cc, but can be fixed in the code which is the source of the issue. I do not think such approach will add understanding in the code even if it is documented in comments because referring from one part of code to another can lead to the situation when some part of code is changed but depended part is not changed.

The following note was ignored but I consider it is very important and it should be discussed:
---
But this implementation has disadvantage. In the case of "SET STATEMENT
... FOR SET SESSION ..." query lex->var_list will be cleared when SET
SESSION is parsed, so all variables settings in SET STATEMENT section
will be cleared. The solution could be in using separate list for
statement variables. But in this case all variables which was set in
"SET SESSION" part will be reset to pre-statement state as
thd->variables will be completely restored after statement execution.
---

My suggestions:
1) Forbid using "SET" as a statement after "FOR" keyword. But the problem will not go because SET SESSION can be used in SP or prepared query.
2) Use separate list of parsed variables for SET and SET STATEMENT ... FOR ... statements
3) Check if SET STATEMENT ... FOR ... was parsed in SQLCOM_SET_OPTION handler in mysql_execute_command(). If it was then return error.

Here is new code testing results:
http://jenkins.percona.com/view/PS 5.6/job/percona-server-5.6-param/322

Hi Vlad,

On Fri, 27 Sep 2013 21:29:46 -0000, Vlad Lesin wrote:>> Hi Vlad,
>>
>> On Mon, 23 Sep 2013 10:27:28 +0400, Vlad Lesin wrote:
>>
>>>> - I guess changes in sql_prepare.cc are no longer required and can be
>>>> reverted?
>>> No, they are still required because objects of "Item" class are
>>> allocated during parsing and thd->free_list contains list of those objects.
>>>
>>
>> OK, I see it now. Shouldn't we instead of adjusting the assertion in
>> sql_prepare.cc just reset thd->free_list after calling
>> sql_set_variables() in mysql_execute_command()? It was impossible with
>> the previous implementation, but is possible now, right?
>
> Resetting thd->free_list after each sql_set_variables() call does not work right because thd->free_list can contain "Item" class instances not only from SET STATEMENT parsing. If fixes in sql_prepare.cc are not desirable thd->free_list can be released in SQLCOM_EXECUTE command handler in mysql_execute_command() function. As it can be seen I have done it. But I have some doubts about this because there is no common solution and the issue which is in sql_prepare.cc is fixed in sql_parse.cc, but can be fixed in the code which is the source of the issue. I do not think such approach will add understanding in the code even if it is documented in comments because referring from one part of code to another can lead to the situation when some part of code is changed but depended part is not changed.
>

Right, but I think the current way is more correct than adjusting the assertion in Prepared_statement::execute_loop(), because this way we keep the invariant that code relies on (no externally allocated objects when executing a prepared statement).

> The following note was ignored but I consider it is very important and it should be discussed:
> ---
> But this implementation has disadvantage. In the case of "SET STATEMENT
> ... FOR SET SESSION ..." query lex->var_list will be cleared when SET
> SESSION is parsed, so all variables settings in SET STATEMENT section
> will be cleared. The solution could be in using separate list for
> statement variables. But in this case all variables which was set in
> "SET SESSION" part will be reset to pre-statement state as
> thd->variables will be completely restored after statement execution.
> ---
>

I don't think SET STATEMENT ... FOR SET SESSSION ... is a "very important" case. I'm fine with documenting it as a known limitation and figuring out a way to fix it, if and when someone gets unhappy about it.

Approved.