- developers - lists.mariadb.org

[Maria-developers] review of mdev-5095 (executing triggers on slave)
by Michael Widenius 19 Dec '13

19 Dec '13

Hi! Here is the review of mdev-5095 I got the diff from: bzr diff -r3942.. > === modified file 'mysql-test/r/mysqld--help.result' > --- mysql-test/r/mysqld--help.result 2012-10-18 21:33:06 +0000 > +++ mysql-test/r/mysqld--help.result 2013-12-03 10:53:01 +0000 > @@ -747,6 +747,14 @@ > --slave-net-timeout=# > Number of seconds to wait for more data from a > master/slave connection before aborting the read > + --slave-run-triggers-for-rbr=name > + Modes for how triggers in row-base replication on slave > + side will be executed. Legal values are NO (default), YES > + and LOGGING. NO means that trigger for RBR will not be > + running on slave YES and LOGGING means that triggers will > + be running on slave (if there was not triggers running on > + the naster), LOGGING also mens that flag about executed > + triggers will be written to binlog. Suggested new text: --slave-run-triggers-for-rbr=name Modes for how triggers in row-base replication on slave side will be executed. Legal values are NO (default), YES and LOGGING. NO means that trigger for RBR will not be running on slave. YES and LOGGING means that triggers will be running on slave, if there was not triggers running on the master for the statement. LOGGING also means that the executed triggers will be written to binlog. <cut> > === modified file 'sql/log_event.cc' <cut> > -#if !defined(MYSQL_CLIENT) && defined(HAVE_REPLICATION) > +#if defined(MYSQL_SERVER) && defined(HAVE_REPLICATION) Was the above needed or mostly an optimization? > + > +static void restore_empty_query_table_list(LEX *lex) > +{ > + if (lex->first_not_own_table()) > + (*lex->first_not_own_table()->prev_global)= NULL; > + lex->query_tables= NULL; > + lex->query_tables_last= &lex->query_tables; > +} Add a comment of what is the purpose of the above function. > @@ -8340,6 +8351,22 @@ int Rows_log_event::do_apply_event(Relay > /* A small test to verify that objects have consistent types */ > DBUG_ASSERT(sizeof(thd->variables.option_bits) == sizeof(OPTION_RELAXED_UNIQUE_CHECKS)); > > + if (slave_run_triggers_for_rbr) > + { > + LEX *lex= thd->lex; > + uint8 new_trg_event_map= get_trg_event_map(); > + > + DBUG_ASSERT(lex->query_tables == NULL); > + if ((lex->query_tables= rli->tables_to_lock)) > + rli->tables_to_lock->prev_global= &lex->query_tables; Add a comment why we have to set prev_global (not obvious). It's also not obvious why restore_empty_tables() is not resetting rli->tables_to_lock->prev_global as it's reseting the the rest of the changed things. > @@ -8429,8 +8462,11 @@ int Rows_log_event::do_apply_event(Relay > */ > TABLE_LIST *ptr= rli->tables_to_lock; > for (uint i=0 ; ptr && (i < rli->tables_to_lock_count); ptr= ptr->next_global, i++) > + { > const_cast<Relay_log_info*>(rli)->m_table_map.set_table(ptr->table_id, ptr->table); > - What does this test really do ? (I was quickly checking the code to understand what m_table_id stands for, but it was not obvious). > + if (m_table_id == ptr->table_id) > + master_had_triggers= ((RPL_TABLE_LIST*)ptr)->master_had_triggers; <cut> > if (table) > { > bool transactional_table= table->file->has_transactions(); > @@ -8618,6 +8656,9 @@ int Rows_log_event::do_apply_event(Relay > */ > thd->reset_current_stmt_binlog_format_row(); > thd->is_slave_error= 1; > + /* remove trigger's tables */ > + if (slave_run_triggers_for_rbr) > + restore_empty_query_table_list(thd->lex); > DBUG_RETURN(error); > } You do the above 'restore_empty_query_table_list(thd->lex);' before every return in this function, except the last return. - Isn't it better to do a 'error:' tag at the end of the function to ensure we don't miss any cases ? - Add a comment why we don't need to do this at the very end of Rows_log_event::do_apply_event(). Something like: /* We don't have to call restore_empty_query_table_list() here as it's called in rows_event_stmt_cleanup(). */ When is lex->query_tables used? Is it only used by 'open' ? If yes, isn't it enough to always reset it after the 'open' call? > @@ -9773,6 +9831,29 @@ Write_rows_log_event::do_before_row_oper > from the start. > */ > } > + if (slave_run_triggers_for_rbr && !master_had_triggers && m_table->triggers ) > + { > + if (m_table->triggers->has_triggers(TRG_EVENT_DELETE, > + TRG_ACTION_AFTER)) > + { > + /* > + The table has AFTER DELETE triggers that might access to > + subject table and therefore might need delete to be done > + immediately. So we turn-off the batching. > + */ > + (void) m_table->file->extra(HA_EXTRA_DELETE_CANNOT_BATCH); > + } > + if (m_table->triggers->has_triggers(TRG_EVENT_UPDATE, > + TRG_ACTION_AFTER)) > + { > + /* > + The table has AFTER UPDATE triggers that might access to subject > + table and therefore might need update to be done immediately. > + So we turn-off the batching. > + */ > + (void) m_table->file->extra(HA_EXTRA_UPDATE_CANNOT_BATCH); > + } > + } Good that you noticed and fixed the above. However, why not just call the function prepare_triggers_for_insert_stmt() that does exactly the same thing? <cut> > @@ -10749,6 +10894,17 @@ Delete_rows_log_event::do_before_row_ope > */ > return 0; > } > + if (slave_run_triggers_for_rbr && !master_had_triggers && m_table->triggers && > + m_table->triggers->has_triggers(TRG_EVENT_DELETE, > + TRG_ACTION_AFTER)) > + { > + /* > + The table has AFTER DELETE triggers that might access to subject table > + and therefore might need delete to be done immediately. So we turn-off > + the batching. > + */ > + (void) m_table->file->extra(HA_EXTRA_DELETE_CANNOT_BATCH); > + } Would be better if we could do a function 'prepare_triggers_for_delete_stmt()' and use the same code here and in sql_delete.cc <cut> > @@ -10877,6 +11049,18 @@ Update_rows_log_event::do_before_row_ope > > m_table->timestamp_field_type= TIMESTAMP_NO_AUTO_SET; > > + if (slave_run_triggers_for_rbr && !master_had_triggers && m_table->triggers && > + m_table->triggers->has_triggers(TRG_EVENT_UPDATE, > + TRG_ACTION_AFTER)) > + { > + /* > + The table has AFTER UPDATE triggers that might access to subject > + table and therefore might need update to be done immediately. > + So we turn-off the batching. > + */ > + (void) m_table->file->extra(HA_EXTRA_UPDATE_CANNOT_BATCH); > + } > + Would be better if we could do a function 'prepare_triggers_for_update_stmt()' and use the same code here and in sql_update.cc <cut> Regards, Monty

