maria-developers team mailing list archive

[Oleksandr Byelkin <sanja@xxxxxxxxxxxxxxxx>]

Hi, Sergei!

On 03.08.2016 12:30, Sergei Golubchik wrote:
> 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/
It adds dependance on presence of the engine, but I hope it is not a big 
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@xxxxxxxxxxx
>
> _______________________________________________
> Mailing list: https://launchpad.net/~maria-developers
> Post to     : maria-developers@xxxxxxxxxxxxxxxxxxx
> Unsubscribe : https://launchpad.net/~maria-developers
> More help   : https://help.launchpad.net/ListHelp