Code review comment for lp:~vlad-lesin/percona-server/5.6-per_query_variables_settings

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 .