Brandon Nesterenko
were some additional synchronization changes that went alongside the use of LOCK_thd_data, so it would be good if you could go over at least the code changes again.
diff --git a/sql/sql_repl.cc b/sql/sql_repl.cc index 18a52dc07ad..47f789de751 100644 --- a/sql/sql_repl.cc +++ b/sql/sql_repl.cc @@ -2294,10 +2294,19 @@ send_event_to_slave(binlog_send_info *info, Log_event_type event_type,
if (info->thd->slave_info) { - strncpy(info->thd->slave_info->gtid_state_sent.log_file, - info->log_file_name + info->dirlen, - strlen(info->log_file_name) - info->dirlen); - info->thd->slave_info->gtid_state_sent.log_pos= pos; + char *new_log_flie= info->log_file_name + info->dirlen; + size_t log_file_size= strlen(info->log_file_name) - info->dirlen;
Hm, spelling mistake s/new_log_flie/new_log_file/. And you can simpler do just: size_t log_file_size= strlen(new_log_file); But more importantly, I almost didn't want to mention the need for locking around this, it's just more overhead, that is going to be added for _every_ transaction in _every_ slave dump thread. Is it really worth it to add this overhead? Isn't all of the info added to SHOW SLAVE STATUS already available directly on the slave? Just query the slave with SHOW SLAVE STATUS and all the information should already be available, without introducing ever more overhead in the replication layer. I browsed through the comments in https://jira.mariadb.org/browse/MDEV-21322, but didn't see any justification for this over just querying the slaves and avoiding the overhead. Also I don't recall seeing any architecture review / discussion of this? So is this MDEV even needed / justified to add to the server?
+--let $nextval= `SELECT max(a)+1 from t1` +--eval insert into t1 values ($nextval)
Is there a reason here to use separate --let and --eval? And not just:
INSERT INTO t1 SELECT max(a)+1 FROM t1;
I like seeing the actual number value for cross referencing the values potentially in the table (e.g. if manually debugging and running a SELECT).
Ok, sure, then it's fine.
Again, sync_with_master_gtid.inc on the slave will not guarantee that the master saw the ack yet, even though it will be a rare race when it doesn't happen. So you'd need to wait for the expected value here with wait_condition.inc or somilar instead, I think. Probably the same in subsequent parts of the test case.
This one is guaranteed that the master saw the ACK implicitly, as the transaction has returned from commit (which only happens in the case of ACK received, or timeout (which would invalidate the test anyway)).
Ah, yes, I missed that. Ok, that probably apply in a number of places I otherwise thought could be non-deterministic.
Right.I tried pretty hard up-front to make the test as deterministic as possible, though I did fix one more spot based on your previous catch of rpl_semi_sync_master_clients not in sync with Sync_Status :)
One thing that I have found effective is to run eg: ./mysql-test-run.pl --mem rpl.rpl_show_slave_hosts{,,,} --parallel=12 --repeat=1000 and just let it run overnight or something, I've found that tends to trigger most non-deterministic failures that will be seen in buildbot (though not all). This for my 4-core/8-thread laptop. - Kristian.