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?
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 enable_ warnings at the start.
EXISTS-
Some parts of the test manipulates various MyISAM variables such as sort_buffer_ size, myisam_ repair_ threads. But table t1 is an
myisam_
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?