29 Jan
2015
29 Jan
'15
10:55 p.m.
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