2 4

[Maria-developers] FW: Rev 3758: MDEV-5470: Cost-based subquery item pushdown must take into account semi-joins
by Sergei Petrunia 18 Dec '13

18 Dec '13

Hi Timour, Please find below a patch that should implement execution-time protection against malfunction between condition moving and semi-joins. There needs also to be an optimization-time counterpart, which should have the same logic. Will you code it yourself or want me to? ----- Forwarded message from Sergei Petrunia <psergey(a)askmonty.org> ----- Date: Thu, 19 Dec 2013 02:49:22 +0400 From: Sergei Petrunia <psergey(a)askmonty.org> To: commits(a)mariadb.org Subject: Rev 3758: MDEV-5470: Cost-based subquery item pushdown must take into account semi-joins At http://bazaar.launchpad.net/~maria-captains/maria/10.0-mdev83 ------------------------------------------------------------ revno: 3758 committer: Sergey Petrunya <psergey(a)askmonty.org> branch nick: 10.0-mdev83 timestamp: Thu 2013-12-19 06:50:28 +0400 message: MDEV-5470: Cost-based subquery item pushdown must take into account semi-joins - When moving expensive conditions, take into account that semi-joins limit where they can be moved. - This is only execution part in JOIN::setup_dynamic_conditions(). Optimization part is not yet present. - No testcases yet. - Moving inside SJ-Materialization nests doesn't seem to be supported (either before this patch or after. This patch itself does take SJ-Materialization into account) === modified file 'sql/item_cmpfunc.cc' --- sql/item_cmpfunc.cc 2013-11-01 11:10:38 +0000 +++ sql/item_cmpfunc.cc 2013-12-19 02:50:28 +0000 @@ -6591,6 +6591,8 @@ void Item_func_collect_stats::dbug_print } #endif /*DBUG_OFF*/ +JOIN_TAB *find_last_tab_for_dynamic_cond(table_map cond_tables, + JOIN *join, JOIN_TAB *join_tab); Item_func_dynamic_cond::Item_func_dynamic_cond(Item *cond, JOIN_TAB *atab, JOIN_TAB **ctab) : Item_func_collect_stats(cond) @@ -6624,13 +6626,15 @@ Item_func_dynamic_cond::Item_func_dynami if (active_tab->bush_root_tab) { first_tab= active_tab->bush_root_tab->bush_children->start; - last_tab= active_tab->bush_root_tab->bush_children->end; + //last_tab= active_tab->bush_root_tab->bush_children->end; } else { first_tab= join->join_tab + join->const_tables; - last_tab= join->join_tab + join->top_join_tab_count; + //last_tab= join->join_tab + join->top_join_tab_count; } + last_tab= find_last_tab_for_dynamic_cond(cond->used_tables(), + active_tab->join, active_tab); DBUG_ASSERT(active_tab >= first_tab && active_tab < last_tab); === modified file 'sql/item_cmpfunc.h' --- sql/item_cmpfunc.h 2013-10-18 19:56:55 +0000 +++ sql/item_cmpfunc.h 2013-12-19 02:50:28 +0000 @@ -561,14 +561,20 @@ class Item_func_collect_stats : public I class Item_func_dynamic_cond : public Item_func_collect_stats { -protected: +protected: // as if there were any ancestor classes? List<Item_subselect> subqueries; /* The join_tab where the condition is currently 'pushed'.*/ st_join_table *active_tab; /* The join_tab currently being executed. */ st_join_table **exec_tab; /* The boundaries of the sub-plan where this predicate can be pushed. */ - st_join_table *first_tab, *last_tab; + st_join_table *first_tab; + st_join_table *last_tab; //psergey-todo: this should point to the last tab. + // conditions depending on semi-join tables have + // limits re. how far they can be moved. +public: + inline st_join_table * get_last_tab() { return last_tab; } +protected: /* The relative access cost based on optimizer estimates. This is the cost of all access methods per one record from the first table in the plan. === modified file 'sql/opt_subselect.cc' --- sql/opt_subselect.cc 2013-11-04 15:56:09 +0000 +++ sql/opt_subselect.cc 2013-12-19 02:50:28 +0000 @@ -1480,6 +1480,7 @@ static bool convert_subq_to_sj(JOIN *par sj_nest->sj_subq_pred= subq_pred; sj_nest->original_subq_pred_used_tables= subq_pred->used_tables() | subq_pred->left_expr->used_tables(); + sj_nest->sj_strategys_join_tab= uint(-1); /* Nests do not participate in those 'chains', so: */ /* sj_nest->next_leaf= sj_nest->next_local= sj_nest->next_global == NULL*/ emb_join_list->push_back(sj_nest); @@ -4321,6 +4322,21 @@ int init_dups_weedout(JOIN *join, uint f /* + For each SJ-inner table in [start_tab; end_tab], set emb_sj_nest to idx +*/ +static void update_sj_strategys_join_tab(JOIN_TAB *start_tab, + JOIN_TAB *end_tab, + uint idx) +{ + for (JOIN_TAB *j= start_tab; j < end_tab; j++) + { + TABLE_LIST *emb_nest; + if ((emb_nest= j->emb_sj_nest)) + emb_nest->sj_strategys_join_tab= idx; + } +} + +/* Setup the strategies to eliminate semi-join duplicates. SYNOPSIS @@ -4448,6 +4464,10 @@ int setup_semijoin_dups_elimination(JOIN for (uint j= i; j < i + pos->n_sj_tables; j++) join->join_tab[j].inside_loosescan_range= TRUE; + + update_sj_strategys_join_tab(join->join_tab + i, + join->join_tab + i + pos->n_sj_tables, + i + pos->n_sj_tables - 1); /* Calculate key length */ keylen= 0; @@ -4461,6 +4481,7 @@ int setup_semijoin_dups_elimination(JOIN tab[pos->n_sj_tables - 1].do_firstmatch= tab; i+= pos->n_sj_tables; pos+= pos->n_sj_tables; + break; } case SJ_OPT_DUPS_WEEDOUT: @@ -4503,6 +4524,9 @@ int setup_semijoin_dups_elimination(JOIN break; } } + update_sj_strategys_join_tab(join->join_tab + i, + join->join_tab + i + pos->n_sj_tables, + i + pos->n_sj_tables - 1); init_dups_weedout(join, first_table, i, i + pos->n_sj_tables - first_table); i+= pos->n_sj_tables; @@ -4558,6 +4582,9 @@ int setup_semijoin_dups_elimination(JOIN } } j[-1].do_firstmatch= jump_to; + update_sj_strategys_join_tab(join->join_tab + i, + join->join_tab + i + pos->n_sj_tables, + i + pos->n_sj_tables - 1); i+= pos->n_sj_tables; pos+= pos->n_sj_tables; === modified file 'sql/sql_select.cc' --- sql/sql_select.cc 2013-11-08 12:36:02 +0000 +++ sql/sql_select.cc 2013-12-19 02:50:28 +0000 @@ -24608,8 +24608,8 @@ double JOIN::static_pushdown_cost(uint i @param pred the predicate to be wrapped in Item_func_dynamic_cond @param init_tab the initial 'active' JOIN_TAB where the dynamic predicate is pushed to - @param cur_tab pointer to the JOIN_TAB* that stores the current JOIN_TAB - during execution + @param active_tab_ptr pointer to the JOIN_TAB* that stores the current JOIN_TAB + during execution */ static Item_func_collect_stats * wrap_with_dynamic_conds(Item *pred, JOIN_TAB *init_tab, JOIN_TAB **active_tab_ptr, @@ -24674,6 +24674,81 @@ wrap_with_dynamic_conds(Item *pred, JOIN } +/* + @brief Find the last JOIN_TAB that a condition can be attached. + + + @param cond_tables used_tables() of the condition that we intend to move. + @param join_tab The left-most join tab where the condition can be + attached (in other words, default attachment point) + + @detail + In a regular inner join, one can take a condition that is attached to a + table $T, and move it to any table that comes after $T. + + When semi-joins are present, this is no longer true. Detailed explanation + can be found in maria-developers@, email "Movable conditions and semi-joins". + In short: + + - Inside an SJ-Materialization nest, a condition can be moved to any table to + the right, but it must stay within the nest. (We don't support nested + semi-joins, so one will not find semi-joins inside an SJ-M nest) + + - All other Semi-Join strategies have a "range" where they apply. If a + condition depends on a table that is within a semi-join, it must not be + moved over the right edge of the range. +*/ + +JOIN_TAB *find_last_tab_for_dynamic_cond(table_map cond_tables, + JOIN *join, JOIN_TAB *join_tab) +{ + if (join_tab->bush_root_tab) + { + /* + We are inside SJ-Materialization nest. There are no semi-joins here, + return the last tab in the nest. + */ + return join_tab->bush_root_tab->bush_children->end; + } + + /* Start from the right-most bound */ + uint last_tab = join->top_join_tab_count; + + cond_tables= cond_tables & ~(PSEUDO_TABLE_BITS | join->const_table_map); + /* + Walk through semi-joins that this table depends on, and check their last + join tabs. The smallest last join tab sets the limit about how far right + the condition can be moved. + */ + Table_map_iterator tm_it(cond_tables); + int tableno; + while ((tableno = tm_it.next_bit()) != Table_map_iterator::BITMAP_END) + { + TABLE_LIST *sj_nest; + if ((sj_nest= join->map2table[tableno]->emb_sj_nest)) + { + if (sj_nest->is_active_sjm()) + { + /* + Whoa this condition also depends on a table from within + SJ-Materialization nest. (Can happen possible because of equality + propagation/substitution). If we didn't return earlier in this + function, we're not inside a semi-join nest. A value that depends + on a table within a SJ-Materialization nest means that it depends on + the materialized table columns. + */ + continue; + } + + if (sj_nest->sj_strategys_join_tab != uint(-1) && + sj_nest->sj_strategys_join_tab < last_tab) + last_tab= sj_nest->sj_strategys_join_tab; + } + } + return join->join_tab + last_tab; +} + + /** Make expensive subqueries dynamically pushdownable. */ @@ -24703,7 +24778,12 @@ bool JOIN::setup_dynamic_conditions() Update the least cardinality join_tab for each tab for the chosen join order. Check if there are any conditions at all that need to be made dynamic. */ - min_tab= NULL; + min_tab= NULL; + /* + psergey-todo: the following loop should walk inside SJ-Materialization + nests, too. Maybe, this whole function should be invoked for each + SJ-Materilization nest. + */ for (tab= last_tab - 1; tab >= first_tab; tab--) { if (optimizer_flag(thd, OPTIMIZER_SWITCH_STATIC_COND_PUSHDOWN)) @@ -24746,7 +24826,8 @@ bool JOIN::setup_dynamic_conditions() */ for (tab= first_tab; tab < last_tab; tab++) { - Item_func_collect_stats *wrapped_select_cond= NULL, *cur_wrapped_cond; + Item_func_collect_stats *wrapped_select_cond= NULL; + Item_func_dynamic_cond *cur_wrapped_cond; if (tab->select_cond && !(wrapped_select_cond= @@ -24800,14 +24881,29 @@ bool JOIN::setup_dynamic_conditions() Add all dynamic conditions pushed to previous JOIN_TABs also to the current JOIN_TAB. This allows to "move" a dynamic condition from one tab to another by enabling it for the corrseponding tab. + + psergey-todo: conditions have limits on how far to the right they can be + moved (Item_func_dynamic_cond::last_tab). Don't attach conditions to + where they can't be moved. */ if (!prev_dynamic_conds.is_empty()) { - List_iterator_fast<Item_func_dynamic_cond> dyn_cond_it(prev_dynamic_conds); - if (!wrapped_select_cond) - wrapped_select_cond= dyn_cond_it++; + List_iterator<Item_func_dynamic_cond> dyn_cond_it(prev_dynamic_conds); while ((cur_wrapped_cond= dyn_cond_it++)) - wrapped_select_cond->add_pred(cur_wrapped_cond); + { + if (!wrapped_select_cond) + wrapped_select_cond= cur_wrapped_cond; + else + wrapped_select_cond->add_pred(cur_wrapped_cond); + + /* + psergey-todo: conditions have limits on how far to the right they can be + moved (Item_func_dynamic_cond::last_tab). Don't attach conditions to + where they can't be moved. + */ + if (cur_wrapped_cond->get_last_tab() == tab) + dyn_cond_it.remove(); + } } prev_dynamic_conds.concat(&cur_dynamic_conds); cur_dynamic_conds.empty(); === modified file 'sql/table.h' --- sql/table.h 2013-09-25 18:07:06 +0000 +++ sql/table.h 2013-12-19 02:50:28 +0000 @@ -1721,6 +1721,9 @@ struct TABLE_LIST table_map sj_inner_tables; /* Number of IN-compared expressions */ uint sj_in_exprs; + + //psergey-todo: add the index of last table in the join order + uint sj_strategys_join_tab; /* If this is a non-jtbm semi-join nest: corresponding subselect predicate */ Item_in_subselect *sj_subq_pred; ----- End forwarded message ----- -- BR Sergei -- Sergei Petrunia, Software Developer MariaDB | Skype: sergefp | Blog: http://s.petrunia.net/blog

