List:Commits« Previous MessageNext Message »
From:Andrei Elkin Date:March 28 2008 10:11am
Subject:Re: bk commit into 5.1 tree (mats:1.2559) BUG#29020
View as plain text  

> Andrei Elkin wrote:
>> Mats, hello.
>> Thanks for acknowledging my earlier comments.
>> This mail explains some of my questions.
>> There remain two outstanding of them.
>>>> I think you need to mention some facts about user visible behavior
>>>> like possible appearence of redundant COMMIT/ROLLBACK in binlog
>>>> (discussed further).
>>> Well... I think not. This is the result of the patch Sven did and is
>>> not really related to this patch (at least not very much).
>> Please do not confuse with autocommit-ON commit that patch made. I was
>> rather about the content of binlog after making e.g an insert into a
>> non-ta table.
> Hmmm... I was actually thinking of the patch for BUG#29288, but I see
> that is not pushed to the team tree.
>> After your current patch it gains more BEGIN/COMMIT braces:
> Yes, and this is intentional.

Then please do not forget to mention that in cset's comments.

>>>>  binlog_commit()'s argument
>>>>   @param all   true if this is the last statement before a COMMIT
>>>>                statement;                               false
>>>> if either this is a statement in a
>>>>                transaction but not the last, or if this is a statement
>>>>                not inside a BEGIN block and autocommit is on.
>>>> the false's meaning has extended with your patch. Could you please update
> the
>>>> comments?
>>> Well... actually, it changed with the introduction of patch for
>>> BUG#12713, but I will update the comment.
>> I thought post-bug#12713 server executed binlog_commit() same way as
>> the pre- version.
>> That's your patch forces committing even though `all' being false.
> No, but AIUI, with the introduction of the patch for BUG#12713, every
> statement is terminated with a statement commit, and every real
> transaction is terminated with a transaction commit. Previously, the
> last statement commit could be skipped, but it shouldn't be skipped
> any more.

What i meant is that either before and after bug#12717 fixes
binlog_commit(all==false) does not do any effective work.
But with your fixes the function gains an extra rule and starts handling
all==false with doing some work.

> What do you mean by "forces committing even though `all' being false"?
> Of course we can commit a statement (and even regardless of whether it
> is part of a transaction or not).

I meant the technical side of committing.

In either format an autocommit-ON query on a trans table has got
committed to binlog via 

TC_LOG_BINLOG::log_xid ->
bypassing (effectively - not making any
work, not actually - still being invoked) binlog_commit(all==FALSE).

Should not be there any tricks related to XA, all==TRUE would be
passed to binlog_commit even though it's the autocommit=ON query not a
part of multi-statement transaction.

So, that's your changes force committing - flushing the cache to the
binlog file - when all==false.

