- developers - lists.mariadb.org

Re: [Maria-developers] [MariaDB/server] Add new scheduling algorithm for reducing tail latencies (#245)
by Kristian Nielsen 12 Oct '16

12 Oct '16

Jiamin Huang <notifications(a)github.com> writes: > This branch introduces a new scheduling algorithm > (Variance-Aware-Transaction-Scheduling, VATS) for the record lock manager > of InnoDB and XtraDB. Instead of using First-Come-First-Served (FCFS), the > newly introduced algorithm uses an Eldest-Transaction-First (ETF) > heuristic, which prefers older transactions over new ones. It could be interesting to extend this to also understand commit order for in-order parallel replication. For in-order parallel replication, the commit order of transactions is fixed from the start. Suppose transactions Tm and Tn are both requesting the same record lock, and that Tm goes before Tn. If Tn gets the lock, it will be immediately killed since Tm must be allowed to go first. But there is no guarantee that Tm will be the older transaction. So prefering Tm over Tn here based on the commit order can potentially avoid some transaction rollback/retry. The function thd_deadlock_victim_preference() seems appropriate for overriding the age-of-transaction preference. If this function returns a prefered deadlock victim, give the lock to the other transaction. If not, the VATS algorithm can be applied. Especially in aggressive parallel replication, there can be a lot of conflicting transactions (one user reported 20% of transactions conflicting, while still getting good speedup from parallel replication). So it might be a worthwhile thing to do. - Kristian.

1 0

Re: [Maria-developers] Optimistic parallel replication for RocksDB
by Kristian Nielsen 11 Oct '16

11 Oct '16

Yoshinori Matsunobu <yoshinori(a)fb.com> writes: > About transaction ids, it's not visible from MyRocks yet. We're currently working on > RocksDB to add an API to get transaction id, and making it available via MyRocks. Ok, I see. Optimistic parallel replication needs the ability to somehow find the THD that is holding the row lock that is blocking another THD. If we have T1 followed by T2, T2 will only commit after T1 has. So if there is a conflicting row lock, we need a way to identify T2 so that the conflict can be resolved. Lock wait timeout is not sufficient here because of the requirement of in-order commit. > Rows are normally released at transaction commit or rollback, but there are some exceptions. > - Auto-increment id allocation is implemented as std::atomic<longlong> and > the lock is released earlier than statement/transaction, like > InnoDB. I hope this doesn't matter for > parallel replication, since auto-inc ids are always given on slaves > (either RBR image, or insert_id with SBR). Agree, it does not matter, MyRocks just should not report these lock conflicts with thd_rpl_deadlock_check() (and since this is using a different mechanism, there is no reason it would). > - MyRocks has data dictionary > (https://github.com/facebook/mysql-5.6/wiki/MyRocks-data-dictionary-format) > and data dictionary operations' transaction scope is different from > applications'. For example, internal index > id allocation is done (and committed) immediately. There is no SQL > statements to directly manipulate data dictionary, > so I assume this won't matter for replication either. Agree, it shouldn't. Optimistic parallel replication handles DDL pessimistically anyway - DDL is not run in parallel with any other statements. Thanks, - Kristian.

1 0

[Maria-developers] Optimistic parallel replication for RocksDB
by Kristian Nielsen 10 Oct '16

10 Oct '16

Sergey, Yoshinori, it was great talking to you about MyRocks in Amsterdam. I took a first look at how to extend MyRocks to work with optimistic parallel replication. It looks conceptually quite simple. Sergey, I understand you have more pressing priorities right now (like getting a tree to build :), so let us revisit this in more detail when you get to it. It looks like the fix is conceptually as simple as this patch, which calls thd_rpl_deadlock_check() whenever a transaction is blocked on a row lock: ----------------------------------------------------------------------- diff --git a/utilities/transactions/transaction_lock_mgr.cc b/utilities/transactions/transaction_lock_mgr.cc index 28e8598..5ff291f 100644 --- a/utilities/transactions/transaction_lock_mgr.cc +++ b/utilities/transactions/transaction_lock_mgr.cc @@ -317,6 +317,8 @@ Status TransactionLockMgr::AcquireWithTimeout(LockMap* lock_map, return result; } +extern "C" int thd_rpl_deadlock_check(MYSQL_THD thd, MYSQL_THD other_thd); + // Try to lock this key after we have acquired the mutex. // Sets *expire_time to the expiration time in microseconds // or 0 if no expiration. @@ -340,6 +342,9 @@ Status TransactionLockMgr::AcquireLocked(LockMap* lock_map, lock_info.expiration_time = txn_lock_info.expiration_time; // lock_cnt does not change } else { + THD *blocked_thd = getTHD(txn_lock_info.txn_id); + THD *bloking_thd = getTHD(lock_info.txn_id); + thd_rpl_deadlock_check(blocked_thd, blocking_thd); result = Status::TimedOut(Status::SubCode::kLockTimeout); } } ----------------------------------------------------------------------- A real patch will need some plumbing to put the code in the right place and have the right information available. Ie. probably the thd_rpl_deadlock_check() call will go into an overridden virtual method in ha_rocksdb.cc. I also did not check if/how one can get from txn_id to THD (what is called getTHD() above), I assume it can be implemented reasonably easy if it is not already there? Hints will be appreciated here as I am new to the MyRocks and RocksDB codebases. When are row locks released? I am interested in whether row locks can be released earlier than at transaction commit time. If so, the simple patch above will give false positives, and it might be worth it to investigate ways to not report locks that are released earlier than commit. Eg. in InnoDB, auto-increment locks are released earlier than commit, and thus are not reported. Once something like this is in place, I think optimistic parallel replication should work. In case of a conflict between transactions T1 and T2, thd_rpl_deadlock_check(T1, T2) will be called and will cause T2 to be killed so that T1 can proceed and T2 be re-tried afterwards. So things look good now; let us revisit this when there is a tree to work on. - Kristian.

1 0

Re: [Maria-developers] 8cd65cd: MDEV-9143 JSON_xxx functions.
by Sergei Golubchik 08 Oct '16

08 Oct '16

Hi, Alexey! On Oct 08, Alexey Botchkov wrote: > > Other comments, that you didn't reply to, does it mean you agree with > > them (like adding tests with values that need json-escaping)? Okay. > > Right, it means that I just agreed with the comment. > > I don't think we're going to need more unittest files so yep, will > move it to strings/ Will check for missing functions from MySQL, try > running MySQL tests with us, then push. > > After that we'll still have one inconsistency with MySQL - that they > have that JSON type for fields and values. JSON-values let the user > to compare values as 'json'-s. Not sure anybody ever uses it. But > JSON fields are probably good to import from MySQL, maybe in read-only > mode. Just to let people switch to MariaDB easily over these JSON > tables. Yes... I think, out of missing features, JSON_TABLE is the most interesting, but also the comlexity is a big unknown. The second one would be GeoJSON (and it's fairly predictable), then goes JSON comparison and JSON data type. Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] 8cd65cd: MDEV-9143 JSON_xxx functions.
by Sergei Golubchik 08 Oct '16

08 Oct '16

Hi, Alexey! On Sep 21, Alexey Botchkov wrote: > > > > + In the worst case one character from the 'a' string > > > + turns into '\uXXXX\uXXXX' which is 12. > > > how comes? add an example, please. Like "for example, character x'1234' > > in the charset A becomes '\u1234\u5678' if JSON string is in the charset B" > > For instance the 'SMILING FACE WITH SUNGLASSES' character, > which is utf32(0001F60E) should be represented as \uD83D\uDE0E. I hope you put this in the comment in the code... Other comments, that you didn't reply to, does it mean you agree with them (like adding tests with values that need json-escaping)? Okay. Thanks for unit tests. There weren't many, but if you think they cover all the library functionality, then it's fine. Just one thought - do you expect any more json_lib related test *files* in the future? If not, you could move json_lib-t.cc into unittest/strings. And why the test is .cc? The file looked pure C to me. Anyway, I think it's ok to push after resolving these minor issues. Thanks! Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] 1c9da8d: MDEV-9312: storage engine not enforced during galera cluster replication
by Jan Lindström 29 Sep '16

29 Sep '16

Hi Nirbhay, This looks ok but one question (no need to change now): On Wed, Sep 28, 2016 at 7:36 PM, Nirbhay Choubey <nirbhay(a)mariadb.com> wrote: > > + Since some wsrep threads (THDs) are create before plugins are > + initialized, LOCK_plugin mutex needs to be initialized here. > + */ > Is there some fundamental reason why we can't create wsrep threads after other plugins and related global system variables (that are really needed for wsrep) are initialized ? R: Jan

2 1

Re: [Maria-developers] [Commits] a8162d4: MDEV-9312: storage engine not enforced during galera cluster replication
by Sergei Golubchik 28 Sep '16

28 Sep '16

Hi, Nirbhay! I don't understand, why do you need to create a dummy plugin here? On Sep 27, Nirbhay Choubey wrote: > revision-id: a8162d4a8737cff67889390fad0153acc175391d (mariadb-10.1.17-22-ga8162d4) > parent(s): 6a6b253a6ecbd4d3dd254044d12ec64475453275 > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-09-27 09:03:26 -0400 > message: > > MDEV-9312: storage engine not enforced during galera cluster replication > > Perform a post initialization of plugin-related variables > of wsrep threads after their global counterparts have been > initialized. ... > +#ifdef WITH_WSREP > + > +/* > + Placeholder for global_system_variables.table_plugin required during > + initialization of startup wsrep threads. > +*/ > +static st_plugin_int *wsrep_dummy_plugin; > + > +/* > + Initialize wsrep_dummy_plugin and assign it to > + global_system_variables.table_plugin. > +*/ > +void wsrep_plugins_pre_init() > +{ > + wsrep_dummy_plugin= > + (st_plugin_int *) my_malloc(sizeof(st_plugin_int), MYF(0)); > + wsrep_dummy_plugin->state= PLUGIN_IS_DISABLED; > + global_system_variables.table_plugin= plugin_int_to_ref(wsrep_dummy_plugin); > +} > + Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

2 2

Re: [Maria-developers] 12ffe70: MDEV-9416: MariaDB galera got signal 11 when altering table add unique index
by Sergei Golubchik 28 Sep '16

28 Sep '16

Hi, Nirbhay! On Sep 24, Nirbhay Choubey wrote: > revision-id: 12ffe70831c9c69c3d6ef83431443696455c332b (mariadb-10.1.17-23-g12ffe70) > parent(s): 8ffded0a78c0a4912b32acac4c3f58f04c3bcd87 > author: Nirbhay Choubey > committer: Nirbhay Choubey > timestamp: 2016-09-24 00:27:38 -0400 > message: > > MDEV-9416: MariaDB galera got signal 11 when altering table add unique index > > When a BF thread attempts to abort a victim thread's transaction, > the victim thread is not locked and thus its not safe to rely on > its data structures like htons registered for the trx. > > So, instead of getting the registered htons from victim, innodb's > hton can be looked up directly from installed_htons[] and used to > abort the transaction. (Same technique is used in older versions) > > diff --git a/sql/handler.cc b/sql/handler.cc > index 4e4c8fa..6a32fec 100644 > --- a/sql/handler.cc > +++ b/sql/handler.cc > @@ -6108,29 +6108,16 @@ int ha_abort_transaction(THD *bf_thd, THD *victim_thd, my_bool signal) > DBUG_RETURN(0); > } > > - /* Try statement transaction if standard one is not set. */ > - THD_TRANS *trans= (victim_thd->transaction.all.ha_list) ? > - &victim_thd->transaction.all : &victim_thd->transaction.stmt; > - > - Ha_trx_info *ha_info= trans->ha_list, *ha_info_next; > - > - for (; ha_info; ha_info= ha_info_next) > + handlerton *hton= installed_htons[DB_TYPE_INNODB]; > + if (hton && hton->abort_transaction) > { > - handlerton *hton= ha_info->ht(); > - if (!hton->abort_transaction) > - { > - /* Skip warning for binlog & wsrep. */ > - if (hton->db_type != DB_TYPE_BINLOG && hton != wsrep_hton) > - { > - WSREP_WARN("Cannot abort transaction."); > - } > - } > - else > - { > - hton->abort_transaction(hton, bf_thd, victim_thd, signal); > - } > - ha_info_next= ha_info->next(); > + hton->abort_transaction(hton, bf_thd, victim_thd, signal); Okay, but please add a comment that it's safe to abort from one thread (bf_thd) the transaction, running in another thread (victim_thd), because innodb's lock_sys and trx_mutex guarantee the necessary protection. And that it's not safe to access victim_thd->transaction, because it's not protected from concurrent accesses. And it's an overkill to take LOCK_plugin and iterate the whole installed_htons[] array every time. Then ok to push > } > + else > + { > + WSREP_WARN("Cannot abort InnoDB transaction"); > + } > + > DBUG_RETURN(0); > } Regards, Sergei Chief Architect MariaDB and security(a)mariadb.org

1 0

Re: [Maria-developers] [Commits] f2aea43: MDEV-10649: Optimizer sometimes use "index" instead of "range" access for UPDATE
by Jan Lindström 28 Sep '16

28 Sep '16

On Tue, Sep 6, 2016 at 8:37 PM, Sergei Petrunia <psergey(a)askmonty.org> wrote: > revision-id: f2aea435df7e92fcf8f09f8f6c160161168c5bed > parent(s): a14f61ef749ad9f9ab2b0f5badf6754ba7443c9e > committer: Sergei Petrunia > branch nick: 10.0 > timestamp: 2016-09-06 20:37:21 +0300 > message: > > MDEV-10649: Optimizer sometimes use "index" instead of "range" access for > UPDATE > > (XtraDB variant only, for now) > > Re-opening a TABLE object (after e.g. FLUSH TABLES or open table cache > eviction) causes ha_innobase to call > dict_stats_update(DICT_STATS_FETCH_ONLY_IF_NOT_IN_MEMORY). > > Inside this call, the following is done: > dict_stats_empty_table(table); > dict_stats_copy(table, t); > > On the other hand, commands like UPDATE make this call to get the "rows in > table" statistics in table->stats.records: > > ha_innobase->info(HA_STATUS_VARIABLE|HA_STATUS_NO_LOCK) > > note the HA_STATUS_NO_LOCK parameter. It means, no locks are taken by > ::info() If the ::info() call happens between dict_stats_empty_table > and dict_stats_copy calls, the UPDATE's optimizer will get an estimate > of table->stats.records=1, which causes it to pick a full table scan, > which in turn will take a lot of row locks and cause other bad > consequences. > > --- > storage/xtradb/dict/dict0stats.cc | 29 +++++++++++++++++++---------- > 1 file changed, 19 insertions(+), 10 deletions(-) > > diff --git a/storage/xtradb/dict/dict0stats.cc b/storage/xtradb/dict/ > dict0stats.cc > index b073398..a4aa436 100644 > --- a/storage/xtradb/dict/dict0stats.cc > +++ b/storage/xtradb/dict/dict0stats.cc > @@ -673,7 +673,10 @@ Write all zeros (or 1 where it makes sense) into a > table and its indexes' > dict_stats_copy( > /*============*/ > dict_table_t* dst, /*!< in/out: destination table */ > - const dict_table_t* src) /*!< in: source table */ > + const dict_table_t* src, /*!< in: source table */ > + bool reset_ignored_indexes) /*!< in: if true, set ignored > indexes > + to have the same statistics > as if > + the table was empty */ > { > dst->stats_last_recalc = src->stats_last_recalc; > dst->stat_n_rows = src->stat_n_rows; > @@ -692,7 +695,16 @@ Write all zeros (or 1 where it makes sense) into a > table and its indexes' > && (src_idx = dict_table_get_next_index(src_idx)))) { > > if (dict_stats_should_ignore_index(dst_idx)) { > - continue; > + if (reset_ignored_indexes) { > + /* Reset index statistics for all ignored > indexes, > + unless they are FT indexes (these have no > statistics)*/ > + if (dst_idx->type & DICT_FTS) { > + continue; > + } > + dict_stats_empty_index(dst_idx); > Does this really help? Yes, we hold dict_sys mutex here so dict_stats_empty_index here is safe for all readers using same mutex. However, as you pointed up above, info() uses no locking method. Thus, we really do not know what it will return, all values before dict_stats_copy(), some of the indexes with new stats, all indexes with new stats. > > @@ -3240,13 +3252,10 @@ N*AVG(Ui). In each call it searches for the > currently fetched index into > > dict_table_stats_lock(table, RW_X_LATCH); > > - /* Initialize all stats to dummy values before > - copying because dict_stats_table_clone_create() > does > - skip corrupted indexes so our dummy object 't' may > - have less indexes than the real object 'table'. */ > - dict_stats_empty_table(table); > - > - dict_stats_copy(table, t); > + /* Pass reset_ignored_indexes=true as parameter > + to dict_stats_copy. This will cause statictics > + for corrupted indexes to be set to empty values */ > + dict_stats_copy(table, t, true); > This is better solution than the original but as noted above, is this enough ? R: Jan

2 2

[Maria-developers] Review for MDEV-6112 multiple triggers per table
by Alexander Barkov 28 Sep '16

28 Sep '16

Hello Monty. I checked your patch for MDEV-6112 multiple triggers per table. Here are some quick notes. I'll send a detailed review separately. 1. It would be nice to push refactoring changes in separate patch: a. do sql_mode_t refactoring b. "trg_idx -> Trigger *" refactoring c. opt_debugging related things 2. Why not to reuse the FOLLOWING_SYM and PRECEDING_SYM keywords instead of adding new ones FOLLOWS_SYM, PRECEDES_SYM ? (this is a non-standard syntax anyway) 3. I'd prefer PRECEDES/FOLLOWS to go before FOR EACH ROW. "FOR EACH ROW stmt" is a kind of single clause responsible for the action. It's very confusing to have PRECEDES/FOLLOWS characteristics in between "FOR EACH ROW" and stmt. 4. You have a lot of changes related to trim_whitespace() where you declare an unused "prefix_removed" variable. It would be nice to avoid declaring it. Possible ways: a. Perhaps the new function version can be defined like this: size_t trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str); and make it return "prefix length removed", instead of adding a new parameter. b. Another option is just to overload it: extern void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str, uint *prefix_removed); inline void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str) { uint not_used; trim_whitespace(cs, str, &not_used); } 5. The "CREATED" field is not populated well. I'm getting strange values like 1970-04-24 09:35:00.30. Btw, his query returns a correct value: SELECT FROM_UNIXTIME(UNIX_TIMESTAMP(CREATED)*100) FROM INFORMATION_SCHEMA.triggers; You must have forgotten to multiply or delete to 100 somewhere. 6. In replication tests it's better to make sure that CREATED is safely replicated (instead of changing it to #). By the way, in other tests it's also a good idea not to use # for CREATED. (see the previous problem) You can use this: SET TIMESTAMP=UNIX_TIMESTAMP('2001-01-01 10:20:30'); ... SET TIMESTAM=DEFAULT; 7. I suggest to move this code into a method, say find_trigger(): + if (table->triggers) + { + Trigger *t; + for (t= table->triggers->get_trigger(TRG_EVENT_INSERT, + TRG_ACTION_BEFORE); + t; + t= t->next) + if (t->is_fields_updated_in_trigger(&full_part_field_set)) + DBUG_RETURN(false); + } It's repeated at least two times. 8. The following structure fields are defined two times: enum trigger_order_type ordering_clause; LEX_STRING anchor_trigger_name; The first time for st_trg_chistics. The second time for %union in sql_yacc.yy Note, there will be the third time in sql_yacc_ora.yy. Please define a structure in sql_lex.h instead: struct st_trg_execution_order_chistics { /** FOLLOWS or PRECEDES as specified in the CREATE TRIGGER statement. */ enum trigger_order_type ordering_clause; /** Trigger name referenced in the FOLLOWS/PRECEDES clause of the CREATE TRIGGER statement. */ LEX_STRING anchor_trigger_name; }; and reuse it in here: struct st_trg_chirstics: public st_trg_execution_order_chistics { ... }; and in here: %union { ... st_trg_execition_order_chistics trg_execution_order_chirstics; ... }; Please also rename trg_characteristics to trg_execution_order_chistics, because "trg_characteristics" is more suitable to something having type st_trg_chirstics rather than st_trg_execution_order_chistics. Greetings.

1 1