1 0

[Maria-developers] MariaDB r3955 make fail; " error: ‘isfinite’ was not declared in this scope"
by darx＠sent.com 18 Dec '13

18 Dec '13

hi, upgrading a MariaDB 10/git src build from r3911 -> r3955, on x86_64 after configure, make fails with a warning & an error @, ... [ 19%] Building CXX object storage/connect/CMakeFiles/connect.dir/ha_connect.cc.o cd /usr/local/src/mariadb/bld/storage/connect && /usr/bin/g++-4.8 -DFORCE_INIT_OF_VARS -DHAVE_CONFIG_H -DHUGE_SUPPORT -DLIBXML2_SUPPORT -DLINUX -DMARIADB -DMYSQL_DYNAMIC_PLUGIN -DMYSQL_SUPPORT -DODBC_SUPPORT -DPIVOT_SUPPORT -DUBUNTU -DUNIX -DZIP_SUPPORT -Dconnect_EXPORTS -Wall -O2 -fmessage-length=0 -D_FORTIFY_SOURCE=2 -fstack-protector -march=amdfam10 -mtune=amdfam10 -std=c++11 -felide-constructors -fno-exceptions -fno-rtti -Wall -Wno-unused-parameter -fno-exceptions -fno-rtti -fpermissive -fexceptions -fPIC -O2 -g -DNDEBUG -DDBUG_OFF -DMY_PTHREAD_FASTMUTEX=1 -fPIC -I/usr/local/src/mariadb/bld/include -I/usr/include/libxml2 -I/usr/local/src/mariadb/include -I/usr/local/src/mariadb/sql -I/usr/local/src/mariadb/bld/pcre -I/usr/local/src/mariadb/pcre -I/usr/local/ssl/include -O2 -fmessage-length=0 -D_FORTIFY_SOURCE=2 -fstack-protector -march=amdfam10 -mtune=amdfam10 -Wall -Wmissing-declarations -Wno-write-strings -Wno-unused-variable -Wno-unused-value -Wno-unused-function -Wno-parentheses -o CMakeFiles/connect.dir/ha_connect.cc.o -c /usr/local/src/mariadb/storage/connect/ha_connect.cc In file included from /usr/local/src/mariadb/sql/gcalc_tools.h:21:0, from /usr/local/src/mariadb/sql/spatial.h:28, from /usr/local/src/mariadb/sql/item.h:3667, from /usr/local/src/mariadb/sql/sql_lex.h:26, from /usr/local/src/mariadb/sql/sql_class.h:463, from /usr/local/src/mariadb/storage/connect/ha_connect.cc:104: /usr/local/src/mariadb/sql/gcalc_slicescan.h:29:40: warning: invalid suffix on literal; C++11 requires a space between literal and identifier [-Wliteral-suffix] #define GCALC_DBUG_ENTER(a) DBUG_ENTER("Gcalc "a) ^ In file included from /usr/local/src/mariadb/sql/item.h:3669:0, from /usr/local/src/mariadb/sql/sql_lex.h:26, from /usr/local/src/mariadb/sql/sql_class.h:463, from /usr/local/src/mariadb/storage/connect/ha_connect.cc:104: /usr/local/src/mariadb/sql/item_func.h: In member function ‘double Item_func::check_float_overflow(double)’: /usr/local/src/mariadb/sql/item_func.h:281:26: error: ‘isfinite’ was not declared in this scope return isfinite(value) ? value : raise_float_overflow(); ^ /usr/local/src/mariadb/sql/item_func.h:281:26: note: suggested alternative: In file included from /usr/include/c++/4.8/random:38:0, from /usr/include/c++/4.8/bits/stl_algo.h:65, from /usr/include/c++/4.8/algorithm:62, from /usr/local/src/mariadb/sql/mdl.h:32, from /usr/local/src/mariadb/sql/table.h:22, from /usr/local/src/mariadb/sql/field.h:29, from /usr/local/src/mariadb/sql/unireg.h:172, from /usr/local/src/mariadb/sql/sql_class.h:24, from /usr/local/src/mariadb/storage/connect/ha_connect.cc:104: /usr/include/c++/4.8/cmath:596:5: note: ‘std::isfinite’ isfinite(_Tp __x) ^ make[2]: *** [storage/connect/CMakeFiles/connect.dir/ha_connect.cc.o] Error 1 make[2]: Leaving directory `/usr/local/src/mariadb/bld' make[1]: *** [storage/connect/CMakeFiles/connect.dir/all] Error 2 make[1]: Leaving directory `/usr/local/src/mariadb/bld' make: *** [all] Error 2

