Re: [Maria-developers] e91559c: MDEV-7006 MDEV-7007: SET STATEMENT and slow log
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 much. Not perfect, but might do as a tradeoff. Or, perhaps you can suggest a better solution. 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.
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 :)
+--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? 2. no error reporting?
+ 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;
+ 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...
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.
DBUG_RETURN(error); }
Regards, Sergei
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
Hi, Sergei! On 29.01.15 17:41, Sergei Golubchik wrote: [skip]
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.
Could you explain what you mean above? First my thought was that I use this methods together in several places and I have to make function out of it, but then I found that they used alone in most places and there is no repeatable sequence. [skip]
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik