maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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