1 0

Re: [Maria-developers] metadata_lock_info plugin
by Sergey Vojtovich 13 Dec '13

13 Dec '13

Hi Kentoku, thanks for your contribution and sorry for this long delay. I finally managed to go through your code. The most important thing I'm worried about is that there is no protection of ticket lists: lists may be updated while we iterate them and it may cause server crash in concurrent environment. From what I can see MDL_context is mostly supposed to be accessed only by thread which owns this context. As such it is not protected by any locks. I believe we should use global lists instead, something like: mysql_mutex_lock(&mdl_locks.m_mutex); iterate mdl_locks.m_locks (hash of MDL_lock) { mysql_prlock_rdlock(&lock->m_rwlock); iterate lock->m_granted (list of MDL_ticket) { // store data } mysql_prlock_unlock(&lock->m_rwlock); } mysql_mutex_lock(&mdl_locks.m_mutex); A few more suggestions in no particular order: - instead of making m_tickets public I'd suggest to try to make i_s_metadata_lock_info_fill_table() a friend function (shouldn't be relevant anymore) - why check for thd->killed? (shouldn't be relevant anymore) - metadata_lock_info_lock_mode_length[] includes ending zero, and then we store e.g. "MDL_SHARED\0" whereas we should store "MDL_SHARED" I'd suggest to declare these arrays as following: static const LEX_STRING metadata_lock_info_lock_mode_str[]= { STRING_WITH_LEN("MDL_INTENTION_EXCLUSIVE"), // or C_STRING_WITH_LEN() ... }; - Roberto suggests to rename ID to THREAD_ID and I agree - Roberto suggests to add QUERY_ID column and I agree - Roberto suggests to add LOCK_DURATION column and I agree - Roberto suggests to use m_namespace_to_wait_state_name instead of metadata_lock_info_lock_names, but I tend to disagree because the former says "Waiting for...". Though I'd suggest to make array of LEX_STRING's to avoid calling strdup(). - Roberto suggests to replace DB with KEY and NAME with SUB_KEY, but I tend to disagree since in most cases it is db and table name. I'd vote for TABLE_SCHEMA and TABLE_NAME to stay in line with the rest I_S tables. Regards, Sergey On Mon, Jul 01, 2013 at 03:46:22AM +0900, kentoku wrote: > Hi Sergey, > Sorry. I think this plugin needs more locks "mdl_locks.m_mutex", > "mdl_locks.m_global_lock" and "mdl_locks.m_commit_lock". Is it right? Would > you please teach me how can I use it? > Thanks, > Kentoku > > > 2013/7/1 kentoku <kentokushiba(a)gmail.com> > > > Hi Sergey, > > > > I made new information schema plugin which named "metadata_lock_info" > > plugin. This plugin makes it possible knowing "who has metadata locks". In > > development stage, sometimes DBAs meet metadata locks when using alter > > table statement. This metadata locks are sometimes caused by GUI tools. In > > service stage, sometimes DBAs meet metadata locks when using alter table > > statement. This metadata locks are sometimes caused by long batch > > processing. In many cases, the DBAs need to judge immediately. So I made it > > for all DBAs. > > Would you please review this plugin? > > lp:~kentokushiba/maria/10.0.3-metadata_lock_info > > Note: This plugin needs a change of sql/mdl.h > > > > Thanks, > > Kentoku > >

