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

Vlad Lesin (vlad-lesin) wrote :

> - How does it replicate? Would a test be needed for that?

There are the following types of variables:
1) variables that are NOT replicated correctly when using STATEMENT mode;
2) variables thar ARE replicated correctly when using STATEMENT mode;
3) sql_mode which is replicated correctly exept NO_DIR_IN_CREATE value;
4) variables that are not replicated at all:
 default_storage_engine, storage_engine, max_heap_table_size;

But as we can set variables in a statements itself and that statements are written to binlog in STATEMENT mode and old values of variables are restored just after statement execution it works correctly.

That variables are saved in Query_log_event on master for each event and restored on slave in Query_log_event::do_apply_event(). "Per query variables settings" code saves old values of variables that are listed in "SET STATEMENT ... FOR ..." statement before query execution and restores them after that. This code works in mysql_execute_command(). There is the following call stack on slave: Query_log_event::do_apply_event()->mysql_parse()->mysql_execute_command(). As variables are restored from mysqlbinlog event by slave thread in Query_log_event::do_apply_event() before "per query variables settings" code saves their old values the old values are lost. But in master the variables are restored to the previous values just after query execution and the next mysqlbinlog event will set them to the previous values on slave. As "SET STATEMENT ... FOR ..." changes only session values I think such behaviour is acceptable.

The NO_DIR_IN_CREATE mode is reset just before event applying on slave in Query_log_event::do_apply_event(). But after that mysql_parse() is invoked and the new value of sql_mode is parsed from statement including NO_DIR_IN_CREATE. For this case we filter out sql_mode variable on slaves during query parsing.

The same is for 3.

> - 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.
But thd->one_shot and lex->one_shot are still in the code. And it's resonable to take them into account while they are still there.

> - 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.
There is no need to factor out it to function, I moved code blocks such a way that only one reset vars block is used.

> - Should lines 2240--2244 be sp_create_assignment_lex() call
> instead?
No, it should not. Because sp_create_assignment_lex() sets parsed statement position to NULL:

    It's a SET statement within SP. It will be either translated
    into one or more sp_instr_stmt instructions, or it will be
    sp_instr_set / sp_instr_set_trigger_field instructions.
    In any case, position of SP-variable can not be determined
    reliably. So, we set the start pointer of the current statement
    to NULL.

 but we don't need it for SET STATEMENT.

> - 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).
The test result does not depend on db-engine of t1.

> - Testcase comments: s/check/check that/g where appropriate.
I am not sure, but it seems done too.

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

« Back to merge proposal