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

Revision history for this message
Laurynas Biveinis (laurynas-biveinis) wrote :

This is not a full review unfortunately as I cannot review the code part without spending too much time right now.

   The commit message describes the feature but not the changes made
   to the server.

   Lines 834--837 probably apply to the test case that served as a
   starting point for the current one. I'd replace them with
   "testcase adopted from foo.test".

   The testcase is missing the --disable_warnings-DROP TABLE IF
   EXISTS-enable_warnings at the start.

   Some parts of the test manipulates various MyISAM variables such as
   myisam_sort_buffer_size, myisam_repair_threads. But table t1 is an
   InnoDB table on 5.5, thus while of course it's possible to adjust
   the MyISAM variable values, a better test would be to adjust
   something relevant.

   I'd prefer to see more feature tests:
   1) Set some session variable to some value that does not allow some
      statement to complete without an error or warning. Issue that
      statement, record the warning. Then set the variable for the
      statement to a value that allows the statement to complete, for
      example join_buffer_size.

   2) autocommit is an interesting variable. What happens if you have
      autocommit=0, start a transaction with a statement set with
      autocommit=1 there? Note that for this MP, if any dark corner
      cases are found, there is no need to work out any additional
      semantics beyond what would a regular session autocommit setting
      do in the same situation.

   3) Set auto_increment_* for statement and check the resulting id.

   4) storage_engine for CREATE TABLE statement, check the resulting
      table type.

   5) query_cache_type, that it does not crash.

   If you disagree that so much testing is required, then I don't have
   that strong feelings about 1), 3) and 4). But I do want to see 2)
   and 5) very much.

   The header comment for set_stmt_get_reset_vars should be not in
   "will do smth" style, but "do smth", thus "retrieves the current
   value of ... ".

   s/will be change to/will be changed to

   Comma after "occurs" in @return.

   char str[var->var->name.length]; is a VLA and thus not a valid C++
   unfortunately.

   Line 1658: s/ans/and

   Is it possible to avoid the new yacc shift/reduce conflict?

review: Needs Fixing

« Back to merge proposal