3 15

Re: [Maria-developers] Rev 3915: MDEV-5388 - Reduce usage of LOCK_open: unused_tables
by Sergei Golubchik 12 Dec '13

12 Dec '13

Hi, Sergey! Did you benchmark the improvement you get with this patch? On Dec 05, Sergey Vojtovich wrote: > At lp:maria/10.0 > > ------------------------------------------------------------ > revno: 3915 > revision-id: svoj(a)mariadb.org-20131205084430-7rt735tbanw0i6uq > parent: svoj(a)mariadb.org-20130827121233-b9mguuxctnjlq5wd > committer: Sergey Vojtovich <svoj(a)mariadb.org> > branch nick: 10.0 > timestamp: Thu 2013-12-05 12:44:30 +0400 > message: > MDEV-5388 - Reduce usage of LOCK_open: unused_tables > > Removed unused_tables, find LRU object by timestamp instead. > === modified file 'sql/table_cache.cc' > --- a/sql/table_cache.cc 2013-08-27 12:12:33 +0000 > +++ b/sql/table_cache.cc 2013-12-05 08:44:30 +0000 > @@ -324,20 +248,44 @@ void tc_add_table(THD *thd, TABLE *table > DBUG_ASSERT(table->in_use == thd); > mysql_mutex_lock(&LOCK_open); > table->s->tdc.all_tables.push_front(table); > - tc_count++; > /* If we have too many TABLE instances around, try to get rid of them */ > - if (tc_count > tc_size && unused_tables) > + if (tc_count == tc_size) I'd rather put (tc_count >= tc_size), looks safer. > { > - TABLE *purge_table= unused_tables; > - tc_remove_table(purge_table); > - mysql_rwlock_rdlock(&LOCK_flush); > + TDC_iterator tdc_it; > mysql_mutex_unlock(&LOCK_open); > - intern_close_table(purge_table); > - mysql_rwlock_unlock(&LOCK_flush); > - check_unused(thd); > + > + tdc_it.init(); > + mysql_mutex_lock(&LOCK_open); > + if (tc_count == tc_size) and here > + { > + TABLE *purge_table= 0; > + TABLE_SHARE *share; I think a status variable for this would be nice. So that one could make an informed decision about the desired size of the cache. > + while ((share= tdc_it.next())) > + { > + TABLE_SHARE::TABLE_list::Iterator it(share->tdc.free_tables); > + TABLE *entry; > + while ((entry= it++)) > + if (!purge_table || entry->tc_time < purge_table->tc_time) > + purge_table= entry; > + } > + if (purge_table) > + { > + tdc_it.deinit(); > + purge_table->s->tdc.free_tables.remove(purge_table); > + purge_table->s->tdc.all_tables.remove(purge_table); > + mysql_rwlock_rdlock(&LOCK_flush); > + mysql_mutex_unlock(&LOCK_open); > + intern_close_table(purge_table); > + mysql_rwlock_unlock(&LOCK_flush); > + check_unused(thd); > + return; > + } > + } > + tdc_it.deinit(); Okay, so you only purge one table at time? Since purge is kind of an expensive operation, it's better to amortize the cost of it by purging many tables at once. > } > - else > - mysql_mutex_unlock(&LOCK_open); > + /* Nothing to evict, increment tc_count. */ > + tc_count++; > + mysql_mutex_unlock(&LOCK_open); > } > 2 Regards, Sergei