>>>> the original logics do not mention  (thd->options &
>>>> in the following if arglist.
>>>> What is the reason on add it?
>>> It was there from the beginning in both binlog_rollback() and
>>> binlog_savepoint_rollback(), so I have just added that as a
>>> condition. The option is used to force the statement to be written to
>>> the binary log in the case of a rollback, so it may be partially
>>> redundant (it's main purpose seems to be to ensure that DROP TEMPORARY
>>> TABLE and CREATE TEMPORARY TABLE goes to the binary log). I think we
>>> should go over it's usage, but that is beyond this patch.
>> But that's binlog_commit() that i have meant, and it did no have the
>> option referred.
> True. As I mentioned on IRC, I tried not using it and semantically
> there is no difference. There is a difference in the binary log in
> that the BEGIN is logged either as "use test; BEGIN" or as just
> "BEGIN". Safer to keep it the way it was though, so I'll change it to
> the original use of OPTION_KEEP_LOG (i.e., not use it).


>>>>> +      in_transaction &&
>>>>> +      (all
>>>>> +       ||
>>>>> +       (!trx_data->at_least_one_stmt &&
>>>>> +        thd->transaction.stmt.modified_non_trans_table)) ||
>>>> Previously binlog_commit called binlog_end_trans(thd, trx_data,
> &qev)
>>>> having as warrants to do that either
>>>>    all or ! in_transaction
>>>> Now i see that in the case
>>>>    in_transaction is true,
>>>>    all is false,
>>>>    the current statement has made what the identifier says - 
>>>>    thd->transaction.stmt.modified_non_trans_table
>>>>    and the trans cache is empty 
>>>> binlog_commit() is going to inject COMMIT into the binlog.
>>>> And what is a use case scenario for such combination?


>> I think you mean that if the first query after BEGIN changes on a
>> non-ta table the query binlogged ahead of BEGIN with binlog format
>> stmt, correct?
> Yes.
>> But there was no special reason behind that policy. It has remained for
>> statement format as a consequence of binlogging algirthm which starts
>> filling the trans cache at first appearence of a transactional table.

> The reason for this policy is that there is no obvious reason to hold
> not send that change over to the slave since it has already
> occurred. The following scenario would fail if we did not do that
> (both threads are on the master):
> T-1 T-2
> W(t) |
> | W(t)
> | |
> | C
> R(t)
> W(n)
> |
> C
> This scenario would move the T-2 W(t) ahead of the T-1 W(t), and the
> only reason that we do *not* write all non-transactional writes
> directly to the binary log is because it would potentially cause the
> slave to stop hard with a syntax error.

I know that, but neither original statement replication nor your
attempt to mimic that behaviour help with Bug#28976 Mixing trans and
non-trans tables in one transaction results in incorrect binlog, the
bug case you must be perfectly aware of.

>> With your fixes, I believe, we could do not copy that statement tricky
>> behaviour and to put the first non-ta query row-events into the cache
>> to appear in binlog after BEGIN.
>> Getting back to that if
>> +  if ((thd->options & OPTION_KEEP_LOG) ||
>> +      in_transaction &&
>> +      (all ||
>> +       (!trx_data->at_least_one_stmt &&
>> +        thd->transaction.stmt.modified_non_trans_table)) ||
>> +      !in_transaction && !all)
>> Why should not it be just as it was before:
>> -  if (all || !in_transaction)
>> ?
> Because that would break the case I give above.

I think we should not try to mimic statement format binloging of mixed
transaction. To repeat, it was a consequence of the design of
accumulating and flushing the trans cache. It happens to be viewed
as an "optimization", but it is not less a confusion nor a remedy
against bug#28976.

But simplyfying the if condition we will gain less troubles in future as
well as will remove pieces of your patch - at_least_one_stmt.

>> You principal use case of ac=ON query on a not-ta table with row-based
>> format satisfy.
>> All can be false for 
>> format.
>> Then in_transaction == false and the rows binlog.
>> Yes, there will be "difference"  in the order of binlogging comparing
>> with stmt format. However, indeed that would be correction of the order of
>> binlogging.

> The AUTO_COMMIT=ON case has to do with writing BEGIN/COMMIT for each
> transactional unit.

In above paragraph I meant that the original condition 

 if (all || !in_transaction)

satisfy to your changes. There will be only the difference with statement
format mixed trasaction binlogging wrt to the first (next to BEGIN)
query(s) of the mixed transaction (if it is on a non-ta table).


bk commit into 5.1 tree (mats:1.2559) BUG#29020Mats Kindahl25 Mar
  • Re: bk commit into 5.1 tree (mats:1.2559) BUG#29020Andrei Elkin26 Mar
    • Re: bk commit into 5.1 tree (mats:1.2559) BUG#29020Mats Kindahl27 Mar
      • Re: bk commit into 5.1 tree (mats:1.2559) BUG#29020Andrei Elkin27 Mar
        • Re: bk commit into 5.1 tree (mats:1.2559) BUG#29020Mats Kindahl28 Mar
          • Re: bk commit into 5.1 tree (mats:1.2559) BUG#29020Andrei Elkin28 Mar
            • Re: bk commit into 5.1 tree (mats:1.2559) BUG#29020Mats Kindahl28 Mar
  • RE: bk commit into 5.1 tree (mats:1.2559) BUG#29020Chuck Bell26 Mar
    • Re: bk commit into 5.1 tree (mats:1.2559) BUG#29020Mats Kindahl27 Mar
  • RE: bk commit into 5.1 tree (mats:1.2559) BUG#29020Chuck Bell26 Mar
    • Re: bk commit into 5.1 tree (mats:1.2559) BUG#29020Mats Kindahl27 Mar