- developers - lists.mariadb.org

[Maria-developers] Missing tests for MDEV-10057 and MDEV-10058
by Alexander Barkov 07 Sep '16

07 Sep '16

Hello Sergey and Igor, You recently closed MDEV-10057 and MDEV-10058, but forgot to add tests. - MDEV-10057 does not have tests at all. - MDEV-10058 has only the parser related test, but does not have this test: EXPLAIN SELECT * FROM (WITH a AS (SELECT * FROM t1) SELECT * FROM t2 NATURAL JOIN t3) AS d1; Can you please add the missing tests? Adding patches without tests makes merging painful. You never know if a merge (especially an automatic one) breaks something. Thanks!

2 2

[Maria-developers] MDEV-8909 union parser cleanup
by Alexander Barkov 07 Sep '16

07 Sep '16

Hi Sergei, Please review a patch for MDEV-8909. It removes some old remainders that are not needed anymore. Thanks.

2 2

[Maria-developers] MDEV-7145
by Phil Sweeney 05 Sep '16

05 Sep '16

Hi all, Re: https://jira.mariadb.org/browse/MDEV-7145 Delayed replication (a feature shipped in MySQL 5.6) has been a MariaDB feature request since 2014, and has been listed as 'major/red' priority in Jira since. It initially targeted 10.1, but missed the cut for some reason, and is now targeted at 10.2. It has the most votes of any open ticket at 26 (and only 1 closed ticket has more). I noticed we've had a couple of alpha releases of 10.2, and this ticket is open not assigned to anyone. Can anyone comment on whether this is still likely to make it into 10.2. Just wanting to know if it needs more lobbying or if it's 100% going to get looked at. This is certainly not a complaint - short of providing a patch (which I'm not capable of), is there anything else needed to increase the chances of this being implemented? Thanks! Phil

2 1

Re: [Maria-developers] [Commits] 97d212a: MDEV-10545: Server crashed in my_copy_fix_mb on querying I_S and P_S tables
by Nirbhay Choubey 02 Sep '16

02 Sep '16