2 6

Re: [Maria-developers] Rev 3914: MDEV-4956 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.used_tables
by Sergei Golubchik 12 Dec '13

12 Dec '13

Hi, Sergey! On Dec 10, Sergey Vojtovich wrote: > At lp:maria/10.0 > > ------------------------------------------------------------ > revno: 3914 > revision-id: svoj(a)mariadb.org-20131210150036-0hwh1gy4z3jalwsg > parent: bar(a)mnogosearch.org-20131203101253-mbz05d9jtkb3iycp > committer: Sergey Vojtovich <svoj(a)mariadb.org> > branch nick: 10.0-mdev4956 > timestamp: Tue 2013-12-10 19:00:36 +0400 > message: > MDEV-4956 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.used_tables > > - tc_acquire_table and tc_release_table do not access > TABLE_SHARE::tdc.used_tables anymore > - in tc_acquire_table(): release LOCK_tdc after we relase LOCK_open > (saves a few CPU cycles in critical section) > - in tc_release_table(): if we reached table cache threshold, evict > to-be-released table without moving it to unused_tables. unused_tables > must be empty at this point. Looks ok to me Regards, Sergei

1 0

[Maria-developers] Issue with index condition pushdown being used with no end_range set
by Zardosht Kasheff 12 Dec '13

