Hi, Kristian! On Jul 03, Kristian Nielsen wrote:
=== modified file 'storage/innobase/lock/lock0lock.cc' --- storage/innobase/lock/lock0lock.cc 2014-02-26 18:22:48 +0000 +++ storage/innobase/lock/lock0lock.cc 2014-06-10 17:48:32 +0000 @@ -1020,6 +1021,28 @@ lock_rec_has_to_wait( return(FALSE); }
+ if ((type_mode & LOCK_GAP || lock_rec_get_gap(lock2)) && + !thd_need_ordering_with(trx->mysql_thd, + lock2->trx->mysql_thd)) { + /* If the upper server layer has already decided on the + commit order between the transaction requesting the + lock and the transaction owning the lock, we do not + need to wait for gap locks. Such ordeering by the upper + server layer happens in parallel replication, where the + commit order is fixed to match the original order on the + master. + + Such gap locks are mainly needed to get serialisability + between transactions so that they will be binlogged in + the correct order so that statement-based replication + will give the correct results. Since the right order + was already determined on the master, we do not need + to enforce it again here (and doing so could lead to + occasional deadlocks). */
Or do you think we should omit this part of the patch, not optimise this particular case, and instead just rely on the general fallback of detecting a deadlock and killing+retrying?
Nope, that's fine. Just the documentation of thd_need_ordering_with() (which is, the comment right above the prototype) should explain that this is an optional method that, that allows the engine to optimize away deadlocks in these cases, etc.
=== modified file 'sql/log.cc' --- sql/log.cc 2014-03-23 19:09:38 +0000 +++ sql/log.cc 2014-06-10 17:48:32 +0000 + if (!ir->completed || ir->dequeued_count < ir->queued_count) + { + included= false; + break; + } + if (!included && 0 == strcmp(ir->name, rli->group_relay_log_name))
do we ever use this reverse style in the code (const == expr)? I don't remember that
I sometimes use it to make it clearer that it's "compare strings for equality".
It was not clear if you wanted me to change it to !strcmp(...) ? Or if you were just asking?
I know there's a coding style when a condition is written as if (5 == a) with the reasoning that it helps to avoid bugs when one mistakely writes an assignment instead of a comparison. Others say it looks ugly. We don't do this in mariadb sources, so that line of your will clearly stand apart in the code. I'd suggest to use one of strcmp(...) == 0 !strcmp(...) strequal(...) the latter implies #define strequal(X,Y) !strcmp(X,Y) somewhere in headers. I've used this trick few times, but, apparently, not in mysql.
@@ -7284,28 +7321,34 @@ int Xid_log_event::do_apply_event(rpl_gr bool res; int err; rpl_gtid gtid; - uint64 sub_id; + uint64 sub_id= 0; Relay_log_info const *rli= rgi->rli;
+ mysql_reset_thd_for_next_command(thd);
hmm. mysql_reset_thd_for_next_command() is called before the new sql statement, not at the end of the old one. So, you end up calling it twice. It is not a function to use when you only need to reset the error status. The same, I suppose, applies to the previous chunk, Gtid_log_event::do_apply_event
So my problem here is a lack of knowledge of how things should be done correctly.
I think the issue here is that I added code that updates the mysql.gtid_slave_pos table, before doing the actual commit of the Xid_log_event.
This table update opens tables, it can auto-commit a transaction, it can throw errors. Thus it does much of what is involved in "a new sql statement".
Before I added this call, I got various assertions, like about ok status already being set if I gave an error, stuff like that.
So if mysql_reset_thd_for_next_command() is not correct, what should I do instead?
Ah... okay, in this case, please, put it in a comment above mysql_reset_thd_for_next_command(). Regards, Sergei