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

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