- How does it replicate? Would a test be needed for that? https://dev.mysql.com/doc/refman/5.6/en/replication-features-variables.html
- Please add tests for invalid syntax
- Please add tests for global-only variable use attempts
- Since it is supported only for variables with certain show type,
please test that variables with unsupported show types are
rejected correctly.
- set_stmt_reset_vars shouldn't assume that the update back to the
original value might possibly fail. I.e. it can fail, but that
should be a fatal assertion in a debug build. Thus instead of
if (!var->check(thd))
error|= var->update(thd);
I'd do something like
DBUG_ASSERT(var->check(thd));
error|= var->update(thd);
DBUG_ASSERT(!error);
- Please add CREATE TABLE STATEMENT(a INT) test to a testcase
- Lines 2063--2067: one_shot is removed as of 5.6.1.
- Lines 2093--2094: can this free list swap cause that something
non related to the per-statement vars might get freed late as
well? If that's not the case, then a
DBUG_ASSERT(!thd->free_list) before the set_stmt_get_reset_vars
loop call would document this. But if it is the case, then care
must be taken to remove only per-statement var Item instances
from the thd->free_list.
- Lines 2106--2112 and 2127--2135: please factor out to a common
function.
- Should lines 2240--2244 be sp_create_assignment_lex() call
instead?
- Spurious line 2260.
Minor comments:
- Please use the disable_warnings; DROP TABLE IF EXISTS;
enable_warnings; idiom in the testcase.
- The test depends on t1 being MyISAM. Please either spell it out
by ENGINE=MyISAM, either look into future-proofing the test by
using an InnoDB table instead (that would require adjusting Test
4 accordingly).
- Testcase comments: s/check/check that/g where appropriate.
- set_stmt_get_reset_vars header comment: s/will retrieve/retrieves
- body comment: s/ans/and
- Diff line 2022: s/Variables list/Variable list
- Diff line 2086: s/fot/for
- How does it replicate? Would a test be needed for that? /dev.mysql. com/doc/ refman/ 5.6/en/ replication- features- variables. html ASSERT( var->check( thd)); ASSERT( !error) ; ASSERT( !thd->free_ list) before the set_stmt_ get_reset_ vars assignment_ lex() call warnings; idiom in the testcase. get_reset_ vars header comment: s/will retrieve/retrieves
https:/
- Please add tests for invalid syntax
- Please add tests for global-only variable use attempts
- Since it is supported only for variables with certain show type,
please test that variables with unsupported show types are
rejected correctly.
- set_stmt_reset_vars shouldn't assume that the update back to the
original value might possibly fail. I.e. it can fail, but that
should be a fatal assertion in a debug build. Thus instead of
if (!var->check(thd))
error|= var->update(thd);
I'd do something like
DBUG_
error|= var->update(thd);
DBUG_
- Please add CREATE TABLE STATEMENT(a INT) test to a testcase
- Lines 2063--2067: one_shot is removed as of 5.6.1.
- Lines 2093--2094: can this free list swap cause that something
non related to the per-statement vars might get freed late as
well? If that's not the case, then a
DBUG_
loop call would document this. But if it is the case, then care
must be taken to remove only per-statement var Item instances
from the thd->free_list.
- Lines 2106--2112 and 2127--2135: please factor out to a common
function.
- Should lines 2240--2244 be sp_create_
instead?
- Spurious line 2260.
Minor comments:
- Please use the disable_warnings; DROP TABLE IF EXISTS;
enable_
- The test depends on t1 being MyISAM. Please either spell it out
by ENGINE=MyISAM, either look into future-proofing the test by
using an InnoDB table instead (that would require adjusting Test
4 accordingly).
- Testcase comments: s/check/check that/g where appropriate.
- set_stmt_
- body comment: s/ans/and
- Diff line 2022: s/Variables list/Variable list
- Diff line 2086: s/fot/for