maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #08101
Re: e91559c: MDEV-7006 MDEV-7007: SET STATEMENT and slow log
On 29.01.15 17:41, Sergei Golubchik wrote:
Hi, Oleksandr!
On Jan 28, Oleksandr Byelkin wrote:
revision-id: e91559cf7f2b19378e9e5d3f72c3a6794a09d1e0
parent(s): cbc318fcf3680c01dca1320b87f625124a497528
committer: Oleksandr Byelkin
branch nick: work-maria-10.1-MDEV-6997
timestamp: 2014-11-26 11:54:29 +0100
message:
MDEV-7006 MDEV-7007: SET STATEMENT and slow log
fixed embedded server tests
old values of SET STATENENT variables now saved in its own Query_arena and restored later
Basically, I have two comments:
1. I think it might be kind of expensive to create MEM_ROOT for every
SET STATEMENT. And perhaps a user will never need to set-statement
these log variables.
I don't see a good solution for that. Either we can drop the support
for log related variables in SET STATEMENT, basically not using your
patch. I wouldn't like that, from a user point of view this is a
pretty artificial limitation, he doesn't care about memroots. Or we
can only create memroot for log related variables and put other
variables in the statement memroot, which also means restoring values
in two steps. Seems like too complex for a solution.
A compromise solution would be to do almost like in your patch, but
not to free memroot after every statement. That is, a memroot is
created once, for the first SET STATEMENT. At the end of the
statement you only MY_MARK_BLOCKS_FREE or MY_KEEP_PREALLOC. Then the
next SET STATEMENT wouldn't need to allocate that mucht.
Not perfect, but might do as a tradeoff.
Or, perhaps you can suggest a better solution.
I'll think but can't came up with something better at the moment
2. I didn't quite like that you've added restore_set_statement_var,
reset_arena_for_set_stmt, free_arena_for_set_stmt, etc everywhere.
I'd prefer you to add them to one function which is called from all
these places. Like, there must be some cleanup-after-statement
function that you should be able to use.
OK
diff --git a/mysql-test/t/set_statement.test b/mysql-test/t/set_statement.test
index a658071..108c1a3 100644
--- a/mysql-test/t/set_statement.test
+++ b/mysql-test/t/set_statement.test
@@ -995,6 +985,57 @@ set global general_log=0;
set statement lock_wait_timeout=1 for select @@lock_wait_timeout;
set global general_log=@save_general_log;
+--echo # MDEV-7006 MDEV-7007: SET statement and slow log
+
+set @save_long_query_time= @@long_query_time;
+set @save_slow_query_log= @@slow_query_log;
+set @save_log_output= @@log_output;
+
+set statement long_query_time=default for select @@long_query_time;
+set statement log_slow_filter=default for select @@log_slow_filter;
+set statement log_slow_verbosity=default for select @@log_slow_verbosity;
+set statement log_slow_rate_limit=default for select @@log_slow_rate_limit;
+set statement slow_query_log=default for select @@slow_query_log;
+
+truncate table mysql.slow_log;
+set slow_query_log= 1;
+set global log_output='TABLE';
+
+select sql_text from mysql.slow_log;
+set @@long_query_time=0.01;
+--echo #should be written
+select sleep(0.1);
+set @@long_query_time=@save_long_query_time;
+select sql_text from mysql.slow_log;
+--echo #---
+--echo #should be written
+set statement long_query_time=0.01 for select sleep(0.1);
+select sql_text from mysql.slow_log;
Okay, I hope it'll work, but please do try it in a separate branch in
buildbot. all these timing-related tests have proven to be very fragile,
so I've learned to be very cautious about these stuff. even in test
that looks safe to me :)
I'll do
+--echo #---
+set @@long_query_time=0.01;
+--echo #should NOT be written
+set statement slow_query_log=0 for select sleep(0.1);
+set @@long_query_time=@save_long_query_time;
+select sql_text from mysql.slow_log;
+--echo #---
+--echo #should be NOT written
+set statement long_query_time=0.01,log_slow_filter='full_scan' for select sleep(0.1);
+select sql_text from mysql.slow_log;
+--echo #---
+--echo #should be NOT written
+set statement long_query_time=0.01,log_slow_rate_limit=9999 for select sleep(0.1);
+select sql_text from mysql.slow_log;
+--echo #---
+#
+# log_slow_verbosity is impossible to check because results are not written
+# in TABLE mode
+#
+
+set global log_output= @save_log_output;
+set @@slow_query_log= @save_slow_query_log;
+set @@long_query_time= @save_long_query_time;
+
+
#
# Prohibited Variables
#
diff --git a/sql/sql_lex.cc b/sql/sql_lex.cc
index 6432e19..08094ce 100644
--- a/sql/sql_lex.cc
+++ b/sql/sql_lex.cc
@@ -4235,6 +4236,53 @@ 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);
+ MEM_ROOT *mem_root= new MEM_ROOT();
+ if (!(mem_root))
+ DBUG_VOID_RETURN;
+ init_sql_alloc(mem_root, ALLOC_ROOT_MIN_BLOCK_SIZE, 0,
+ MYF(MY_THREAD_SPECIFIC));
+ if (!(arena_for_set_stmt= new Query_arena(mem_root,
+ Query_arena::STMT_INITIALIZED)))
+ DBUG_VOID_RETURN;
1. allocate a new memroot for every SET STATEMENT?
discussed above.
2. no error reporting?
AFAIK EOM reported automagically we only need abort more or less clean
+ DBUG_PRINT("info", ("mem_root: 0x%lx arena: 0x%lx",
+ (ulong) mem_root, (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();
+ free_root(arena_for_set_stmt->mem_root, MYF(0));
+ delete(arena_for_set_stmt->mem_root);
+ delete(arena_for_set_stmt);
Eh... The arena is allocated in the memroot. I don't think you
can free the memroot before the arena. Probably something like this:
MEM_ROOT *memroot= arena_for_set_stmt->mem_root;
delete arena_for_set_stmt;
free_root(memroot, MYF(0));
delete memroot;
you are right
+ arena_for_set_stmt= 0;
+ DBUG_VOID_RETURN;
+}
+
void LEX::restore_set_statement_var()
{
DBUG_ENTER("LEX::restore_set_statement_var");
@@ -4243,7 +4291,9 @@ void LEX::restore_set_statement_var()
DBUG_PRINT("info", ("vars: %d", old_var_list.elements));
sql_set_variables(thd, &old_var_list, false);
old_var_list.empty();
+ free_arena_for_set_stmt();
}
+ DBUG_ASSERT(!is_arena_for_set_stmt());
the indentation is broken like this in many parts of the patch.
May be that's because you've forwarded it with your MUA...
I'll check
DBUG_VOID_RETURN;
}
diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
index 5af700b..ae831d0 100644
--- a/sql/sql_parse.cc
+++ b/sql/sql_parse.cc
@@ -1956,6 +1958,7 @@ bool dispatch_command(enum enum_server_command command, THD *thd,
/* Check that some variables are reset properly */
DBUG_ASSERT(thd->abort_on_warning == 0);
+ thd->lex->restore_set_statement_var();
too bad you had to put these restore_set_statement_var,
reset_arena_for_set_stmt, free_arena_for_set_stmt, etc
everywhere explicitly.
OK
DBUG_RETURN(error);
}
Regards,
Sergei
References
