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

Alexey Kopytov (akopytov) wrote :

  - 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().

review: Needs Fixing

« Back to merge proposal