maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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