- developers - lists.mariadb.org

[Maria-developers] Review of MDEV-4506, parallel replication, part 1
by Michael Widenius 26 Sep '13

26 Sep '13

Hi! Here is the first part of the review. I have 1/2 of one file left (rpl_parallel.cc) to review but I wanted you to have a chance to read what I have done so far. I plan to finish the review tomorrow and start working on the code during the weekend. >>>>> "Kristian" == Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: Kristian> Hi Monty, Kristian> Here is a copy of the status mail I sent earlier. The status is mostly still Kristian> current. Kristian> The code is in this tree: Kristian> lp:~maria-captains/maria/10.0-knielsen Kristian> In that tree, you can extract the parallel replication code as a single patch Kristian> with a command like this: Kristian> bzr diff -rrevid:knielsen@knielsen-hq.org-20130608103621-g91eaielv4n22aha Kristian> In the forwarded mail, I describe getting a 4.5 times speedup on a quad-core Kristian> laptop, so that's quite promising. === modified file 'mysql-test/r/mysqld--help.result' --- mysql-test/r/mysqld--help.result 2013-05-28 11:28:31 +0000 +++ mysql-test/r/mysqld--help.result 2013-08-29 08:18:22 +0000 <cut> @@ -783,6 +794,11 @@ --slave-net-timeout=# Number of seconds to wait for more data from any master/slave connection before aborting the read + --slave-parallel-threads=# + If non-zero, number of threads to spawn to apply in + parallel events on the slave that were group-committed on + the master or were logged with GTID in different + replication domains. What happens if --slave-parallel-threads is 0 ? Does this mean that there will be no parallel threads? Is this the same as 1 ? If same as 1, why not have the option to take values starting from 1 ? IRC: K> yes, if --slave-parallel-threads is 0, then the feature is disabled. The old code is used, and the SQL thread applies the events itself K> if --slave-parallel-threads is > 0, then a pool of threads are spawned. The SQL threads do not apply events themselves, they just put it in queues for the parallel threads to apply M> do we ever need to do this with only one thread? K> (we might want to give an error if user tries to do this, as it does not make much sense) M> So why not have the value from 1 to N M> where 1 is old code and > 1 is using new code ? K> That might make sense === modified file 'mysql-test/suite/perfschema/r/dml_setup_instruments.result' --- mysql-test/suite/perfschema/r/dml_setup_instruments.result 2013-03-26 09:35:34 +0000 +++ mysql-test/suite/perfschema/r/dml_setup_instruments.result 2013-08-29 08:18:22 +0000 @@ -38,14 +38,14 @@ order by name limit 10; NAME ENABLED TIMED wait/synch/cond/sql/COND_flush_thread_cache YES YES wait/synch/cond/sql/COND_manager YES YES +wait/synch/cond/sql/COND_parallel_entry YES YES +wait/synch/cond/sql/COND_prepare_ordered YES YES wait/synch/cond/sql/COND_queue_state YES YES wait/synch/cond/sql/COND_rpl_status YES YES +wait/synch/cond/sql/COND_rpl_thread YES YES +wait/synch/cond/sql/COND_rpl_thread_pool YES YES wait/synch/cond/sql/COND_server_started YES YES wait/synch/cond/sql/COND_thread_cache YES YES -wait/synch/cond/sql/COND_thread_count YES YES -wait/synch/cond/sql/Delayed_insert::cond YES YES -wait/synch/cond/sql/Delayed_insert::cond_client YES YES -wait/synch/cond/sql/Event_scheduler::COND_state YES YES Any idea why the above was deleted? Don't see anything in your patch that could affect that. <cut> === added file 'mysql-test/suite/rpl/t/rpl_parallel.test' --- mysql-test/suite/rpl/t/rpl_parallel.test 1970-01-01 00:00:00 +0000 +++ mysql-test/suite/rpl/t/rpl_parallel.test 2013-08-29 08:18:22 +0000 I didn't find any test results for rpl_parallel.test or rpl_paralell2 in your tree. Did you forget to commit these? I was also missing some comment in these files what they was testing. === modified file 'sql/log.cc' --- sql/log.cc 2013-06-06 15:51:28 +0000 +++ sql/log.cc 2013-08-29 08:18:22 +0000 @@ -6541,44 +6543,201 @@ MYSQL_BIN_LOG::write_transaction_to_binl } Could you please document in detail the arguments and return value for the following function. - I spent 3 hours to try to understand this function in detail and having a full description of the arguments would have helped a bit. bool -MYSQL_BIN_LOG::write_transaction_to_binlog_events(group_commit_entry *entry) +MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *entry, + wait_for_commit *wfc) { + group_commit_entry *orig_queue; + wait_for_commit *list, *cur, *last; + /* To facilitate group commit for the binlog, we first queue up ourselves in the group commit queue. Then the first thread to enter the queue waits for the LOCK_log mutex, and commits for everyone in the queue once it gets the lock. Any other threads in the queue just wait for the first one to finish the commit and wake them up. Do you still use LOCK_log for this? It's a bit confusing to mention LOCK_log here when you take the LOCK_prepare_ordered in the function. If LOCK_log is required, shouldn't there be an assert for it in this function? + + To support in-order parallel replication with group commit, after we add + some transaction to the queue, we check if there were other transactions + already prepared to commit but just waiting for the first one to commit. + If so, we add those to the queue as well, transitively for all waiters. */ entry->thd->clear_wakeup_ready(); mysql_mutex_lock(&LOCK_prepare_ordered); <cut> + orig_queue= group_commit_queue; + + /* + Iteratively process everything added to the queue, looking for waiters, + and their waiters, and so on. If a waiter is ready to commit, we + immediately add it to the queue; if not we just wake it up. + + This would be natural to do with recursion, but we want to avoid + potentially unbounded recursion blowing the C stack, so we use the list + approach instead. Add here: cur->next_subsequent_commit is at this point an empty list. 'last' will always point to the last element of the list. We use this list to simulate recursion. I did find it confusing that here you threat next_subsequent_commit as a way to avoid recursion, but you use this list for other things in register_wait_for_prior_commit(). Suggestion: - If you want to reuse memory for the list, please use an union over next_subsequent_commit and have two names for it, so that you don't in the code use the same name for two different list. + */ + list= wfc; + cur= list; + last= list; + for (;;) + { + /* Add the entry to the group commit queue. */ + entry->next= group_commit_queue; + group_commit_queue= entry; + + if (entry->cache_mngr->using_xa) + { + DEBUG_SYNC(entry->thd, "commit_before_prepare_ordered"); + run_prepare_ordered(entry->thd, entry->all); + DEBUG_SYNC(entry->thd, "commit_after_prepare_ordered"); + } + + if (!cur) + break; // Can happen if initial entry has no wait_for_commit + Add a comment what the following if is testing (As there is several lists involved, the code is not that easy to understand) + if (cur->subsequent_commits_list) + { + bool have_lock; + wait_for_commit *waiter; + + mysql_mutex_lock(&cur->LOCK_wait_commit); + have_lock= true; + waiter= cur->subsequent_commits_list; + /* Check again, now safely under lock. */ + if (waiter) + { + /* Grab the list of waiters and process it. */ + cur->subsequent_commits_list= NULL; + do + { + wait_for_commit *next= waiter->next_subsequent_commit; + group_commit_entry *entry2= + (group_commit_entry *)waiter->opaque_pointer; + if (entry2) + { + /* + This is another transaction ready to be written to the binary + log. We can put it into the queue directly, without needing a + separate context switch to the other thread. We just set a flag + so that the other thread will know when it wakes up that it was + already processed. + + So put it at the end of the list to be processed in a subsequent + iteration of the outer loop. + */ + entry2->queued_by_other= true; + last->next_subsequent_commit= waiter; + last= waiter; + /* + As a small optimisation, we do not actually need to set + waiter->next_subsequent_commit to NULL, as we can use the + pointer `last' to check for end-of-list. + */ + } + else + { + /* + Wake up the waiting transaction. + + For this, we need to set the "wakeup running" flag and release + the waitee lock to avoid a deadlock, see comments on + THD::wakeup_subsequent_commits2() for details. + */ + if (have_lock) + { + cur->wakeup_subsequent_commits_running= true; + mysql_mutex_unlock(&cur->LOCK_wait_commit); + have_lock= false; Move have_lock= false to just after the 'if (have_lock)' - Easier to see that you are really reseting the variable. - Faster as there is no need to reload the variable from stack after mysql_mutex_unlock() function. - I try to nowadays in my code to always reset flag variables just after testing them to achive the above. + } + waiter->wakeup(); + } + waiter= next; + } while (waiter); + } You can optimze away one row, one 'if level' and one 'if' away by doing this: Move: 'cur->subsequent_commits_list= NULL;' before the if. Change 'if (waiter)' to 'while (waiter)' Remove 'do' and the corresponding 'while (waiter);' + if (have_lock) + mysql_mutex_unlock(&cur->LOCK_wait_commit); + } + if (cur == last) + break; Add a comment here of what is in next_subsequent_commit list. + cur= cur->next_subsequent_commit; + entry= (group_commit_entry *)cur->opaque_pointer; + DBUG_ASSERT(entry != NULL); } + + /* Now we need to clear the wakeup_subsequent_commits_running flags. */ + if (list) + { + for (;;) + { + if (list->wakeup_subsequent_commits_running) + { + mysql_mutex_lock(&list->LOCK_wait_commit); + list->wakeup_subsequent_commits_running= false; + mysql_mutex_unlock(&list->LOCK_wait_commit); Why do we need the mutex above? The only place where we test this variable, as far as I can find, is in wait_for_commit::register_wait_for_prior_commit() This thread doesn't know if the other thread is just before mysql_mutex_lock(&waitee->LOCK_wait_commit), inside the lock or after the lock. So when we reset the variable should not matter. But somehow this sounds strange. What happens if the above variable is reset just before mysql_mutex_lock(&waitee->LOCK_wait_commit) in wait_for_commit::register_wait_for_prior_commit() ? Isn't the waitee added to the list when it shouldn't? + } + if (list == last) + break; + list= list->next_subsequent_commit; + } + } + + if (opt_binlog_commit_wait_count > 0) + mysql_cond_signal(&COND_prepare_ordered); mysql_mutex_unlock(&LOCK_prepare_ordered); DEBUG_SYNC(entry->thd, "commit_after_release_LOCK_prepare_ordered"); + return orig_queue == NULL; +} + +bool +MYSQL_BIN_LOG::write_transaction_to_binlog_events(group_commit_entry *entry) +{ + wait_for_commit *wfc; + bool is_leader; + + wfc= entry->thd->wait_for_commit_ptr; + entry->queued_by_other= false; + if (wfc && wfc->waiting_for_commit) + { Add comment, something like: /* We have to wait for the threads in wfc->waiting_for_commit */ + mysql_mutex_lock(&wfc->LOCK_wait_commit); + /* Do an extra check here, this time safely under lock. */ + if (wfc->waiting_for_commit) + { + wfc->opaque_pointer= entry; + do + { + mysql_cond_wait(&wfc->COND_wait_commit, &wfc->LOCK_wait_commit); + } while (wfc->waiting_for_commit); + wfc->opaque_pointer= NULL; + } + mysql_mutex_unlock(&wfc->LOCK_wait_commit); + } <cut> @@ -6597,7 +6756,10 @@ MYSQL_BIN_LOG::write_transaction_to_binl if (next) { - next->thd->signal_wakeup_ready(); + if (next->queued_by_other) + next->thd->wait_for_commit_ptr->wakeup(); What does the above code mean ? (add a comment). Why do we wakeup the thread we where waiting for instead of ourselves? + else + next->thd->signal_wakeup_ready(); } else { <cut> @@ -6968,6 +7140,48 @@ MYSQL_BIN_LOG::write_transaction_or_stmt return 0; } + +void +MYSQL_BIN_LOG::wait_for_sufficient_commits() +{ + size_t count; + group_commit_entry *e; + group_commit_entry *last_head; + struct timespec wait_until; + + mysql_mutex_assert_owner(&LOCK_log); + mysql_mutex_assert_owner(&LOCK_prepare_ordered); + + count= 0; + for (e= last_head= group_commit_queue; e; e= e->next) + ++count; + if (count >= opt_binlog_commit_wait_count) + return; Assuming that the queue is bigger may be better: for (e= last_head= group_commit_queue, count=0; e ; e= e->next) if (++count >= opt_binlog_commit_wait_count) return; + mysql_mutex_unlock(&LOCK_log); Here you are going to take LOCK_log and LOCK_prepare_ordered in the wrong order. safe_mutex() should have noticed this. In other code you are first taking LOCK_log and then LOCK_prepare_ordered. If someone else is calling trx_group_commit_leader() while you are in this code you would get a mutex deadlock. + set_timespec_nsec(wait_until, (ulonglong)1000*opt_binlog_commit_wait_usec); + + for (;;) + { + int err; + group_commit_entry *head; + + err= mysql_cond_timedwait(&COND_prepare_ordered, &LOCK_prepare_ordered, + &wait_until); + if (err == ETIMEDOUT) + break; + head= group_commit_queue; + for (e= head; e && e != last_head; e= e->next) + ++count; + if (count >= opt_binlog_commit_wait_count) + break; + last_head= head; + } + + mysql_mutex_lock(&LOCK_log); +} <cut> === modified file 'sql/log_event.cc' --- sql/log_event.cc 2013-06-06 15:51:28 +0000 +++ sql/log_event.cc 2013-08-29 08:18:22 +0000 <cut> @@ -6101,6 +6110,18 @@ Gtid_log_event::Gtid_log_event(const cha domain_id= uint4korr(buf); buf+= 4; flags2= *buf; + if (flags2 & FL_GROUP_COMMIT_ID) + { + if (event_len < (uint)header_size + GTID_HEADER_LEN + 2) + { + seq_no= 0; // So is_valid() returns false + return; + } + ++buf; + commit_id= uint8korr(buf); + } + else + commit_id= 0; } Setting commit_id=0 first instead in the if will generate smaller and faster code (setting a variable is faster than an a jmp). === modified file 'sql/log_event.h' --- sql/log_event.h 2013-06-06 15:51:28 +0000 +++ sql/log_event.h 2013-08-29 08:18:22 +0000 @@ -1317,9 +1317,9 @@ class Log_event @see do_apply_event */ - int apply_event(Relay_log_info const *rli) + int apply_event(struct rpl_group_info *rgi) What is the simple rule to know when to use Relay_log_info and when to use rpl_group_info ? If I understand things correct, relay_log_info should only be used for direct manipulation of data in the relay log. By the way, you should probably change 'struct rpl_group_info' to just rpl_group_info; Now you are mixing both versions in the code. === modified file 'sql/log_event_old.cc' --- sql/log_event_old.cc 2013-02-19 10:45:29 +0000 +++ sql/log_event_old.cc 2013-08-29 08:18:22 +0000 @@ -67,7 +68,7 @@ Old_rows_log_event::do_apply_event(Old_r do_apply_event(). We still check here to prevent future coding errors. */ - DBUG_ASSERT(rli->sql_thd == ev_thd); + DBUG_ASSERT(rgi->thd == ev_thd); Should we change all test for rli->sql_thd to rgi->thd ? You have at least this test in log_event.c and slave.cc. I assume that rli should not have a sql_thd member anymore ? <cut> === added file 'sql/rpl_parallel.cc' --- sql/rpl_parallel.cc 1970-01-01 00:00:00 +0000 +++ sql/rpl_parallel.cc 2013-08-29 08:18:22 +0000 @@ -0,0 +1,699 @@ +#include "my_global.h" +#include "rpl_parallel.h" +#include "slave.h" +#include "rpl_mi.h" + + +/* + Code for optional parallel execution of replicated events on the slave. + + ToDo list: + + - Review every field in Relay_log_info, and all code that accesses it. + Split out the necessary parts into rpl_group_info, to avoid conflicts + between parallel execution of events. (Such as deferred events ...) + + - Error handling. If we fail in one of multiple parallel executions, we + need to make a best effort to complete prior transactions and roll back + following transactions, so slave binlog position will be correct. + And all the retry logic for temporary errors like deadlock. + + - Stopping the slave needs to handle stopping all parallel executions. And + the logic in sql_slave_killed() that waits for current event group to + complete needs to be extended appropriately... + + - Audit the use of Relay_log_info::data_lock. Make sure it is held + correctly in all needed places also when using parallel replication. + + - We need some user-configurable limit on how far ahead the SQL thread will + fetch and queue events for parallel execution (otherwise if slave gets + behind we will fill up memory with pending malloc()'ed events). + + - Fix update of relay-log.info and master.info. In non-GTID replication, + they must be serialised to preserve correctness. In GTID replication, we + should not update them at all except at slave thread stop. + + - All the waits (eg. in struct wait_for_commit and in + rpl_parallel_thread_pool::get_thread()) need to be killable. And on kill, + everything needs to be correctly rolled back and stopped in all threads, + to ensure a consistent slave replication state. I can add kill checking in all functions that waits for a mutex. Will do it with adding enter_cond()/exit_cond() in all functions waiting for mysql_cond_wait(). + + - Handle the case of a partial event group. This occurs when the master + crashes in the middle of writing the event group to the binlog. The + slave rolls back the transaction; parallel execution needs to be able + to deal with this wrt. commit_orderer and such. + + - Relay_log_info::is_in_group(). This needs to be handled correctly in all + callers. I think it needs to be split into two, one version in + Relay_log_info to be used from next_event() in slave.cc, one to be used in + per-transaction stuff. + + - We should fail if we connect to the master with opt_slave_parallel_threads + greater than zero and master does not support GTID. Just to avoid a bunch + of potential problems, we won't be able to do any parallel replication + in this case anyway. If the master doesn't support GTID, I think we should instead of stopping, just issue a warning in the log and continue with normal replication. Otherwise we would have problems when using multi-source replication if a single slave doesn't support GTID. +*/ + +struct rpl_parallel_thread_pool global_rpl_thread_pool; + + +static void +rpt_handle_event(rpl_parallel_thread::queued_event *qev, + struct rpl_parallel_thread *rpt) +{ + int err; + struct rpl_group_info *rgi= qev->rgi; + Relay_log_info *rli= rgi->rli; + THD *thd= rgi->thd; + + thd->rli_slave= rli; + thd->rpl_filter = rli->mi->rpl_filter; + /* ToDo: Get rid of rli->group_info, it is not thread safe. */ + rli->group_info= rgi; + + /* ToDo: Access to thd, and what about rli, split out a parallel part? */ + mysql_mutex_lock(&rli->data_lock); + err= apply_event_and_update_pos(qev->ev, thd, rgi, rpt); We should also get rid of the thd argument and change the function to be a bool so that we can return an error. + /* ToDo: error handling. */ +} + + +pthread_handler_t +handle_rpl_parallel_thread(void *arg) +{ <cut> + mysql_mutex_lock(&rpt->LOCK_rpl_thread); + rpt->thd= thd; + + while (rpt->delay_start) + mysql_cond_wait(&rpt->COND_rpl_thread, &rpt->LOCK_rpl_thread); A much easier way to achive the wait is to have the caller lock mysql_mutex_lock(&rpt->LOCK_rpl_thread) and then just release it when it's time for this thread to continue. - No need for signaling or checking if the thread got the signal - No need to have a cond wait here - No need to have a delay_start flag + rpt->running= true; + + while (!rpt->stop && !thd->killed) + { + rpl_parallel_thread *list; + + old_msg= thd->proc_info; + thd->enter_cond(&rpt->COND_rpl_thread, &rpt->LOCK_rpl_thread, + "Waiting for work from SQL thread"); + while (!rpt->stop && !thd->killed && !(events= rpt->event_queue)) + mysql_cond_wait(&rpt->COND_rpl_thread, &rpt->LOCK_rpl_thread); Add comment: /* Mark that this thread is now executing */ + rpt->free= false; + rpt->event_queue= rpt->last_in_queue= NULL; + thd->exit_cond(old_msg); + Note that events may be undefined here if thd->killed was set above (for the first loop) It would be good to check for thd->killed or rpt->stop here and then abort the loop. Another question: do we really need rpt->stop ? Isn't it enough with using the thd->killed for this ? Otherwise we would need to check for both rpt->top and thd->killed in every loop that may be aborted. + more_events: + while (events) + { + struct rpl_parallel_thread::queued_event *next= events->next; + Log_event_type event_type= events->ev->get_type_code(); + rpl_group_info *rgi= events->rgi; + rpl_parallel_entry *entry= rgi->parallel_entry; + uint64 wait_for_sub_id; + uint64 wait_start_sub_id; + bool end_of_group; + Add a comment why this is handled here and not in rpt_handle_event() + if (event_type == GTID_EVENT) + { + in_event_group= true; Add a comment what the following variable stands for + group_standalone= + (0 != (static_cast<Gtid_log_event *>(events->ev)->flags2 & + Gtid_log_event::FL_STANDALONE)); + Rest of this file will be reviewed tomorrow. <cut> === modified file 'sql/rpl_rli.cc' --- sql/rpl_rli.cc 2013-06-06 15:51:28 +0000 +++ sql/rpl_rli.cc 2013-08-29 08:18:22 +0000 @@ -1194,13 +1194,15 @@ bool Relay_log_info::cached_charset_comp void Relay_log_info::stmt_done(my_off_t event_master_log_pos, - time_t event_creation_time, THD *thd) + time_t event_creation_time, THD *thd, + struct rpl_group_info *rgi) Do we need thd anymore as this is in rgi->thd? If they are not the same, please add a comment about this. It's not otherwise clear when to use rgi->thd and thd as this function uses both. @@ -1265,7 +1267,7 @@ void Relay_log_info::cleanup_context(THD { DBUG_ENTER("Relay_log_info::cleanup_context"); - DBUG_ASSERT(sql_thd == thd); + DBUG_ASSERT(opt_slave_parallel_threads > 0 || sql_thd == thd); Add a comment which THD is passed to the function. <cut> + +int +event_group_new_gtid(rpl_group_info *rgi, Gtid_log_event *gev) +{ + uint64 sub_id= rpl_global_gtid_slave_state.next_subid(gev->domain_id); + if (!sub_id) + { Add a comment that this is an end of memory error. <cut> + +void +delete_or_keep_event_post_apply(Relay_log_info *rli, + Log_event_type typ, Log_event *ev) +{ + /* + ToDo: This needs to work on rpl_group_info, not Relay_log_info, to be + thread-safe for parallel replication. + */ + switch (typ) { + case FORMAT_DESCRIPTION_EVENT: + /* + Format_description_log_event should not be deleted because it + will be used to read info about the relay log's format; + it will be deleted when the SQL thread does not need it, + i.e. when this thread terminates. + */ + break; + case ANNOTATE_ROWS_EVENT: + /* + Annotate_rows event should not be deleted because after it has + been applied, thd->query points to the string inside this event. + The thd->query will be used to generate new Annotate_rows event + during applying the subsequent Rows events. + */ + rli->set_annotate_event((Annotate_rows_log_event*) ev); I assume we should create relay_group_info->set_annotate_event instead of the above. + break; + case DELETE_ROWS_EVENT: + case UPDATE_ROWS_EVENT: + case WRITE_ROWS_EVENT: + /* + After the last Rows event has been applied, the saved Annotate_rows + event (if any) is not needed anymore and can be deleted. + */ + if (((Rows_log_event*)ev)->get_flags(Rows_log_event::STMT_END_F)) + rli->free_annotate_event(); + /* fall through */ + default: + DBUG_PRINT("info", ("Deleting the event after it has been executed")); + if (!rli->is_deferred_event(ev)) + delete ev; I assume that is_deferred_event should also be moved to relay_group_info. + break; + } +} + #endif === modified file 'sql/rpl_rli.h' --- sql/rpl_rli.h 2013-06-06 15:51:28 +0000 +++ sql/rpl_rli.h 2013-08-29 08:18:22 +0000 @@ -22,6 +22,7 @@ #include "log.h" /* LOG_INFO, MYSQL_BIN_LOG */ #include "sql_class.h" /* THD */ #include "log_event.h" +#include "rpl_parallel.h" struct RPL_TABLE_LIST; class Master_info; @@ -52,6 +53,8 @@ class Master_info; *****************************************************************************/ +struct rpl_group_info; + class Relay_log_info : public Slave_reporting_capability { public: @@ -311,13 +314,9 @@ class Relay_log_info : public Slave_repo char slave_patternload_file[FN_REFLEN]; size_t slave_patternload_file_size; - /* - Current GTID being processed. - The sub_id gives the binlog order within one domain_id. A zero sub_id - means that there is no active GTID. - */ - uint64 gtid_sub_id; - rpl_gtid current_gtid; + /* ToDo: We need to remove this, always use the per-transaction one to work with parallel replication. */ + struct rpl_group_info *group_info; + rpl_parallel parallel; Relay_log_info(bool is_slave_recovery); ~Relay_log_info(); @@ -459,7 +458,8 @@ class Relay_log_info : public Slave_repo the <code>Seconds_behind_master</code> field. */ void stmt_done(my_off_t event_log_pos, - time_t event_creation_time, THD *thd); + time_t event_creation_time, THD *thd, + struct rpl_group_info *rgi); /** @@ -594,6 +594,60 @@ class Relay_log_info : public Slave_repo }; +/* + This is data for various state needed to be kept for the processing of + one event group in the SQL thread. + + For single-threaded replication it is linked from the RLI, for parallel + replication it is linked into each event group being executed in parallel. +*/ Could you please rewrite the above to make it more clear. After discussing with you, I think the following could be added: /* In single-threaded replication, there will be one global rpl_group_info and one global RPLI per master connection. They will be linked together. In parallel replication, there will be one rpl_group_info object for each running thd. All rpl_group_info will share the same RLI. */ +struct rpl_group_info +{ + Relay_log_info *rli; + THD *thd; <cut> === modified file 'sql/sql_class.cc' --- sql/sql_class.cc 2013-06-06 15:51:28 +0000 +++ sql/sql_class.cc 2013-08-29 08:18:22 +0000 +void +wait_for_commit::wakeup() +{ + /* + We signal each waiter on their own condition and mutex (rather than using + pthread_cond_broadcast() or something like that). + + Otherwise we would need to somehow ensure that they were done + waking up before we could allow this THD to be destroyed, which would + be annoying and unnecessary. + */ + mysql_mutex_lock(&LOCK_wait_commit); + waiting_for_commit= false; + mysql_cond_signal(&COND_wait_commit); + mysql_mutex_unlock(&LOCK_wait_commit); +} In this particular case it looks safe to move the cond signal out of the mutex. I don't see how anyone could miss the signal. + + +/* + Register that the next commit of this THD should wait to complete until + commit in another THD (the waitee) has completed. + + The wait may occur explicitly, with the waiter sitting in + wait_for_prior_commit() until the waitee calls wakeup_subsequent_commits(). + + Alternatively, the TC (eg. binlog) may do the commits of both waitee and + waiter at once during group commit, resolving both of them in the right + order. + + Only one waitee can be registered for a waiter; it must be removed by + wait_for_prior_commit() or unregister_wait_for_prior_commit() before a new + one is registered. But it is ok for several waiters to register a wait for + the same waitee. It is also permissible for one THD to be both a waiter and + a waitee at the same time. +*/ +void +wait_for_commit::register_wait_for_prior_commit(wait_for_commit *waitee) +{ + waiting_for_commit= true; + DBUG_ASSERT(!this->waitee /* No prior registration allowed */); + this->waitee= waitee; + + mysql_mutex_lock(&waitee->LOCK_wait_commit); + /* + If waitee is in the middle of wakeup, then there is nothing to wait for, + so we need not register. This is necessary to avoid a race in unregister, + see comments on wakeup_subsequent_commits2() for details. + */ + if (waitee->wakeup_subsequent_commits_running) + waiting_for_commit= false; + else + { Please add a comment here what the following code is doing. As this is two different lists involved, it's not trivial code. You also seam to use the list next_subsequent_commit for something different in queue_for_group_commit() code. + this->next_subsequent_commit= waitee->subsequent_commits_list; + waitee->subsequent_commits_list= this; + } + mysql_mutex_unlock(&waitee->LOCK_wait_commit); +} <cut> +/* + Wakeup anyone waiting for us to have committed. + + Note about locking: + + We have a potential race or deadlock between wakeup_subsequent_commits() in + the waitee and unregister_wait_for_prior_commit() in the waiter. + + Both waiter and waitee needs to take their own lock before it is safe to take + a lock on the other party - else the other party might disappear and invalid + memory data could be accessed. But if we take the two locks in different + order, we may end up in a deadlock. + + The waiter needs to lock the waitee to delete itself from the list in + unregister_wait_for_prior_commit(). Thus wakeup_subsequent_commits() can not + hold its own lock while locking waiters, lest we deadlock. lest ? + So we need to prevent unregister_wait_for_prior_commit() running while wakeup + is in progress - otherwise the unregister could complete before the wakeup, + leading to incorrect spurious wakeup or accessing invalid memory. + <cut> + Then also register_wait_for_prior_commit() needs to check if + wakeup_subsequent_commits() is running, and skip the registration if + so. This is needed in case a new waiter manages to register itself and + immediately try to unregister while wakeup_subsequent_commits() is + running. Else the new waiter would also wait rather than unregister, but it + would not be woken up until next wakeup, which could be potentially much + later than necessary. +*/ Add new line here +void +wait_for_commit::wakeup_subsequent_commits2() +{ <cut> === modified file 'sql/sql_class.h' --- sql/sql_class.h 2013-06-06 15:51:28 +0000 +++ sql/sql_class.h 2013-08-29 08:18:22 +0000 @@ -1553,6 +1553,116 @@ class Global_read_lock }; +/* + Class to facilitate the commit of one transactions waiting for the commit of + another transaction to complete first. + + This is used during (parallel) replication, to allow different transactions + to be applied in parallel, but still commit in order. + + The transaction that wants to wait for a prior commit must first register + to wait with register_wait_for_prior_commit(waitee). Such registration + must be done holding the waitee->LOCK_wait_commit, to prevent the other + THD from disappearing during the registration. + + Then during commit, if a THD is registered to wait, it will call + wait_for_prior_commit() as part of ha_commit_trans(). If no wait is + registered, or if the waitee for has already completed commit, then + wait_for_prior_commit() returns immediately. + + And when a THD that may be waited for has completed commit (more precisely + commit_ordered()), then it must call wakeup_subsequent_commits() to wake + up any waiters. Note that this must be done at a point that is guaranteed + to be later than any waiters registering themselves. It is safe to call + wakeup_subsequent_commits() multiple times, as waiters are removed from + registration as part of the wakeup. + + The reason for separate register and wait calls is that this allows to + register the wait early, at a point where the waited-for THD is known to + exist. And then the actual wait can be done much later, where the + waited-for THD may have been long gone. By registering early, the waitee + can signal before disappearing. +*/ Instead of taking locks to ensure that THD will disapper, why not keep all THD objects around until all transactions in a group has been executed? +struct wait_for_commit +{ <cut> + bool waiting_for_commit; + /* + Flag set when wakeup_subsequent_commits_running() is active, see commonts commonts -> comments === modified file 'sql/sys_vars.cc' --- sql/sys_vars.cc 2013-06-07 07:31:11 +0000 +++ sql/sys_vars.cc 2013-08-29 08:18:22 +0000 <cut> +static bool +check_slave_parallel_threads(sys_var *self, THD *thd, set_var *var) +{ + bool running; + + mysql_mutex_lock(&LOCK_active_mi); + running= master_info_index->give_error_if_slave_running(); + mysql_mutex_unlock(&LOCK_active_mi); + if (running) + return true; + + return false; +} + +static bool +fix_slave_parallel_threads(sys_var *self, THD *thd, enum_var_type type) +{ + bool running; + bool err= false; + + mysql_mutex_unlock(&LOCK_global_system_variables); + mysql_mutex_lock(&LOCK_active_mi); + running= master_info_index->give_error_if_slave_running(); + mysql_mutex_unlock(&LOCK_active_mi); You can call check_slave_parallel_threads() above to reuse code. + if (running || rpl_parallel_change_thread_count(&global_rpl_thread_pool, + opt_slave_parallel_threads)) + err= true; + mysql_mutex_lock(&LOCK_global_system_variables); + + return err; +} + + +static Sys_var_ulong Sys_slave_parallel_threads( + "slave_parallel_threads", + "If non-zero, number of threads to spawn to apply in parallel events " + "on the slave that were group-committed on the master or were logged " + "with GTID in different replication domains.", + GLOBAL_VAR(opt_slave_parallel_threads), CMD_LINE(REQUIRED_ARG), + VALID_RANGE(0,16383), DEFAULT(0), BLOCK_SIZE(1), NO_MUTEX_GUARD, + NOT_IN_BINLOG, ON_CHECK(check_slave_parallel_threads), + ON_UPDATE(fix_slave_parallel_threads)); #endif We have talked about having 1 to be the minimum, which would give the old behavior. <cut> Overal things looks very good and it should not be too much work to add the things you have written on the todo part.a Regards Monty

2 10

[Maria-developers] [Spatial] On current implementation approach
by Mateusz Loskot 25 Sep '13

25 Sep '13

Hi, I'm going to ask question about how the current Spatial Extensions are implemented. I have spent some time reading the source code in the current trunk (spatial.h|cc, gcal*.h|cc, related Field and Item definitions, etc.), so I have a rough understanding of the overall structure, how the geometry data types are implemented and exposed to SQL, how the spatial functions are defined and registered. I did not looked into details of implementation of geospatial algorithms, but that's too low level for the question I'm going to ask. Initially, I was going to ask very detailed question, listing all the relevant code definitions and asking separately about each of them, but that's not necessary at this stage, I think. Instead, I'm going to simplify and ask about the bigger picture, more about MariaDB extensions API: 1. Is it possible to implement MariaDB extensions like Spatial (custom type + set of functions) without such a tight coupling with the internal implementation of the type system (without messing Field class with geometry types directly, etc.)? 2. Is it possible to implement Spatial using User-Defined Functions (UDF) defined in shared binary? 3. What is the reason behind using Well-Known-Binary (WKB) stream of bytes to transport geometry values into/from functions? Is it due to limitations of MariaDB type system where String is the only universal carrier for complex data? This concern is related to necessity of encoding/decoding WKB when chaining spatial function calls, and possibilities to avoid it. Best regards, -- Mateusz Loskot, http://mateusz.loskot.net "Participation in this whole process is a form of torture" ~~ Szalony

3 10

[Maria-developers] close mdev-4387
by Roberto Spadim 23 Sep '13

23 Sep '13

hi guys, please close https://mariadb.atlassian.net/browse/MDEV-4387 it's duplicated by alread commited https://mariadb.atlassian.net/browse/MDEV-5025 thanks =) -- Roberto Spadim

1 0

Re: [Maria-developers] [Roles] Final Status
by Vicentiu Ciorbaru 23 Sep '13

23 Sep '13

Hi Sergei! >> Even though I didn't manage to implement everything yet, I want to >> finish this project and get it ready for an upstream push, outside of >> GSoC. > 1. I'd need either from you in email (but then cc: it to > maria-developers) or in your blog (or both) a statement about the > license. A simple one would be to say that this your code is available > to us under BSD license. Another option is our MariaDB Contributor > Agreement: https://mariadb.com/kb/en/mca/ I hereby declare that all my work done so far (and future) by me on the Roles project, is released under the BSD licence. I will also make a statement on my blog in my wrap up post a bit later. If there are any more clarifications needed, let me know! Regards, Vicențiu

2 2

[Maria-developers] Review of MDEV-4506, parallel replication, part 3
by Michael Widenius 22 Sep '13

22 Sep '13

Hi! This is a review of the latest code pushed to your tree: bzr diff -r 3662.. Note that I will apply some of the trivial changes myself to the tree to speed up things (this is mostly about comments). === added file 'mysql-test/suite/rpl/r/rpl_parallel.result' --- mysql-test/suite/rpl/r/rpl_parallel.result 1970-01-01 00:00:00 +0000 +++ mysql-test/suite/rpl/r/rpl_parallel.result 2013-09-22 20:25:20 +0000 Great that you got these done! Will make my life much easier when I work the parallel replication code! <cut> === modified file 'sql/log.cc' --- sql/log.cc 2013-07-04 22:26:15 +0000 +++ sql/log.cc 2013-09-22 20:25:20 +0000 @@ -6542,27 +6542,93 @@ MYSQL_BIN_LOG::write_transaction_to_binl } } + +/* + Put a transaction that is ready to commit in the group commit queue. + The transaction is identified by the ENTRY object passed into this function. + + To facilitate group commit for the binlog, we first queue up ourselves in + this function. Then later the first thread to enter the queue waits for + the LOCK_log mutex, and commits for everyone in the queue once it gets the + lock. Any other threads in the queue just wait for the first one to finish + the commit and wake them up. This way, all transactions in the queue get + committed in a single disk operation. + + The return value of this function is TRUE if queued as the first entry in + the queue (meaning this is the leader), FALSE otherwise. I moved this last in the comment as: @retval TRUE If queued as the first entry in the queue (meaning this is the leader) @retval FALSE Otherwise + + The main work in this function is when the commit in one transaction has + been marked to wait for the commit of another transaction to happen + first. This is used to support in-order parallel replication, where + transactions can execute out-of-order but need to be committed in-order with + how they happened on the master. The waiting of one commit on another needs + to be integrated with the group commit queue, to ensure that the waiting + transaction can participate in the same group commit as the waited-for + transaction. + + So when we put a transaction in the queue, we check if there were other + transactions already prepared to commit but just waiting for the first one + to commit. If so, we add those to the queue as well, transitively for all + waiters. +*/ + <cut> @@ -6574,10 +6640,28 @@ MYSQL_BIN_LOG::queue_for_group_commit(gr This would be natural to do with recursion, but we want to avoid potentially unbounded recursion blowing the C stack, so we use the list approach instead. + + We keep a list of all the waiters that need to be processed in `list', + linked through the next_subsequent_commit pointer. Initially this list + contains only the entry passed into this function. + + We process entries in the list one by one. The element currently being + processed is pointed to by `cur`, and the element at the end of the list + is pointed to by `last` (we do not use NULL to terminate the list). + + As we process an element, it is first added to the group_commit_queue. + Then any waiters for that element are added at the end of the list, to + be processed in subsequent iterations. This continues until the list + is exhausted, with all elements ever added eventually processed. + + The end result is a breath-first traversal of the tree of waiters, + re-using the next_subsequent_commit pointers in place of extra stack + space in a recursive traversal. I added: The temporary list created in next_subsequent_commit is not used by the caller or any other function. @@ -6670,6 +6763,18 @@ MYSQL_BIN_LOG::queue_for_group_commit(gr { if (list->wakeup_subsequent_commits_running) { + /* + ToDo: We should not need a full lock/unlock of LOCK_wait_commit + here. All we need is a single (full) memory barrier, to ensure that + the reads of the list above are not reordered with the write of + wakeup_subsequent_commits_running, or with the writes to the list + from other threads that is allowed to happen after + wakeup_subsequent_commits_running has been set to false. + + We do not currently have explicit memory barrier primitives in the + source tree, but if we get them the below mysql_mutex_lock() could + be replaced with a full memory barrier just before the loop. + */ ok (for now). Another way would of course to only do a mutex lock for the first entry we find... No more comments. Everything looked fine! I will start coordinate with you tomorrow about things to do. Regards, Monty

1 0

[Maria-developers] Status and future of spatial extensions
by Mateusz Loskot 22 Sep '13

22 Sep '13

Hi, I haven't monitored MariaDB development closely, neither I tracked what's happening in the MySQL sandbox, so I'm hoping to catch up by asking questions here. I'm interested in the Spatial Extensions. So, I looked around the KB and the documentation and I found a few pages: general overview [1], 5.3 features [2] and page about plans [3]. I've looked into JIRA issues and recent commits too. Still, I don't see a clear big picture about the current status, roadmap and developers of the Spatial Extensions. Could anyone shed more light on the Spatial part of the MariaDB project, where it is now, where it is going, who is behind the wheel, etc.? What is (if any) the relation with geo-spatial development in MySQL? Is that correct understanding that MySQL and MariaDB follow separate paths, thus the GIS implementations go separately and in future they may seriously diverge and become no longer equivalent drop-in replacements? [1] https://mariadb.com/kb/en/gis-functionality/ [2] https://mariadb.com/kb/en/the-maria53-gis-tree-on-launchpad/ [3] https://mariadb.com/kb/en/mariadb-plans-gis/ Best regards, -- Mateusz Loskot, http://mateusz.loskot.net "Participation in this whole process is a form of torture" ~~ Szalony

1 0

Re: [Maria-developers] Rev 3818: Check for duplicate index (port from mysql)
by Sergei Golubchik 22 Sep '13

22 Sep '13

Hi, Sanja! On Sep 20, sanja(a)askmonty.org wrote: > ------------------------------------------------------------ > revno: 3818 > revision-id: sanja(a)askmonty.org-20130920193019-vr2q24c8g255aofu > parent: sanja(a)askmonty.org-20130920141533-nwk5jt07ojwmmctr > committer: sanja(a)askmonty.org > branch nick: work-maria-10.0-fts2 > timestamp: Fri 2013-09-20 22:30:19 +0300 > message: > Check for duplicate index (port from mysql) > (pre fts) ok to push Regards, Sergei

1 0

[Maria-developers] Review of MDEV-4506, parallel replication, part 2
by Michael Widenius 22 Sep '13

22 Sep '13

Hi! Part 2 of review of parallel replication === added file 'sql/rpl_parallel.cc' <cut> +pthread_handler_t +handle_rpl_parallel_thread(void *arg) +{ <cut> + if (end_of_group) + { + in_event_group= false; + Add comment: /* All events for this group has now been executed and logged (but not necessaerly synced). Inform the other event groups that are waiting for this thread that we are done. */ + rgi->commit_orderer.unregister_wait_for_prior_commit(); + thd->wait_for_commit_ptr= NULL; + + /* + Record that we have finished, so other event groups will no + longer attempt to wait for us to commit. + + We can race here with the next transactions, but that is fine, as + long as we check that we do not decrease last_committed_sub_id. If + this commit is done, then any prior commits will also have been + done and also no longer need waiting for. + */ + mysql_mutex_lock(&entry->LOCK_parallel_entry); + if (entry->last_committed_sub_id < event_gtid_sub_id) + { + entry->last_committed_sub_id= event_gtid_sub_id; + mysql_cond_broadcast(&entry->COND_parallel_entry); + } + mysql_mutex_unlock(&entry->LOCK_parallel_entry); + Add: /* Tell storage engines that they can stop waiting. See comments in sql_class.cc::wakeup_subsequent_commits2() for why we can't do this part of the wakeup in unregister_wait_for_prior_commit(). */ + rgi->commit_orderer.wakeup_subsequent_commits(); + delete rgi; + } + + events= next; + } + + mysql_mutex_lock(&rpt->LOCK_rpl_thread); + if ((events= rpt->event_queue) != NULL) + { + rpt->event_queue= rpt->last_in_queue= NULL; + mysql_mutex_unlock(&rpt->LOCK_rpl_thread); + goto more_events; + } + + if (!in_event_group) + { + rpt->current_entry= NULL; As far as I can see, rpt->free is always 0 here. - It's set to 0 at the top of this loop and only set to 1 here. Can we remove the rpt->free variable? + if (!rpt->stop && !rpt->free) + { Add comment. /* Take next group of events from the replication pool. This is faster than having to wakeup the pool manager thread to give us a new event. */ + mysql_mutex_lock(&rpt->pool->LOCK_rpl_thread_pool); + list= rpt->pool->free_list; + rpt->next= list; + rpt->pool->free_list= rpt; + if (!list) + mysql_cond_broadcast(&rpt->pool->COND_rpl_thread_pool); + mysql_mutex_unlock(&rpt->pool->LOCK_rpl_thread_pool); + rpt->free= true; + } + } + } <cut> +int +rpl_parallel_change_thread_count(rpl_parallel_thread_pool *pool, + uint32 new_count, bool skip_check) +{ + uint32 i; + rpl_parallel_thread **new_list= NULL; + rpl_parallel_thread *new_free_list= NULL; + + /* + Allocate the new list of threads up-front. + That way, if we fail half-way, we only need to free whatever we managed + to allocate, and will not be left with a half-functional thread pool. + */ + if (new_count && + !(new_list= (rpl_parallel_thread **)my_malloc(new_count*sizeof(*new_list), + MYF(MY_WME)))) + { + my_error(ER_OUTOFMEMORY, MYF(0), (int(new_count*sizeof(*new_list)))); + goto err;; + } Why not allocate new_list[] elements at the same time? Faster, less memory not allocation to check and freeing is also esier. if (!my_multi_malloc(MYF(MY_WME), &new_list, (uint) new_count*sizeof(*new_list), &list_part, new_count*sizeof(**new_list), NULL)) ... + + for (i= 0; i < new_count; ++i) + { + pthread_t th; + + if (!(new_list[i]= (rpl_parallel_thread *)my_malloc(sizeof(*(new_list[i])), + MYF(MY_WME)))) + { + my_error(ER_OUTOFMEMORY, MYF(0), sizeof(*(new_list[i]))); + goto err; + } The above would then simple be: new_list[i]= list_part++; Instead of setting everything to false and NULL, why not simple bzero() the structure? That way we can be more sure that we don't forget to initialize any new element in the future. + new_list[i]->delay_start= true; + new_list[i]->running= false; + new_list[i]->stop= false; + new_list[i]->free= false; + mysql_mutex_init(key_LOCK_rpl_thread, &new_list[i]->LOCK_rpl_thread, + MY_MUTEX_INIT_SLOW); + mysql_cond_init(key_COND_rpl_thread, &new_list[i]->COND_rpl_thread, NULL); + new_list[i]->pool= pool; + new_list[i]->current_entry= NULL; + new_list[i]->event_queue= NULL; + new_list[i]->last_in_queue= NULL; <cut> Add: /* The following loops wait until each thread is done it's work for each event group and then stops it when the thread has nothing to do. Note that this may take some time as rpl_paralell:do_event() may request threads at the same time. */ To make this faster and ensuring we don't start new request, wouldn't it be better to go through pool->threads and signal each of them? As long as rpt->stop is defined as 'stop when you are done processing one group' then this is fine (which is how it looks it's defined now). + for (i= 0; i < pool->count; ++i) + { + rpl_parallel_thread *rpt= pool->get_thread(NULL); + rpt->stop= true; + mysql_cond_signal(&rpt->COND_rpl_thread); + mysql_mutex_unlock(&rpt->LOCK_rpl_thread); + } + + for (i= 0; i < pool->count; ++i) + { + rpl_parallel_thread *rpt= pool->threads[i]; + mysql_mutex_lock(&rpt->LOCK_rpl_thread); + while (rpt->running) + mysql_cond_wait(&rpt->COND_rpl_thread, &rpt->LOCK_rpl_thread); + mysql_mutex_unlock(&rpt->LOCK_rpl_thread); + mysql_mutex_destroy(&rpt->LOCK_rpl_thread); + mysql_cond_destroy(&rpt->COND_rpl_thread); + my_free(rpt); By using my_multi_malloc() you can remove the above free + } + + my_free(pool->threads); + pool->threads= new_list; + pool->free_list= new_free_list; + pool->count= new_count; + for (i= 0; i < pool->count; ++i) + { + mysql_mutex_lock(&pool->threads[i]->LOCK_rpl_thread); + pool->threads[i]->delay_start= false; + mysql_cond_signal(&pool->threads[i]->COND_rpl_thread); + mysql_mutex_unlock(&pool->threads[i]->LOCK_rpl_thread); + } + + if (!skip_check) + { + mysql_mutex_lock(&LOCK_active_mi); + pool->changing= false; + mysql_mutex_unlock(&LOCK_active_mi); + } + return 0; + +err: + if (new_list) + { + while (new_free_list) + { + rpl_parallel_thread *next= new_free_list->next; + mysql_mutex_lock(&new_free_list->LOCK_rpl_thread); + new_free_list->delay_start= false; + new_free_list->stop= true; + mysql_cond_signal(&new_free_list->COND_rpl_thread); + while (!new_free_list->running) + mysql_cond_wait(&new_free_list->COND_rpl_thread, + &new_free_list->LOCK_rpl_thread); + while (new_free_list->running) + mysql_cond_wait(&new_free_list->COND_rpl_thread, + &new_free_list->LOCK_rpl_thread); + mysql_mutex_unlock(&new_free_list->LOCK_rpl_thread); + my_free(new_free_list); By using my_multi_malloc() you can remove the above free and even the 'next' variable + new_free_list= next; + } + my_free(new_list); + } <cut> <cut> +void +rpl_parallel::wait_for_done() +{ + struct rpl_parallel_entry *e; + uint32 i; + + for (i= 0; i < domain_hash.records; ++i) + { + e= (struct rpl_parallel_entry *)my_hash_element(&domain_hash, i); + mysql_mutex_lock(&e->LOCK_parallel_entry); + while (e->current_sub_id > e->last_committed_sub_id) + mysql_cond_wait(&e->COND_parallel_entry, &e->LOCK_parallel_entry); + mysql_mutex_unlock(&e->LOCK_parallel_entry); + } +} How to handle errors in the above case? For example when we get an error and we had to abort things? I assume we should also abort for: e->rpl_thread->thd->killed() but this is hard as thd and even rpl_thread may be deleted. One way to fix this would be to add an entry 'killed' to rpl_parell_entry that we can set. However it will be hard to keep this up to date. The other way would be to, on error, set last_committed_sub_id back to it's last value. We could also have some kind of shutdown flag that we check to avoid hang on shutdown. What do you suggest? Assuming setting the value back would be the easiest solution.. + for (i= 0; i < domain_hash.records; ++i) + { + e= (struct rpl_parallel_entry *)my_hash_element(&domain_hash, i); + mysql_mutex_lock(&e->LOCK_parallel_entry); + while (e->current_sub_id > e->last_committed_sub_id) + mysql_cond_wait(&e->COND_parallel_entry, &e->LOCK_parallel_entry); + mysql_mutex_unlock(&e->LOCK_parallel_entry); + } +bool +rpl_parallel::do_event(struct rpl_group_info *serial_rgi, Log_event *ev) +{ + rpl_parallel_entry *e; + rpl_parallel_thread *cur_thread; + rpl_parallel_thread::queued_event *qev; + struct rpl_group_info *rgi; + Relay_log_info *rli= serial_rgi->rli; + enum Log_event_type typ; + + /* ToDo: what to do with this lock?!? */ + mysql_mutex_unlock(&rli->data_lock); + + if (!(qev= (rpl_parallel_thread::queued_event *)my_malloc(sizeof(*qev), + MYF(0)))) + { + my_error(ER_OUT_OF_RESOURCES, MYF(0)); + return true; + } + qev->ev= ev; + qev->next= NULL; + + if ((typ= ev->get_type_code()) == GTID_EVENT) + { + Gtid_log_event *gtid_ev= static_cast<Gtid_log_event *>(ev); + + if (!(e= find(gtid_ev->domain_id)) || + !(rgi= new rpl_group_info(rli)) || + event_group_new_gtid(rgi, gtid_ev)) + { Here add delete of rgi. Also set rgi to 0 before. + my_error(ER_OUT_OF_RESOURCES, MYF(MY_WME)); + return true; + } + + if ((gtid_ev->flags2 & Gtid_log_event::FL_GROUP_COMMIT_ID) && + e->last_commit_id == gtid_ev->commit_id) + { + /* + We are already executing something else in this domain. But the two + event groups were committed together in the same group commit on the + master, so we can still do them in parallel here on the slave. + + However, the commit of this event must wait for the commit of the prior + event, to preserve binlog commit order and visibility across all + servers in the replication hierarchy. + */ + rpl_parallel_thread *rpt= global_rpl_thread_pool.get_thread(e); Here we could add a test if get_thread() is 0, which could happen on shutdown or when killing a replication thread. I think it's relatively safe to use get_thread() as a end-synchronize point as we should never have anything half-done when we call get_thread()). As get_thread() can take a very long time and is not called in many places, it's also sounds like a resonable thing to do. + rgi->wait_commit_sub_id= e->current_sub_id; + rgi->wait_commit_group_info= e->current_group_info; + rgi->wait_start_sub_id= e->prev_groupcommit_sub_id; + e->rpl_thread= cur_thread= rpt; + /* get_thread() returns with the LOCK_rpl_thread locked. */ Add the abvove comments also where get_thread() is defined. + } + else + { + /* Check if we already have a worker thread for this entry. */ I had a hard time to understand what the above meant. Is this new suggested comments correct? /* Get the last or current thread for the domain */ + cur_thread= e->rpl_thread; + if (cur_thread) + { /* Active thread exists. The thread may still be executing queries for this domain or it may have been rescheduled to work for another domain. */ + mysql_mutex_lock(&cur_thread->LOCK_rpl_thread); + if (cur_thread->current_entry != e) + { + /* Not ours anymore, we need to grab a new one. */ Suggested new comment: /* The thread has been reused for another domain. (e->rpl_thread was not reset or was changed during the mutex_lock) As there is no threads working on this domain, it's safe to assign a new thread to work on it */ + mysql_mutex_unlock(&cur_thread->LOCK_rpl_thread); + e->rpl_thread= cur_thread= NULL; + } + } <cut> Add here: /* Current is not set for the case where the master did not have GTID, like a MariaDB 5.5 or MySQL master. */ + else if (!Log_event::is_group_event(typ) || !current) + { + /* + Events like ROTATE and FORMAT_DESCRIPTION. Do not run in worker thread. + Same for events not preceeded by GTID (we should not see those normally, + but they might be from an old master). + */ + qev->rgi= serial_rgi; + rpt_handle_event(qev, NULL); + delete_or_keep_event_post_apply(rli, typ, qev->ev); Why is delete_or_keep_event_post_apply() not part of rpt_handle_event()? You always call these two together... + + return false; + } + else + { The following code is identical (except one line) to the code earlier in this function. Better to create a function for this to reuse code and to clarify the differences. + cur_thread= current->rpl_thread; + if (cur_thread) + { + mysql_mutex_lock(&cur_thread->LOCK_rpl_thread); + if (cur_thread->current_entry != current) + { + /* Not ours anymore, we need to grab a new one. */ + mysql_mutex_unlock(&cur_thread->LOCK_rpl_thread); + cur_thread= NULL; + } + } + if (!cur_thread) + { + cur_thread= current->rpl_thread= + global_rpl_thread_pool.get_thread(current); + } + } + qev->rgi= current->current_group_info; + + /* + Queue the event for processing. + */ + if (cur_thread->last_in_queue) + cur_thread->last_in_queue->next= qev; + else + cur_thread->event_queue= qev; + cur_thread->last_in_queue= qev; + mysql_cond_signal(&cur_thread->COND_rpl_thread); + mysql_mutex_unlock(&cur_thread->LOCK_rpl_thread); In this case it's safe to move the cond_signal outside of the mutex_unlock. (Should make things notable faster as we win a sleep-wakup in other thread). <cut> === added file 'sql/rpl_parallel.h' --- sql/rpl_parallel.h 1970-01-01 00:00:00 +0000 +++ sql/rpl_parallel.h 2013-08-29 08:18:22 +0000 @@ -0,0 +1,92 @@ +#ifndef RPL_PARALLEL_H +#define RPL_PARALLEL_H + +#include "log_event.h" + + +struct rpl_parallel; +struct rpl_parallel_entry; +struct rpl_parallel_thread_pool; + +class Relay_log_info; +struct rpl_parallel_thread { + bool delay_start; I think we can remove deley_start. Se comments from previous review. + bool running; + bool stop; We need to define 'stop' properly. Like 'stop after we have executed the current event group' (So that we don't have to do rollback). + bool free; I think we can remove free. Se comments above. Otherwise it would be good to have a good descripition for each struct in this file. Will now start reviewing your new commits to the parallel replication tree. Regards, Monty

1 0

[Maria-developers] MDEV-5019 - THREADPOOL - Create Information Schema Table for Threadpool
by Roberto Spadim 22 Sep '13

22 Sep '13

Hi guys! I openned a new MDEV https://mariadb.atlassian.net/browse/MDEV-5019 the idea is expose the threadpool group information like oracle thread pool (more information about oracle thread pool here: http://mikaelronstrom.blogspot.com.br/2011_10_01_archive.html) I can help, but i don't know how access the threadpool at scheduler.h/scheduler.c from a information schema plugin 1) lock important mutex and others mdl 2) acess variables 3) write information schema based on variables 1 and 2 i don't know how to do :( can anyone help ? :) thanks guys :) -- Roberto Spadim

2 9

[Maria-developers] Perf tuning for hot key updating with high concurrency
by Hui Liu 22 Sep '13

22 Sep '13

It's an old issue which discussed in Percona-google-group: https://groups.google.com/forum/?fromgroups#!topic/percona-discussion/z3r4-… The patch provided in that discuss group is a rough idea, based on Percona-Server, and later I almost re-wrote the patch for real-case testing. I think it's more simple to implement in the InnoDB layer, compared to the the idea post by Monty, that introduce 'Incremental lock' or 'Relative update lock' to solve this problem. I backported them to Mariadb-10.0.4, and use simple simulate test with oltp_cc.lua, which is provided as an attachment. These notes migth be helpful to understand of this patch. 1) Why InnoDB's concurrency control is BAD for high concurrency hot key updating case? It's described in the percona-disscuss-group, please refer that. 2) What's the basic logic for high concurrency hot key updating? The idea is still the same as before, but the implement is different now. With a simple case that 1000 threads updating the same record id=1 and threshold=2 to describe the basic idea for this patch. conn (1000) ... (6) -> (5) -> (4) (1) (running) (2) (suspended) (3) (suspended) That is, thread 1 is running, and thread 2 and 3 are suspended, waiting for thread 1 relase the record lock for id=1. For thread 4, it detected they are already two threads suspended, and it would be rescheduled in hash queue. conn (1000) .. (6) -> (5) (1) (running) (2) (suspended) (3) (suspended) bucket1: (4) bucket2: And for 5 and 6 ... 1000, they are also rescheduled in that queue (1) (running) (2) (suspended) (3) (suspended) bucket1: (1000)...(6) -> (5) -> (4) bucket2: After 1 is committed, 2 is waked up and running, then 4 detects it would entry now, so it enters. (1) (commit/exit) (2) (running) (3) (suspended) (4) (suspended) bucket1: (1000) ... (6) -> (5) bucket2: The same logic for 5 and 6...1000. While for the old logic, a snapshot might be: (1) (commit/exit) (2) (running) (3) (suspended) (4) (suspended) (5) (suspended) .... (1000) (suspended) Hope the demo is not too naive. You can see, in such case, there are always threshold threads running inside innodb, while not all are suspended inside. 3) Why not use the new concurrency control with AOTMIC add/sub for active conc? The result is bad for new concurrency control in 5.6/10.0, that no FIFO any more. So I changed to default (5.1/5.5) control strategy again. Note the changes in CMakeLists.txt that make -DUSE_DEFAULT_CONC=ON as default. 4) How about the concurrency counter for rescheduled thread in bucket queue? If thread number waiting for the record lock is over the configured threshold, it would be rescheduled into queue in one hash bucket, hashed by (space,space_no, heap_no) a) for the first one in the queue, that is the queue header, it would timed-wait retrying whether it would enter into innodb layer, by checking the n_ref_count for that record stored by HASH. b) for the non-head threads, they would be FAKE released the concurrent slot, that means threads in FIFO would be waked up, enter/exit innodb with faked inside and tickets. Until they are force to exit innodb, the active slot number is equal to real case without rescheduling. 5) What about the basic test result? $ ./sysbench/sysbench --test=sysbench/tests/db/oltp_cc.lua --num-threads=1000 --mysql-socket=/u01/md1004/run/mysql.sock --mysql-db=bmw --oltp-tables-count=4 --mysql-user=root --oltp-table-size=4 --max-requests=0 --max-time=120000 --oltp-read-only=off run 1000conn, oltp-cc.lua, 4x4(16 hotkey): patched (innodb_lock_wait_threshold=12 as default) : 3.1K QPS, 9.1K TPS unpatched (innodb_lock_wait_threshold=0 as disabled) : 1.2K QPS, 3.5K TPS $ ./sysbench/sysbench --test=sysbench/tests/db/oltp_cc.lua --num-threads=1000 --mysql-socket=/u01/md1004/run/mysql.sock --mysql-db=bmw --oltp-tables-count=1 --mysql-user=root --oltp-table-size=4 --max-requests=0 --max-time=120000 --oltp-read-only=off run 1000conn, oltp-cc.lua, 1x4(4 hotkey): patched (innodb_lock_wait_threshold=12 as default) : 1.3K QPS, 3.5K TPS unpatched (innodb_lock_wait_threshold=0 as disabled) : 200 QPS, 720 TPS 6) Any rest issues? The rescheduling of the waiting thread breaks the normal mechanism: each lock request must be registered in lock-table, then deadlock could be found if exists. As I am trying to reduce n in lock relaetd O(n) and O(n^2) operations, for the deadlock case, it does not work very well. For example, the update sequance: t1 updates 1, and t2 updates 1, then t3/t4/t5/t6 update 1, and then t1 updates 2, after that t2 updates 1: t1 1 2 t2 2 1 t3 1 t4 1 t5 1 t6 1 As we can see, in the new schedule model, t3/t4/t5 are suspended, and might be rescheduled in queue. Then t1 is suspended as it updates 2 which record lock is hold by t2. t2 is rescheduled as it detected too many lock waiting for rec 1. The snapshot might be: (t1[2]) (suspended) (t3) (suspended) (t4) (suspended) bucket1: (t2[1]) -> (t6) -> (t5) bucket2: The deadlock can be break until t2 enters with 1. Then for t5/t6, they would wait until timeout, and then force enter into innodb side, suspended as it should be. It's same for t2. Until t2 enters, the deadlock is found. So, the cost for deadlock detecting might be much large than it should be, depends on the timeout and retry counts. Hope Jan or Monty would have a look for this patch, or other ideas for solve this problem, if you are going.

1 0