Hi Svoj! Thanks for the review. On Fri, Sep 2, 2016 at 1:41 AM, Sergey Vojtovich <svoj(a)mariadb.org> wrote: > Hi Nirbhay, > > Ok to push, one question below. > > On Thu, Sep 01, 2016 at 12:46:58PM -0400, Nirbhay Choubey wrote: > > revision-id: 97d212a34ff4e0558126bc393bbef97036611d83 > (mariadb-10.1.17-4-g97d212a) > > parent(s): a322651b8aa702e58d473edfae26606f10a089fb > > author: Nirbhay Choubey > > committer: Nirbhay Choubey > > timestamp: 2016-09-01 12:46:56 -0400 > > message: > > > > MDEV-10545: Server crashed in my_copy_fix_mb on querying I_S and P_S > tables > > > > Once THDs have been added to the global "threads" list, > > they must modify query_string only after acquiring per- > > thread LOCK_thd_data mutex. > > > > --- > > sql/log_event.cc | 2 +- > > sql/sql_acl.cc | 8 ++++---- > > sql/sql_parse.cc | 7 +++---- > > 3 files changed, 8 insertions(+), 9 deletions(-) > > > > diff --git a/sql/log_event.cc b/sql/log_event.cc > > index afa58af..66e7c60 100644 > > --- a/sql/log_event.cc > > +++ b/sql/log_event.cc > > @@ -6022,7 +6022,7 @@ int Load_log_event::do_apply_event(NET* net, > rpl_group_info *rgi, > > new_db.str= (char *) rpl_filter->get_rewrite_db(db, &new_db.length); > > thd->set_db(new_db.str, new_db.length); > > DBUG_ASSERT(thd->query() == 0); > > - thd->reset_query_inner(); // Should not be needed > > + if (thd->query()) thd->reset_query(); // Should not be needed > Why if? Original code didn't have it. > Now that we call reset_query(), the check has been added to avoid it and an extra mutex. And from the above assert, I think it will be ever better have this check within unlikely(). Best, Nirbhay > > Regards, > Sergey > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits

2 2

Re: [Maria-developers] 811e062: MDEV-10035: DBUG_ASSERT on CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE
by Sergei Golubchik 31 Aug '16

31 Aug '16

Hi, Oleksandr! On Jun 02, Oleksandr Byelkin wrote: > revision-id: 811e062f812e450a2d0ae2c3be7f4ad1b90eadc8 (mariadb-10.2.0-53-g811e062) > parent(s): eb86c32225db2a76c6f256130e7bb316900a9408 > committer: Oleksandr Byelkin > timestamp: 2016-06-02 12:12:26 +0200 > message: > > MDEV-10035: DBUG_ASSERT on CREATE VIEW v1 AS SELECT * FROM t1 FOR UPDATE > > Ability to print lock type added. > Restoring correct lock type for CREATE VIEW added. So your fix is to save/print the lock option, but not let it to affect the locking... I suppose it may be ok for views, let's keep it your way for now. Ok to push Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 48d1ba6: MDEV-10296 - Multi-instance table cache
by Sergei Golubchik 31 Aug '16

31 Aug '16

Hi, Sergey! Ok, see comments below. But why is this supposed to improve scalability? Are you talking about the use case of one table being access by multiple threads? Or this helps in other cases too? And what is the effect of the more expensive tc_remove_all_unused_tables() ? On Jun 29, Sergey Vojtovich wrote: > revision-id: 48d1ba6097939efe5efd28dd3ed9d281cc2bc2f4 (mariadb-10.2.0-93-g48d1ba6) > parent(s): 8bec9746f0d5b363f385713035ca3f2daff34e1c > committer: Sergey Vojtovich > timestamp: 2016-06-29 16:33:08 +0400 > message: > > MDEV-10296 - Multi-instance table cache > > Improve scalability by implementing multi-instance table cache. > > diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > index e075c64..22b12a9 100644 > --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result > @@ -4559,6 +4559,20 @@ NUMERIC_BLOCK_SIZE 1 > ENUM_VALUE_LIST NULL > READ_ONLY NO > COMMAND_LINE_ARGUMENT REQUIRED > +VARIABLE_NAME TABLE_OPEN_CACHE_INSTANCES > +SESSION_VALUE NULL > +GLOBAL_VALUE 1 > +GLOBAL_VALUE_ORIGIN COMPILE-TIME Default value 1 and it's not auto-tuned on startup? You think multi-instance table cache is better stay unused? :) If not, either use a reasonable default or, may be, choose it on startup based on my_gencpus() > +DEFAULT_VALUE 1 > +VARIABLE_SCOPE GLOBAL > +VARIABLE_TYPE BIGINT UNSIGNED > +VARIABLE_COMMENT The number of table cache instances > +NUMERIC_MIN_VALUE 1 > +NUMERIC_MAX_VALUE 64 > +NUMERIC_BLOCK_SIZE 1 > +ENUM_VALUE_LIST NULL > +READ_ONLY YES > +COMMAND_LINE_ARGUMENT REQUIRED > VARIABLE_NAME THREAD_CACHE_SIZE > SESSION_VALUE NULL > GLOBAL_VALUE 151 > diff --git a/sql/table_cache.cc b/sql/table_cache.cc > index bfe51df..f1eb006 100644 > --- a/sql/table_cache.cc > +++ b/sql/table_cache.cc > @@ -125,67 +114,101 @@ static int fix_thd_pins(THD *thd) > part of table definition cache. > */ > > +struct Share_free_tables > { > + typedef I_P_List <TABLE, TABLE_share> List; > + List list; > + char pad[CPU_LEVEL1_DCACHE_LINESIZE]; a comment, please, explaining the padding. Or see a suggestion at the end. > +}; > > + > +struct Table_cache_instance > +{ > + /** > + Protects free_tables (TABLE::global_free_next and TABLE::global_free_prev), > + records, Share_free_tables::List (TABLE::prev and TABLE::next), > + TABLE::in_use. > + */ > + mysql_mutex_t LOCK_table_cache; > + I_P_List <TABLE, I_P_List_adapter<TABLE, &TABLE::global_free_next, > + &TABLE::global_free_prev>, > + I_P_List_null_counter, I_P_List_fast_push_back<TABLE> > > + free_tables; > + ulong records; > + char pad[CPU_LEVEL1_DCACHE_LINESIZE]; why is it padded? > + > + Table_cache_instance(): records(0) > { > + mysql_mutex_init(key_LOCK_table_cache, &LOCK_table_cache, > + MY_MUTEX_INIT_FAST); > } > + > + ~Table_cache_instance() > + { > + mysql_mutex_destroy(&LOCK_table_cache); > + DBUG_ASSERT(free_tables.is_empty()); > + DBUG_ASSERT(records == 0); > + } > +}; > > > +static Table_cache_instance *tc; > > + > +static inline Share_free_tables *SHARE_FREE_TABLES(TDC_element *element) fairly unconventional letter case choice :) Besides, instead of pointer arithmetics, you could've simply put Share_free_tables free_tables[1]; at the end of TDC_element. > { > + return (Share_free_tables*) (element + 1); > } > > > +static void intern_close_table(TABLE *table) > +{ > + delete table->triggers; > + DBUG_ASSERT(table->file); old intern_close_table had if (table->file) // Not true if placeholder do you mean, these placeholder TABLEs can no longer be in the table cache? > + closefrm(table); > + tdc_release_share(table->s); old intern_close_table() also did table->alias.free(); > + my_free(table); > +} > > + > +/** > + Get number of TABLE objects (used and unused) in table cache. > */ > > +uint tc_records(void) > { > + ulong total= 0; > + for (ulong i= 0; i < tc_instances; i++) > + { > + mysql_mutex_lock(&tc[i].LOCK_table_cache); > + total+= tc[i].records; > + mysql_mutex_unlock(&tc[i].LOCK_table_cache); > + } > + return total; > } > > > /** > Remove TABLE object from table cache. > */ > > static void tc_remove_table(TABLE *table) > { > + TDC_element *element= table->s->tdc; > + > + mysql_mutex_lock(&element->LOCK_table_share); > + /* Wait for MDL deadlock detector to complete traversing tdc.all_tables. */ > + while (element->all_tables_refs) > + mysql_cond_wait(&element->COND_release, &element->LOCK_table_share); > + element->all_tables.remove(table); > + mysql_mutex_unlock(&element->LOCK_table_share); > + > + intern_close_table(table); > } > > > static void tc_remove_all_unused_tables(TDC_element *element, > + Share_free_tables::List *purge_tables, > bool mark_flushed) > { > TABLE *table; > @@ -200,10 +223,18 @@ static void tc_remove_all_unused_tables(TDC_element *element, > */ > if (mark_flushed) > element->flushed= true; > + for (ulong i= 0; i < tc_instances; i++) > { > + mysql_mutex_lock(&tc[i].LOCK_table_cache); > + while ((table= SHARE_FREE_TABLES(element)[i].list.pop_front())) > + { > + tc[i].records--; > + tc[i].free_tables.remove(table); > + DBUG_ASSERT(element->all_tables_refs == 0); > + element->all_tables.remove(table); > + purge_tables->push_front(table); > + } > + mysql_mutex_unlock(&tc[i].LOCK_table_cache); > } > } > > @@ -225,7 +256,7 @@ static void tc_remove_all_unused_tables(TDC_element *element, > > struct tc_purge_arg > { > + Share_free_tables::List purge_tables; > bool mark_flushed; > }; > > @@ -281,79 +298,33 @@ static TABLE *tc_free_tables_back(TDC_element *element) > - free evicted object > */ > > +void tc_add_table(THD *thd, TABLE *table) > { > + ulong i= thd->thread_id % tc_instances; > + TABLE *LRU_table= 0; > + TDC_element *element= table->s->tdc; > > + DBUG_ASSERT(table->in_use == thd); > mysql_mutex_lock(&element->LOCK_table_share); > + /* Wait for MDL deadlock detector to complete traversing tdc.all_tables. */ > + while (element->all_tables_refs) > + mysql_cond_wait(&element->COND_release, &element->LOCK_table_share); > + element->all_tables.push_front(table); > mysql_mutex_unlock(&element->LOCK_table_share); > > + mysql_mutex_lock(&tc[i].LOCK_table_cache); > + if (tc[i].records == tc_size && (LRU_table= tc[i].free_tables.pop_front())) > { > + SHARE_FREE_TABLES(LRU_table->s->tdc)[i].list.remove(LRU_table); > + /* Needed if MDL deadlock detector chimes in before tc_remove_table() */ > + LRU_table->in_use= thd; How can MDL deadlock detector get to the unused table? It traverses the lock dependency graph, and an unused table cannot be locked, can it? > } > + else > + tc[i].records++; > + mysql_mutex_unlock(&tc[i].LOCK_table_cache); > + > + if (LRU_table) > + tc_remove_table(LRU_table); > } > > > @@ -369,10 +340,11 @@ void tc_add_table(THD *thd, TABLE *table) > > static TABLE *tc_acquire_table(THD *thd, TDC_element *element) > { > + ulong i= thd->thread_id % tc_instances; > TABLE *table; > > - mysql_mutex_lock(&element->LOCK_table_share); > - table= element->free_tables.pop_front(); > + mysql_mutex_lock(&tc[i].LOCK_table_cache); > + table= SHARE_FREE_TABLES(element)[i].list.pop_front(); > if (table) > { > DBUG_ASSERT(!table->in_use); > @@ -381,8 +353,9 @@ static TABLE *tc_acquire_table(THD *thd, TDC_element *element) > DBUG_ASSERT(table->db_stat && table->file); > /* The children must be detached from the table. */ > DBUG_ASSERT(!table->file->extra(HA_EXTRA_IS_ATTACHED_CHILDREN)); > + tc[i].free_tables.remove(table); > } > - mysql_mutex_unlock(&element->LOCK_table_share); > + mysql_mutex_unlock(&tc[i].LOCK_table_cache); > return table; > } > > @@ -413,40 +386,27 @@ static TABLE *tc_acquire_table(THD *thd, TDC_element *element) > @retval false object released > */ > > -bool tc_release_table(TABLE *table) > +void tc_release_table(TABLE *table) > { > + ulong i= table->in_use->thread_id % tc_instances; > DBUG_ASSERT(table->in_use); > DBUG_ASSERT(table->file); > > - if (table->needs_reopen() || tc_records() > tc_size) > + mysql_mutex_lock(&tc[i].LOCK_table_cache); > + if (table->needs_reopen() || table->s->tdc->flushed || > + tc[i].records > tc_size) > { > - mysql_mutex_lock(&table->s->tdc->LOCK_table_share); > - goto purge; > + tc[i].records--; > + mysql_mutex_unlock(&tc[i].LOCK_table_cache); > + tc_remove_table(table); Does it mean that if the table cache is full, you free the table, instead of adding it to the cache? That's not very LRU-ish > + } > + else > + { > + table->in_use= 0; > + SHARE_FREE_TABLES(table->s->tdc)[i].list.push_front(table); > + tc[i].free_tables.push_back(table); > + mysql_mutex_unlock(&tc[i].LOCK_table_cache); > } > } > > > @@ -573,23 +535,29 @@ static uchar *tdc_hash_key(const TDC_element *element, size_t *length, > Initialize table definition cache. > */ > > -void tdc_init(void) > +bool tdc_init(void) > { > DBUG_ENTER("tdc_init"); > #ifdef HAVE_PSI_INTERFACE > - init_tc_psi_keys(); > + mysql_mutex_register("sql", all_tc_mutexes, array_elements(all_tc_mutexes)); > + mysql_cond_register("sql", all_tc_conds, array_elements(all_tc_conds)); > #endif > + /* Extra instance is allocated to avoid false sharing */ > + if (!(tc= new Table_cache_instance[tc_instances + 1])) > + DBUG_RETURN(true); > tdc_inited= true; > mysql_mutex_init(key_LOCK_unused_shares, &LOCK_unused_shares, > MY_MUTEX_INIT_FAST); > tdc_version= 1L; /* Increments on each reload */ > - lf_hash_init(&tdc_hash, sizeof(TDC_element), LF_HASH_UNIQUE, 0, 0, > + lf_hash_init(&tdc_hash, sizeof(TDC_element) + > + sizeof(Share_free_tables) * tc_instances, > + LF_HASH_UNIQUE, 0, 0, > (my_hash_get_key) tdc_hash_key, > &my_charset_bin); > tdc_hash.alloc.constructor= lf_alloc_constructor; > tdc_hash.alloc.destructor= lf_alloc_destructor; > tdc_hash.initializer= (lf_hash_initializer) tdc_hash_initializer; > - DBUG_VOID_RETURN; > + DBUG_RETURN(false); > } > > > @@ -631,6 +599,7 @@ void tdc_deinit(void) > tdc_inited= false; > lf_hash_destroy(&tdc_hash); > mysql_mutex_destroy(&LOCK_unused_shares); > + delete [] tc; > } > DBUG_VOID_RETURN; > } > @@ -1038,7 +1007,7 @@ bool tdc_remove_table(THD *thd, enum_tdc_remove_table_type remove_type, > const char *db, const char *table_name, > bool kill_delayed_threads) > { > - TDC_element::TABLE_list purge_tables; > + Share_free_tables::List purge_tables; > TABLE *table; > TDC_element *element; > uint my_refs= 1; > diff --git a/sql/table_cache.h b/sql/table_cache.h > index c05baae..4f449cb 100644 > --- a/sql/table_cache.h > +++ b/sql/table_cache.h > @@ -45,7 +43,7 @@ struct TDC_element > for this share. > */ > All_share_tables_list all_tables; > - TABLE_list free_tables; > + char pad[CPU_LEVEL1_DCACHE_LINESIZE]; // free_tables follows this immediately hmm. may be you'd rather put padding in one place - at the beginning of Share_free_tables? > }; Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 3

Re: [Maria-developers] 9019cfd: MDEV-9593 - Print the real version in the error log
by Sergei Golubchik 31 Aug '16

31 Aug '16

Hi, Sergey! Ok to push. On Aug 26, Sergey Vojtovich wrote: > revision-id: 9019cfd215819dd765501e4b8d63b36396f3e558 (mariadb-10.2.1-22-g9019cfd) > parent(s): b4eed0afa2aef878ad7b9849a2ff2dabfe85dfe0 > committer: Sergey Vojtovich > timestamp: 2016-08-26 16:39:32 +0400 > message: > > MDEV-9593 - Print the real version in the error log Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] db9822d: MDEV-8931: (server part of) session state tracking
by Sergei Golubchik 31 Aug '16

31 Aug '16

Hi, Oleksandr! Looks ok to push, thanks! On Aug 09, Oleksandr Byelkin wrote: > revision-id: db9822df31917c3d0162322a77bcd4433bfb705e (mariadb-10.2.1-15-gdb9822d) > parent(s): b5e0f70e04cec625c99760e5bfb28bb31bc972ed > committer: Oleksandr Byelkin > timestamp: 2016-08-09 15:49:30 +0200 > message: > > MDEV-8931: (server part of) session state tracking > > Postreview fixes. > New MySQL tests fixes. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 4949925: MDEV-10421 duplicate CHECK CONSTRAINTs.
by Sergei Golubchik 31 Aug '16

31 Aug '16

Hi, Alexey! Looks good, thanks! See few comments below, then ok to push! On Aug 30, Alexey Botchkov wrote: > revision-id: 49499250c04b631bd908945b7c59d6affaeb2703 (mariadb-10.2.1-9-g4949925) > parent(s): 08683a726773f8cdf16a4a3dfb3920e5f7842481 > committer: Alexey Botchkov > timestamp: 2016-08-30 16:43:05 +0400 > message: > > MDEV-10421 duplicate CHECK CONSTRAINTs. > > mysql_prepare_create_table fixed so it doesn't let duplicating > constraint names. Syntax for CONSTRAINT IF NOT EXISTS added > and handled in mysql_alter_table. > > diff --git a/sql/field.h b/sql/field.h > index 13a0e91..7b271b0 100644 > --- a/sql/field.h > +++ b/sql/field.h > @@ -606,6 +606,7 @@ class Virtual_column_info: public Sql_alloc > /* Flag indicating that the field is physically stored in the database */ > bool stored_in_db; > bool utf8; /* Already in utf8 */ > + bool create_if_not_exists; You apparently forgot to remove create_if_not_exists. It doesn't seem to be used anywhere. > /* The expression to compute the value of the virtual column */ > Item *expr_item; > /* Text representation of the defining expression */ > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index e745fe8..c120fbc 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -6130,6 +6146,38 @@ handle_if_exists_options(THD *thd, TABLE *table, Alter_info *alter_info) > } > #endif /*WITH_PARTITION_STORAGE_ENGINE*/ > > + /* ADD CONSTRAINT IF NOT EXISTS. */ > + { > + List_iterator<Virtual_column_info> it(alter_info->check_constraint_list); > + Virtual_column_info *check; > + TABLE_SHARE *share= table->s; > + uint c; > + while ((check=it++)) > + { > + if (!check->flags /* & Alter_info::CHECK_CONSTRAINT_IF_NOT_EXISTS */ && why is that? because CHECK_CONSTRAINT_IF_NOT_EXISTS is the only possible flag now? I'd still prefer it a bit more future-proof, better to check the flag here. > + check->name.length) > + continue; > + check->flags= 0; > + for (c= share->field_check_constraints; > + c < share->table_check_constraints ; c++) > + { > + Virtual_column_info *dup= table->check_constraints[c]; > + if (dup->name.length == check->name.length && > + my_strcasecmp(system_charset_info, > + check->name.str, dup->name.str) == 0) > + { > + push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE, > + ER_DUP_CONSTRAINT_NAME, ER_THD(thd, ER_DUP_CONSTRAINT_NAME), > + "CHECK", check->name.str); > + it.remove(); > + if (alter_info->check_constraint_list.elements == 0) > + alter_info->flags&= ~Alter_info::ALTER_ADD_CHECK_CONSTRAINT; > + > + break; > + } > + } > + } > + } > DBUG_VOID_RETURN; > } You seem to forgot to add #define ER_FK_DUP_NAME ER_DUP_CONSTRAINT_NAME to mysql.h. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

[Maria-developers] MDEV-10702 Crash in SET STATEMENT FOR EXECUTE
by Alexander Barkov 31 Aug '16

31 Aug '16

Hello Sergei, Please review a patch for mdev-10702.diff Thanks!

1 0