12 Dec '13

Hello all, I've noticed a scenario with index condition pushdown (ICP) in MariaDB that leads to really bad performance in TokuDB. I don't know if it is a bug in ICP or if TokuDB is misusing/misunderstanding the API. Here is the problem. Suppose we run the following query on the following schema: SELECT * FROM foo WHERE col1 = '7' ORDER BY b desc | foo | CREATE TABLE `foo` ( `id` int(11) NOT NULL AUTO_INCREMENT, `col1` varchar(100) COLLATE utf8_unicode_ci NOT NULL, `b` int(11) DEFAULT NULL, `c` int(11) DEFAULT NULL, `d` int(11) DEFAULT NULL, PRIMARY KEY (`id`), CLUSTERING KEY `col1_2` (`col1`,`b`) ) ENGINE=TokuDB AUTO_INCREMENT=8001 DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci ROW_FORMAT=TOKUDB_ZLIB | The output of explain is: MariaDB [test]> explain SELECT * FROM foo WHERE col1 = '7' ORDER BY b desc; +------+-------------+-------+------+---------------+--------+---------+-------+------+-------------+ | id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra | +------+-------------+-------+------+---------------+--------+---------+-------+------+-------------+ | 1 | SIMPLE | foo | ref | col1_2 | col1_2 | 302 | const | 1320 | Using where | +------+-------------+-------+------+---------------+--------+---------+-------+------+-------------+ 1 row in set (2.32 sec) The query is doing a reverse range scan, as it should. We get handler::idx_cond_push called, but end_range is not set. As a result, TokuDB thinks it can use index condition pushdown to filter rows. As we do this and get to the end, because end_range is not set, we never get a result of ICP_OUT_OF_RANGE, and always get a result of ICP_NO_MATCH. So, when we go to retrieve that first row past the end of the range, the row that will tell MySQL it should stop searching, TokuDB never finds a row and never gets ICP_NO_MATCH. It scans to the beginning of the index (because it is running in reverse order), getting ICP_NO_MATCH for every row it encounters. Although my index is clustering, I've seen this with a normal key as well. If col1 is an int instead of the funky varchar, handler::idx_cond_push is never called, so this problem does not exist. It seems to me that if end_range is not set and we cannot reliably learn when we go out of range, we should not have handler::idx_cond_push called, otherwise we can get bad performance such as the example above. Thoughts? Thanks --Zardosht

1 1

Re: [Maria-developers] [Commits] Rev 3916: MDEV-5403 - Reduce usage of LOCK_open: tc_count
by Sergei Golubchik 10 Dec '13

10 Dec '13

Hi, Sergey! Not a review, only a comment on TDC_atomic part: 1. This is perfectly generally useful, not TDC specific at all. I'd consider moving it to my_atomic.h under #ifdef __cplusplus 2. These are supposed to be trivial inlined functions. I don't think you should use inheritance and virtual methods here. Everything should be in the template. I understand that function names (add64/add32/etc) are a bit tricky. Two approaches: pass them as template parameters (like in template <typename T, ADD, CAS, ...>) or - perhaps, if you put this into my_atomic.h you'll be able to generate the necessary code without using my_atomic_add32/etc ? Regards, Sergei

2 1

Re: [Maria-developers] [svoj@mariadb.org: [Commits] Rev 3807: MDEV-4956 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.used_tables in lp:maria/10.0]
by Sergei Golubchik 10 Dec '13

10 Dec '13

