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 [email protected] _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
