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

Revision history for this message
Alexey Kopytov (akopytov) wrote :

  - 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_*

review: Needs Fixing

« Back to merge proposal