Hi, Sanja! On May 22, sanja@askmonty.org wrote:
=== added file 'mysql-test/suite/percona/percona_rpl_stm_per_query_variables_settings.test' --- a/mysql-test/suite/percona/percona_rpl_stm_per_query_variables_settings.test 1970-01-01 00:00:00 +0000 +++ b/mysql-test/suite/percona/percona_rpl_stm_per_query_variables_settings.test 2014-05-22 12:50:01 +0000
could you rename tests to have shorter names? rpl_set_statement looks good enough to me
@@ -0,0 +1,57 @@ +--source include/master-slave.inc +--source include/have_binlog_format_statement.inc + +--disable_warnings +DROP TABLE IF EXISTS t1; +DROP TABLE IF EXISTS t2; +--enable_warnings + +call mtr.add_suppression("Unsafe statement written to the binary log*"); +CREATE TABLE t1 (a bigint unsigned not null); +CREATE TABLE t2 (a char(255) not null);
okay, although I wouldn't bother creating *two* tables, I'd create one and hard-coded it into percona_rpl_set_statement_variable_test.inc (removing rpl_ssvt_table parameter)
+ +--echo +--echo There are the following types of variables: +--echo 1) variables that are NOT replicated correctly when using STATEMENT mode; +--echo + +--let $rpl_ssvt_var_name=max_join_size +--let $rpl_ssvt_var_value=2 +--let $rpl_ssvt_table=t1 +--source suite/percona/percona_rpl_set_statement_variable_test.inc + +--echo +--echo 2) variables thar ARE replicated correctly +--echo They must be replicated correctly with "SET STATEMENT" too. +--echo +--let $rpl_ssvt_var_name=auto_increment_increment +--let $rpl_ssvt_var_value=10 +--let $rpl_ssvt_table=t1 +--source suite/percona/percona_rpl_set_statement_variable_test.inc + +--echo +--echo 3) sql_mode which is replicated correctly exept NO_DIR_IN_CREATE value; +--echo +--let $rpl_ssvt_var_name=sql_mode +--let $rpl_ssvt_var_value='ERROR_FOR_DIVISION_BY_ZERO' +--let $rpl_ssvt_table=t2 +--source suite/percona/percona_rpl_set_statement_variable_test.inc +--let $rpl_ssvt_var_name=sql_mode +--let $rpl_ssvt_var_value='NO_DIR_IN_CREATE' +--let $rpl_ssvt_table=t2 +--source suite/percona/percona_rpl_set_statement_variable_test.inc + +--echo +--echo 4) variables that are not replicated at all: +--echo default_storage_engine, storage_engine, max_heap_table_size +--echo +--let $rpl_ssvt_var_name=max_heap_table_size +--let $rpl_ssvt_var_value=16384 +--let $rpl_ssvt_table=t1 +--source suite/percona/percona_rpl_set_statement_variable_test.inc + +connection master; +DROP TABLE t1; +DROP TABLE t2; +sync_slave_with_master; +source include/stop_slave.inc;
=== added file 'mysql-test/suite/percona/percona_statement_set.result' --- a/mysql-test/suite/percona/percona_statement_set.result 1970-01-01 00:00:00 +0000 +++ b/mysql-test/suite/percona/percona_statement_set.result 2014-05-22 12:50:01 +0000
reverse polish notation in percona someone loves. starwars too much seen he apparently. Please, rename to "set_statement.result"
@@ -0,0 +1,875 @@ +'# SET STATEMENT ..... FOR .... TEST' +DROP TABLE IF EXISTS t1; +DROP FUNCTION IF EXISTS myProc; +DROP PROCEDURE IF EXISTS p1; +DROP PROCEDURE IF EXISTS p2; +DROP PROCEDURE IF EXISTS p3; +DROP PROCEDURE IF EXISTS p4; +DROP PROCEDURE IF EXISTS p5; +DROP TABLE IF EXISTS STATEMENT; +'# Setup database' +CREATE TABLE t1 (v1 INT, v2 INT); +INSERT INTO t1 VALUES (1,2); +INSERT INTO t1 VALUES (3,4); +'' +'#------------------ STATEMENT Test 1 -----------------------#' +'# Initialize variables to known setting' +SET SESSION sort_buffer_size=100000; +'' +'# Pre-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'sort_buffer_size'; +Variable_name Value +sort_buffer_size 100000 +SET STATEMENT sort_buffer_size=150000 FOR SELECT * FROM t1; +v1 v2 +1 2 +3 4 +'' +'# Post-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'sort_buffer_size'; +Variable_name Value +sort_buffer_size 100000 +'' +'#------------------ STATEMENT Test 2 -----------------------#' +'# Initialize variables to known setting' +SET SESSION binlog_format=mixed; +SET SESSION sort_buffer_size=100000; +'# Pre-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'sort_buffer_size'; +Variable_name Value +sort_buffer_size 100000 +SHOW SESSION VARIABLES LIKE 'binlog_format'; +Variable_name Value +binlog_format MIXED +SET STATEMENT sort_buffer_size=150000, binlog_format=row +FOR SELECT * FROM t1; +v1 v2 +1 2 +3 4 +'# Post-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'sort_buffer_size'; +Variable_name Value +sort_buffer_size 100000 +SHOW SESSION VARIABLES LIKE 'binlog_format'; +Variable_name Value +binlog_format MIXED +'' +'#------------------ STATEMENT Test 3 -----------------------#' +'# set initial variable value, make prepared statement +SET SESSION binlog_format=row; +PREPARE stmt1 FROM 'SET STATEMENT binlog_format=row FOR SELECT * FROM t1'; +'' +'# Change variable setting' +SET SESSION binlog_format=mixed; +'' +'# Pre-STATEMENT variable value' +'' +SHOW SESSION VARIABLES LIKE 'binlog_format'; +Variable_name Value +binlog_format MIXED +'' +EXECUTE stmt1; +v1 v2 +1 2 +3 4 +'' +'# Post-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'binlog_format'; +Variable_name Value +binlog_format MIXED +'' +DEALLOCATE PREPARE stmt1; +'#------------------ STATEMENT Test 4 -----------------------#' +'# set initial variable value, make prepared statement +SET SESSION myisam_sort_buffer_size=500000, myisam_repair_threads=1; +'' +'# Pre-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'myisam_sort_buffer_size'; +Variable_name Value +myisam_sort_buffer_size 500000 +SHOW SESSION VARIABLES LIKE 'myisam_repair_threads'; +Variable_name Value +myisam_repair_threads 1 +'' +SET STATEMENT myisam_sort_buffer_size=800000, +myisam_repair_threads=2 FOR OPTIMIZE TABLE t1;
how comes that is "make prepared statement" (see comment above) ?
+Table Op Msg_type Msg_text +test.t1 optimize status OK +'' +'# Post-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'myisam_sort_buffer_size'; +Variable_name Value +myisam_sort_buffer_size 500000 +SHOW SESSION VARIABLES LIKE 'myisam_repair_threads'; +Variable_name Value +myisam_repair_threads 1 +'' ... === added file 'mysql-test/suite/percona/percona_statement_set.test' --- a/mysql-test/suite/percona/percona_statement_set.test 1970-01-01 00:00:00 +0000 +++ b/mysql-test/suite/percona/percona_statement_set.test 2014-05-22 12:50:01 +0000 @@ -0,0 +1,847 @@ +--echo '# SET STATEMENT ..... FOR .... TEST'
many of the tests below are pretty useless. they don't test that the value is actually set. See below:
+############################ STATEMENT_SET ############################# +# # +# Testing working functionality of SET STATEMENT # +# # +# # +# There is important documentation within # +# # +# # +# Author: Joe Lukas # +# Creation: # +# 2009-08-02 Implement this test as part of # +# WL#681 Per query variable settings # +# # +######################################################################## + +--disable_warnings +DROP TABLE IF EXISTS t1; +DROP FUNCTION IF EXISTS myProc; +DROP PROCEDURE IF EXISTS p1; +DROP PROCEDURE IF EXISTS p2; +DROP PROCEDURE IF EXISTS p3; +DROP PROCEDURE IF EXISTS p4; +DROP PROCEDURE IF EXISTS p5; +DROP TABLE IF EXISTS STATEMENT; +--enable_warnings +#################################################################### +#Set up current database +#################################################################### +--echo '# Setup database' +CREATE TABLE t1 (v1 INT, v2 INT); +INSERT INTO t1 VALUES (1,2); +INSERT INTO t1 VALUES (3,4); +--echo '' +--echo '#------------------ STATEMENT Test 1 -----------------------#' +#################################################################### +# Checks with variable value type ulong # +#################################################################### +--echo '# Initialize variables to known setting' +SET SESSION sort_buffer_size=100000; +--echo '' +--echo '# Pre-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'sort_buffer_size'; +SET STATEMENT sort_buffer_size=150000 FOR SELECT * FROM t1;
How do you know that SET STATEMENT did anything at all?
+--echo '' +--echo '# Post-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'sort_buffer_size'; +--echo '' +--echo '#------------------ STATEMENT Test 2 -----------------------#' +#################################################################### +# Checks for multiple set values inside STATEMENT ... FOR # +#################################################################### +--echo '# Initialize variables to known setting' +SET SESSION binlog_format=mixed; +SET SESSION sort_buffer_size=100000; +--echo '# Pre-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'sort_buffer_size'; +SHOW SESSION VARIABLES LIKE 'binlog_format'; +SET STATEMENT sort_buffer_size=150000, binlog_format=row + FOR SELECT * FROM t1; +--echo '# Post-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'sort_buffer_size'; +SHOW SESSION VARIABLES LIKE 'binlog_format'; + +--echo '' +--echo '#------------------ STATEMENT Test 3 -----------------------#' +#################################################################### +# Check current variable value is stored in using stored # +# statements. # +#################################################################### +--echo '# set initial variable value, make prepared statement +SET SESSION binlog_format=row; +PREPARE stmt1 FROM 'SET STATEMENT binlog_format=row FOR SELECT * FROM t1';
How will you know that SET STATEMENT did anything at all? (the same applies to most tests below, I'll stop repeating it)
+--echo '' +--echo '# Change variable setting' +SET SESSION binlog_format=mixed; +--echo '' +--echo '# Pre-STATEMENT variable value' +--echo '' +SHOW SESSION VARIABLES LIKE 'binlog_format'; +--echo '' +EXECUTE stmt1; +--echo '' +--echo '# Post-STATEMENT variable value' +SHOW SESSION VARIABLES LIKE 'binlog_format'; + +--echo '' +DEALLOCATE PREPARE stmt1; ... === modified file 'sql/share/errmsg-utf8.txt' --- a/sql/share/errmsg-utf8.txt 2014-02-11 13:21:48 +0000 +++ b/sql/share/errmsg-utf8.txt 2014-05-22 12:50:01 +0000 @@ -7067,3 +7067,5 @@ ER_IT_IS_A_VIEW 42S02 eng "'%-.192s' is a view" ER_SLAVE_SKIP_NOT_IN_GTID eng "When using GTID, @@sql_slave_skip_counter can not be used. Instead, setting @@gtid_slave_pos explicitly can be used to skip to after a given GTID position." +ER_SET_STATEMENT_TABLES_IS_NOT_SUPPORTED + eng "SET STATEMENT does not support using tables in the assignment variables expressions"
why not?
=== modified file 'sql/sql_parse.cc' --- a/sql/sql_parse.cc 2014-03-29 10:33:25 +0000 +++ b/sql/sql_parse.cc 2014-05-22 12:50:01 +0000 @@ -2430,6 +2432,33 @@ mysql_execute_command(THD *thd) thd->get_binlog_format(&orig_binlog_format, &orig_current_stmt_binlog_format);
+ if (lex->set_statement && !lex->var_list.is_empty())
Hmm? No separate list of variables, but a common lex->var_list? Then the following won't work: SET STATEMENT A=1 SET B=2 I remember in the WL and in GSoC we agreed that for SET STATEMENT A=2 SET A=2 (same variable in SET STATEMENT and SET), it's acceptable to restore the original value of A, basically ignoring SET. But I doubt that the same logic applies when SET STATEMENT and SET modify *different* variables.
+ { + MEM_ROOT *save_mem_root; + /* + We should be sure that variables backup and assignment made on + runtime memory + */ + save_mem_root= thd->mem_root; + thd->mem_root= thd->get_execution_mem_root();
Why?
+ per_query_variables_backup= copy_system_variables(&thd->variables); + if (!per_query_variables_backup || + (res= sql_set_variables(thd, &lex->var_list))) + { + thd->mem_root= save_mem_root; + /* + We encountered some sort of error, but no message was sent. + Send something semi-generic here since we don't know which + assignment in the list caused the error. + */ + if (!thd->is_error()) + my_error(ER_WRONG_ARGUMENTS, MYF(0), "SET"); + goto error; + } + thd->mem_root= save_mem_root; + } + /* Force statement logging for DDL commands to allow us to update privilege, system or statistic tables directly without the updates @@ -5144,6 +5179,30 @@ finish:
lex->unit.cleanup();
+ if (lex->set_statement && !lex->var_list.is_empty()) { + List_iterator_fast<set_var_base> it(thd->lex->var_list); + set_var *var; + + free_system_variables(&thd->variables); + thd->variables= *per_query_variables_backup; + /* per_query_variables_backup was in a mem_root */ + per_query_variables_backup= NULL; + /* + When variables are restored after "SET STATEMENT ... FOR ..." statement + execution an update callback must be invoked for the system variables + to save special logic if it is. set_var_base class does not contain + refference to variable as it is just an interface class. But only + system variables are allowed to be used in "SET STATEMENT ... FOR ..." + statement, so cast from set_var_base* to set_var* can be used here. + */ + while ((var=(set_var *)it++)) + { + var->var->stmt_update(thd); + }
I don't think it'll work. Compare set_var::update() and set_var::stmt_update(). The former calls ::session_update(), while the latter does not. Normally, session_update() simply updates the value, performs the assignment. And it is, indeed, not needed here. but in some custom classes session_update() is more complex, and it's not safe to omit it. Please 1) see all non-trivial implementations of sys_var::session_update() 2) write SET SESSION tests for these variables 3) if they'll all work - please explain why 4) if they won't - I'm afraid, this whole approach needs to be redone, then
+ + thd->lex->set_statement= false; + } + if (! thd->in_sub_stmt) { if (thd->killed != NOT_KILLED)
=== modified file 'sql/sql_plugin.cc' --- a/sql/sql_plugin.cc 2014-03-26 21:25:38 +0000 +++ b/sql/sql_plugin.cc 2014-05-22 12:50:01 +0000 @@ -4088,3 +4088,48 @@ static void restore_ptr_backup(uint n, s (backup++)->restore(); }
+ +/** + Create deep copy of system_variables instance. +*/ +struct system_variables * +copy_system_variables(const struct system_variables *src)
What is it doing in sql_plugin.cc?
+{ + struct system_variables *dst; + + DBUG_ASSERT(src); + + dst= (struct system_variables *) + sql_alloc(sizeof(struct system_variables)); + if (!dst) + return NULL; + *dst= *src; + + if (dst->dynamic_variables_ptr) + { + if (!(dst->dynamic_variables_ptr= + (char *)my_malloc(dst->dynamic_variables_size, MYF(MY_WME | MY_FAE)))) + return NULL; + memcpy(dst->dynamic_variables_ptr, + src->dynamic_variables_ptr, + src->dynamic_variables_size); + } + + mysql_mutex_lock(&LOCK_plugin); + dst->table_plugin= + intern_plugin_lock(NULL, src->table_plugin);
I'm surprised that plugin locking actually works in here, (I believe it does, otherwise you'd noticed it). What about SET STATEMENT default_engine=MEMORY ? What about SET STATEMENT default_engine=MyISAM (where MyISAM is already default)? please, try both in debug build.
+ mysql_mutex_unlock(&LOCK_plugin); + + return dst; +} + +void free_system_variables(struct system_variables *v) +{ + DBUG_ASSERT(v); + + mysql_mutex_lock(&LOCK_plugin); + intern_plugin_unlock(NULL, v->table_plugin); + mysql_mutex_unlock(&LOCK_plugin); + + my_free(v->dynamic_variables_ptr); +}
=== modified file 'sql/sql_yacc.yy' --- a/sql/sql_yacc.yy 2014-03-26 21:25:38 +0000 +++ b/sql/sql_yacc.yy 2014-05-22 12:50:01 +0000 @@ -813,8 +813,10 @@ static void sp_create_assignment_lex(THD lex->one_shot_set= 0; lex->autocommit= 0; /* get_ptr() is only correct with no lookahead. */ - DBUG_ASSERT(no_lookahead); + if (no_lookahead) lex->sphead->m_tmp_query= lip->get_ptr(); + else + lex->sphead->m_tmp_query= lip->get_tok_end();
Why?
/* Inherit from outer lex. */ lex->option_type= old_lex->option_type; } @@ -955,10 +957,10 @@ bool my_yyoverflow(short **a, YYSTYPE ** %parse-param { THD *thd } %lex-param { THD *thd } /* - Currently there are 163 shift/reduce conflicts. + Currently there are 164 shift/reduce conflicts. We should not introduce new conflicts any more. */ -%expect 163 +%expect 164
Why?
/* Comments for TOKENS. @@ -14427,8 +14431,44 @@ set: } start_option_value_list {} + | SET STATEMENT_SYM + { + LEX *lex= Lex; + mysql_init_select(lex); + lex->sql_command= SQLCOM_SET_OPTION; + /* Don't clear var_list in the case of recursive statement */ + if (!lex->set_statement) + lex->var_list.empty();
When var_list is not empty here (and set_statement not set)? In other words, when this statement will actually have any effect?
+ lex->one_shot_set= 0; + lex->autocommit= 0; + lex->set_statement= true; + sp_head *sp= lex->sphead; + if (sp && !sp->is_invoked()) + { + sp->m_param_begin= ((yychar == YYEMPTY) ? YYLIP->get_ptr() : YYLIP->get_tok_start()); + sp->m_param_end= ((yychar == YYEMPTY) ? YYLIP->get_ptr() : YYLIP->get_tok_end());
Why?
+ } + } + set_stmt_option_value_following_option_type_list
Eh? What does "set stmt option value following option type list" mean? Please, rename to set_stmt_option_value_list or even set_stmt_list
+ { + if (Lex->query_tables) + { + my_error(ER_SET_STATEMENT_TABLES_IS_NOT_SUPPORTED, MYF(0)); + MYSQL_YYABORT; + } + } + FOR_SYM statement + {} ;
+set_stmt_option_value_following_option_type_list: + /* + Only system variables can be used here. If this condition is changed + please check careful code under lex->option_type == OPT_STATEMENT + condition on wrong type casts. + */ + option_value_following_option_type + | set_stmt_option_value_following_option_type_list ',' option_value_following_option_type
// Start of option value list start_option_value_list: Regards, Sergei