maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sanja!

On Feb 17, sanja@xxxxxxxxxxx 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