Re: [Maria-developers] fc42991: MDEV-7901: re-implement analyze table for low impact
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@mariadb.org
Hi, Oleksandr!
Looks mostly ok, see few comments below
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
On Aug 01, Oleksandr Byelkin wrote: thatr, use seq_0_to_9 - https://mariadb.com/kb/en/mariadb/sequence/ It adds dependance on presence of the engine, but I hope it is not a big
Hi, Sergei! On 03.08.2016 12:30, Sergei Golubchik wrote: limitation (done).
+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.
Test & comment added.
+ 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?
no, I tought I removed it, but it is done now.
+ 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?
No. actually it is prohibited before we open table first time. I put DBUG_ASSERT here just in case and changed the call parameter.
+ thd->open_options&= ~extra_open_options; ...
Regards, Sergei Chief Architect MariaDB and security@mariadb.org
_______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
participants (2)
-
Oleksandr Byelkin -
Sergei Golubchik