Hi, Sergey! Looks pretty much ok. See few comments/questions below. On Oct 24, Sergey Vojtovich wrote: > ------------------------------------------------------------ > revno: 3807 > revision-id: svoj(a)mariadb.org-20130827121233-xh1uyhgfwbhedqyf > parent: jplindst(a)mariadb.org-20130823060357-pww92qxla7o8iir7 > committer: Sergey Vojtovich <svoj(a)mariadb.org> > branch nick: 10.0 > timestamp: Tue 2013-08-27 16:12:33 +0400 > message: > MDEV-4956 - Reduce usage of LOCK_open: TABLE_SHARE::tdc.used_tables > > - tc_acquire_table and tc_release_table do not access > TABLE_SHARE::tdc.used_tables anymore ok > - in tc_acquire_table(): release LOCK_tdc after we relase LOCK_open > (saves a few CPU cycles in critical section) This is a bit counter-intuitive. Your change is to hold the lock more than necessary. Does it give a measurable performance improvement? > - in tc_release_table(): if we reached table cache threshold, evict > to-be-released table without moving it to unused_tables. unused_tables > must be empty at this point (added assertion). Why unused_tables must be empty at this point? > === modified file 'sql/sql_base.cc' > --- a/sql/sql_base.cc 2013-08-14 08:48:50 +0000 > +++ b/sql/sql_base.cc 2013-08-27 12:12:33 +0000 > @@ -370,7 +372,7 @@ void free_io_cache(TABLE *table) > > void kill_delayed_threads_for_table(TABLE_SHARE *share) > { > - TABLE_SHARE::TABLE_list::Iterator it(share->tdc.used_tables); > + TABLE_SHARE::All_share_tables_list::Iterator it(share->tdc.all_tables); In this function, I'd suggest to add some flag to be able to skip the mutex and the loop altogether - as in the absolute majority of cases there will be no delayed insert thread to kill. Either use a global flag to know if there's a delayed thread running, or create a per-share flag. > TABLE *tab; > Regards, Sergei

2 4

[Maria-developers] Reviews of parallel replications
by Michael Widenius 10 Dec '13

10 Dec '13

Hi! Now continuing with reviews of the parallel replication code. Will take one patch at a time with a target to have everything reviewed this week. Review of revno 3678: Parallel replication: error handling. ------------------------------------------------------------ revno: 3678 committer: knielsen(a)knielsen-hq.org branch nick: work-10.0-mdev4506 timestamp: Mon 2013-10-14 15:28:16 +0200 message: MDEV-4506: Parallel replication: error handling. Add an error code to the wait_for_commit facility. Now, when a transaction fails, it can signal the error to any subsequent transaction that is waiting for it to commit. The waiting transactions then receive the error code back from wait_for_prior_commit() and can handle the error appropriately. Also fix one race that could cause crash if @@slave_parallel_threads were changed several times quickly in succession. <cut> === modified file 'sql/handler.cc' --- sql/handler.cc 2013-10-13 21:24:05 +0000 +++ sql/handler.cc 2013-10-14 13:28:16 +0000 @@ -1458,10 +1458,11 @@ int ha_commit_one_phase(THD *thd, bool a transaction.all.ha_list, see why in trans_register_ha()). */ bool is_real_trans=all || thd->transaction.all.ha_list == 0; + int res; DBUG_ENTER("ha_commit_one_phase"); - if (is_real_trans) - thd->wait_for_prior_commit(); - int res= commit_one_phase_2(thd, all, trans, is_real_trans); + if (is_real_trans && (res= thd->wait_for_prior_commit())) + DBUG_RETURN(res); + res= commit_one_phase_2(thd, all, trans, is_real_trans); DBUG_RETURN(res); Another way to write the above is: if (is_real_trans && !(res= thd->wait_for_prior_commit())) res= commit_one_phase_2(thd, all, trans, is_real_trans); DBUG_RETURN(res); <cut> I was checking how ha_commit_one_phase() was used. In ha_commit_trans() or trans_xa_commit(), if ha_commit_one_phase() fails, it will not run rollback. Is this right? I would have assumed that on failure, ha_commit_one_phase() should run ha_rollback_trans() before returning. === modified file 'sql/log.cc' @@ -7844,7 +7844,8 @@ int TC_LOG_MMAP::log_and_order(THD *thd, mysql_mutex_unlock(&LOCK_prepare_ordered); } - thd->wait_for_prior_commit(); + if (thd->wait_for_prior_commit()) + return 0; What does the return 0 mean here? Is it an error? Is thd->is_fatal_error assumed to be set if this happens? A comment here would not be totally wrong... @@ -5633,7 +5634,7 @@ wait_for_commit::~wait_for_commit() void -wait_for_commit::wakeup() +wait_for_commit::wakeup(int wakeup_error) { /* We signal each waiter on their own condition and mutex (rather than using @@ -5649,6 +5650,7 @@ wait_for_commit::wakeup() */ mysql_mutex_lock(&LOCK_wait_commit); waiting_for_commit= false; + this->wakeup_error= wakeup_error; Consider adding an assert that 'this->wakeup_error' is 0. Good way to ensure we don't accidently call this function twice... (as things would be bad if we first would call it with an error and then with 0) mysql_mutex_unlock(&LOCK_wait_commit); mysql_cond_signal(&COND_wait_commit); } Regards, Monty

1 0