maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Oleksandr!

Looks mostly ok, see few comments below

On Aug 01, Oleksandr Byelkin wrote:
> revision-id: fc42991720838e165ad448b6707602cded92faa4 (mariadb-10.2.1-9-gfc42991)
> parent(s): 08683a726773f8cdf16a4a3dfb3920e5f7842481
> committer: Oleksandr Byelkin
> timestamp: 2016-08-01 19:25:45 +0200
> message:
> 
> MDEV-7901: re-implement analyze table for low impact
> 
> Table before collecting engine independent statistics now is reopened in read mode,
> InnoDB allow write operations in this case.
> 
> ---
>  mysql-test/r/stat_tables_innodb_debug.result |  27 +++
>  mysql-test/t/stat_tables_innodb_debug.test   |  37 ++++
>  sql/sql_admin.cc                             | 295 +++++++++++++++------------
>  storage/innobase/handler/ha_innodb.cc        |   1 +
>  storage/xtradb/handler/ha_innodb.cc          |   2 +-
>  5 files changed, 227 insertions(+), 135 deletions(-)
> 
> diff --git a/mysql-test/r/stat_tables_innodb_debug.result b/mysql-test/r/stat_tables_innodb_debug.result
> new file mode 100644
> index 0000000..b02848d
> --- /dev/null
> +++ b/mysql-test/r/stat_tables_innodb_debug.result
> @@ -0,0 +1,27 @@
> +# 
> +# MDEV-7901: re-implement analyze table for low impact
> +# 
> +create table t1(a int primary key) engine=innodb;
> +insert into t1 values (0),(1),(2),(3),(4),(5),(6),(7),(8),(9);

you use t1 only to populate t2. You don't need to create a table for
thatr, use seq_0_to_9 - https://mariadb.com/kb/en/mariadb/sequence/

> +create table t2 (a int, b int, c int, key IDX1(a), key IDX2(a,b)) engine=innodb;
> +insert into t2 select a/10, a/2, a from test.t1;
> +SET DEBUG_SYNC='statistics_collection_start1 SIGNAL analyzing WAIT_FOR written';
> +analyze table t2 persistent for all;
> +connect  con1, localhost, root,,;
> +connection con1;
> +SET DEBUG_SYNC= 'now WAIT_FOR analyzing';
> +select count(*) from t2;
> +count(*)
> +10
> +insert into t2 values (333,333,333);
> +update t2 set a=1;
> +SET DEBUG_SYNC= 'now SIGNAL written';
> +connection default;
> +Table	Op	Msg_type	Msg_text
> +test.t2	analyze	status	Engine-independent statistics collected
> +test.t2	analyze	status	OK
> +select count(*) from t2;
> +count(*)
> +11
> +set debug_sync='RESET';
> +drop table t1,t2;

good direct test

> diff --git a/sql/sql_admin.cc b/sql/sql_admin.cc
> index 578b78d..933f3d0 100644
> --- a/sql/sql_admin.cc
> +++ b/sql/sql_admin.cc
> @@ -812,7 +773,73 @@ static bool mysql_admin_table(THD* thd, TABLE_LIST* tables,
>  
>      if (compl_result_code == HA_ADMIN_OK && collect_eis)
>      {

ok, I've quickly checked the code and I think (not sure) that there is
no need to reopen for innodb. But as you do, please, show the benefits
of it - add a test case for MyISAM too (remember, TL_READ allows
concurrent inserts).

besides, this reopening code needs a comment explaining why you're doing
it.

> +      enum_sql_command save_sql_command= lex->sql_command;
> +      trans_commit_stmt(thd);
> +      trans_commit(thd);
> +      thd->open_options|= extra_open_options;
> +      close_thread_tables(thd);
> +      table->table= NULL;
> +      thd->mdl_context.release_transactional_locks();
> +      lex->sql_command= save_sql_command;

where is lex->sql_command changed?

> +      table->mdl_request.init(MDL_key::TABLE, table->db, table->table_name,
> +                              MDL_SHARED_NO_READ_WRITE, MDL_TRANSACTION);
> +      table->mdl_request.set_type(MDL_SHARED_READ);
> +
> +      table->lock_type= TL_READ;
> +      open_error= open_only_one_table(thd, table,
> +                                      repair_table_use_frm,
> +                                      (view_operator_func != NULL));

can one analyze a view? how does that work?

> +      thd->open_options&= ~extra_open_options;
...


Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx