Re: [Maria-developers] [Commits] 83659a6: MDEV-7006 MDEV-7007: SET STATEMENT and slow log
Hi, Sanja! On Feb 17, sanja@mariadb.com wrote:
revision-id: 83659a68642da2eb6e9327f17a0ef730f512aa96 parent(s): 3e2849d2a01b06a61407b00989c3f981e62dd183 committer: Oleksandr Byelkin branch nick: server timestamp: 2015-02-17 12:54:51 +0100 message:
MDEV-7006 MDEV-7007: SET STATEMENT and slow log fixed embedded server tests MDEV-7009: SET STATEMENT min_examined_row_limit has no effect MDEV-6948:SET STATEMENT gtid_domain_id = ... FOR has no effect (same for gtid_seq_no and server_id)
old values of SET STATENENT variables now saved in its own Query_arena and restored later
Looks good to me. I didn't check, though, whether you've put every restore correctly. See minor comments below:
diff --git a/mysql-test/t/set_statement_notembedded_binlog.test b/mysql-test/t/set_statement_notembedded_binlog.test new file mode 100644 index 0000000..c9c2334 --- /dev/null +++ b/mysql-test/t/set_statement_notembedded_binlog.test @@ -0,0 +1,22 @@ +--source include/have_log_bin.inc +--source include/not_embedded.inc + +--disable_warnings +drop table if exists t1; +drop view if exists t1; +--enable_warnings
This was useful in our early MySQL days, but it's not anymore. I've stopped adding these drop...if exists at the beginning of test files a couple of years ago. mtr has a after-test check to ensure that no test leaves its tables behind. So one can rely on every test starting clean.
+ +--echo # +--echo # MDEV-6948: SET STATEMENT gtid_domain_id = ... FOR has no effect +--echo # (same for gtid_seq_no and server_id) +--echo # +reset master; +create table t1 (i int); +set gtid_domain_id = 10; +insert into t1 values (1),(2); +set statement gtid_domain_id = 20 for insert into t1 values (3),(4); + +--replace_column 1 x 2 x 3 x 4 x 5 x +show binlog events limit 5,5; + +drop table t1; diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 0c1c3b3..ee956ed 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -4223,6 +4224,56 @@ int LEX::print_explain(select_result_sink *output, uint8 explain_flags, return res; }
+void LEX::set_arena_for_set_stmt(Query_arena *backup) +{ + DBUG_ENTER("LEX::set_arena_for_set_stmt"); + DBUG_ASSERT(arena_for_set_stmt== 0); + if (!mem_root_for_set_stmt) + { + mem_root_for_set_stmt= new MEM_ROOT(); + if (!(mem_root_for_set_stmt)) + DBUG_VOID_RETURN;
What happens in OOM case? You return early, but does the caller know that? Or it'll continue, thinking that everything's ok?
+ init_sql_alloc(mem_root_for_set_stmt, ALLOC_ROOT_SET, ALLOC_ROOT_SET, + MYF(MY_THREAD_SPECIFIC)); + } + if (!(arena_for_set_stmt= new Query_arena(mem_root_for_set_stmt,
Why are you allocating memory for Query_arena, instead of creating it on mem_root_for_set_stmt?
+ Query_arena::STMT_INITIALIZED))) + DBUG_VOID_RETURN; + DBUG_PRINT("info", ("mem_root: 0x%lx arena: 0x%lx", + (ulong) mem_root_for_set_stmt, + (ulong) arena_for_set_stmt)); + thd->set_n_backup_active_arena(arena_for_set_stmt, backup); + DBUG_VOID_RETURN; +} + + +void LEX::reset_arena_for_set_stmt(Query_arena *backup) +{ + DBUG_ENTER("LEX::reset_arena_for_set_stmt"); + DBUG_ASSERT(arena_for_set_stmt); + thd->restore_active_arena(arena_for_set_stmt, backup); + DBUG_PRINT("info", ("mem_root: 0x%lx arena: 0x%lx", + (ulong) arena_for_set_stmt->mem_root, + (ulong) arena_for_set_stmt)); + DBUG_VOID_RETURN; +} + + +void LEX::free_arena_for_set_stmt() +{ + DBUG_ENTER("LEX::free_arena_for_set_stmt"); + if (!arena_for_set_stmt) + return; + DBUG_PRINT("info", ("mem_root: 0x%lx arena: 0x%lx", + (ulong) arena_for_set_stmt->mem_root, + (ulong) arena_for_set_stmt)); + arena_for_set_stmt->free_items(); + delete(arena_for_set_stmt); + free_root(mem_root_for_set_stmt, MYF(MY_KEEP_PREALLOC)); + arena_for_set_stmt= 0; + DBUG_VOID_RETURN; +} + void LEX::restore_set_statement_var() { DBUG_ENTER("LEX::restore_set_statement_var"); diff --git a/sql/sql_lex.h b/sql/sql_lex.h index 46d667c..ed3c4a0 100644 --- a/sql/sql_lex.h +++ b/sql/sql_lex.h @@ -2677,10 +2688,22 @@ struct LEX: public Query_tables_list limit_rows_examined_cnt= ULONGLONG_MAX; }
+ + inline void free_set_stmt_mem_root() + {
Please, add DBUG_ASSERT(!is_arena_for_set_stmt());
+ if (mem_root_for_set_stmt) + { + free_root(mem_root_for_set_stmt, MYF(0)); + delete mem_root_for_set_stmt; + mem_root_for_set_stmt= 0; + } + } + LEX();
virtual ~LEX() { + free_set_stmt_mem_root(); destroy_query_tables_list(); plugin_unlock_list(NULL, (plugin_ref *)plugins.buffer, plugins.elements); delete_dynamic(&plugins);
Regards, Sergei
On 25.02.15 23:15, Sergei Golubchik wrote:
Hi, Sanja!
On Feb 17, sanja@mariadb.com wrote:
revision-id: 83659a68642da2eb6e9327f17a0ef730f512aa96 parent(s): 3e2849d2a01b06a61407b00989c3f981e62dd183 committer: Oleksandr Byelkin branch nick: server timestamp: 2015-02-17 12:54:51 +0100 message:
MDEV-7006 MDEV-7007: SET STATEMENT and slow log fixed embedded server tests MDEV-7009: SET STATEMENT min_examined_row_limit has no effect MDEV-6948:SET STATEMENT gtid_domain_id = ... FOR has no effect (same for gtid_seq_no and server_id)
old values of SET STATENENT variables now saved in its own Query_arena and restored later Looks good to me. I didn't check, though, whether you've put every restore correctly.
See minor comments below:
diff --git a/mysql-test/t/set_statement_notembedded_binlog.test b/mysql-test/t/set_statement_notembedded_binlog.test new file mode 100644 index 0000000..c9c2334 --- /dev/null +++ b/mysql-test/t/set_statement_notembedded_binlog.test @@ -0,0 +1,22 @@ +--source include/have_log_bin.inc +--source include/not_embedded.inc + +--disable_warnings +drop table if exists t1; +drop view if exists t1; +--enable_warnings This was useful in our early MySQL days, but it's not anymore. I've stopped adding these drop...if exists at the beginning of test files a couple of years ago. mtr has a after-test check to ensure that no test leaves its tables behind. So one can rely on every test starting clean. OK + +--echo # +--echo # MDEV-6948: SET STATEMENT gtid_domain_id = ... FOR has no effect +--echo # (same for gtid_seq_no and server_id) +--echo # +reset master; +create table t1 (i int); +set gtid_domain_id = 10; +insert into t1 values (1),(2); +set statement gtid_domain_id = 20 for insert into t1 values (3),(4); + +--replace_column 1 x 2 x 3 x 4 x 5 x +show binlog events limit 5,5; + +drop table t1; diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc index 0c1c3b3..ee956ed 100644 --- a/sql/sql_lex.cc +++ b/sql/sql_lex.cc @@ -4223,6 +4224,56 @@ int LEX::print_explain(select_result_sink *output, uint8 explain_flags, return res; }
+void LEX::set_arena_for_set_stmt(Query_arena *backup) +{ + DBUG_ENTER("LEX::set_arena_for_set_stmt"); + DBUG_ASSERT(arena_for_set_stmt== 0); + if (!mem_root_for_set_stmt) + { + mem_root_for_set_stmt= new MEM_ROOT(); + if (!(mem_root_for_set_stmt)) + DBUG_VOID_RETURN; What happens in OOM case? You return early, but does the caller know that? Or it'll continue, thinking that everything's ok?
Yes, it will continue till first thd->error then abort. but I put aditional check to speedup the process.
+ init_sql_alloc(mem_root_for_set_stmt, ALLOC_ROOT_SET, ALLOC_ROOT_SET, + MYF(MY_THREAD_SPECIFIC)); + } + if (!(arena_for_set_stmt= new Query_arena(mem_root_for_set_stmt, Why are you allocating memory for Query_arena, instead of creating it on mem_root_for_set_stmt?
Arena is needed because we create Items and so they should go to the item free list of the arena.
+ Query_arena::STMT_INITIALIZED))) + DBUG_VOID_RETURN; + DBUG_PRINT("info", ("mem_root: 0x%lx arena: 0x%lx", + (ulong) mem_root_for_set_stmt, + (ulong) arena_for_set_stmt)); + thd->set_n_backup_active_arena(arena_for_set_stmt, backup); + DBUG_VOID_RETURN; +} + + +void LEX::reset_arena_for_set_stmt(Query_arena *backup) +{ + DBUG_ENTER("LEX::reset_arena_for_set_stmt"); + DBUG_ASSERT(arena_for_set_stmt); + thd->restore_active_arena(arena_for_set_stmt, backup); + DBUG_PRINT("info", ("mem_root: 0x%lx arena: 0x%lx", + (ulong) arena_for_set_stmt->mem_root, + (ulong) arena_for_set_stmt)); + DBUG_VOID_RETURN; +} + + +void LEX::free_arena_for_set_stmt() +{ + DBUG_ENTER("LEX::free_arena_for_set_stmt"); + if (!arena_for_set_stmt) + return; + DBUG_PRINT("info", ("mem_root: 0x%lx arena: 0x%lx", + (ulong) arena_for_set_stmt->mem_root, + (ulong) arena_for_set_stmt)); + arena_for_set_stmt->free_items(); + delete(arena_for_set_stmt); + free_root(mem_root_for_set_stmt, MYF(MY_KEEP_PREALLOC)); + arena_for_set_stmt= 0; + DBUG_VOID_RETURN; +} + void LEX::restore_set_statement_var() { DBUG_ENTER("LEX::restore_set_statement_var"); diff --git a/sql/sql_lex.h b/sql/sql_lex.h index 46d667c..ed3c4a0 100644 --- a/sql/sql_lex.h +++ b/sql/sql_lex.h @@ -2677,10 +2688,22 @@ struct LEX: public Query_tables_list limit_rows_examined_cnt= ULONGLONG_MAX; }
+ + inline void free_set_stmt_mem_root() + { Please, add
DBUG_ASSERT(!is_arena_for_set_stmt());
OK
+ if (mem_root_for_set_stmt) + { + free_root(mem_root_for_set_stmt, MYF(0)); + delete mem_root_for_set_stmt; + mem_root_for_set_stmt= 0; + } + } + LEX();
virtual ~LEX() { + free_set_stmt_mem_root(); destroy_query_tables_list(); plugin_unlock_list(NULL, (plugin_ref *)plugins.buffer, plugins.elements); delete_dynamic(&plugins); Regards, Sergei
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik