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

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

   - 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

review: Needs Fixing

« Back to merge proposal