Re: bk commit into 5.0 tree (mats:1.2544) BUG#12691

[Mats Kindahl]

Hi Sven!

Thanks for the review. I'll fix the things I name below, and then I will 
push.

Just my few cents,
Mats Kindahl

Sven Sandberg wrote:
> Hi Mats,
>
> The fix is fine and seems to solve the problem. My comments are mostly 
> about commenting the test file. OK to push if you fix these.
>
> /Sven
>
>
> Mats Kindahl wrote:

[snip]

>> diff -Nrup a/mysql-test/t/rpl_slave_skip.test 
>> b/mysql-test/t/rpl_slave_skip.test
>> --- /dev/null    Wed Dec 31 16:00:00 196900
>> +++ b/mysql-test/t/rpl_slave_skip.test    2007-10-26 09:41:56 +02:00
>> @@ -0,0 +1,136 @@
>
> Please add a comment explaining what the test does at a high level, 
> including what each is different in each of the three parts.
>
> It is a bit difficult to see where one part of the test begins and 
> next test ends. Can you add some sort of header? (e.g.,
> --echo **** TEST 1: autocommit, transactional tables ****
> , etc)

Sure.

[snip]

>> +
>> +# Testing with using autocommit instead. It should still write a BEGIN
>> +# event, so the behaviour should be the same
>
> Testing *without* autocommit

Right.

[snip]
>> +
>> +--echo **** On Master ****
>> +connection master;
>> +disable_warnings;
>> +BEGIN;
>> +INSERT INTO t1 VALUES (1, 'master');
>> +INSERT INTO t2 VALUES (2, 'master');
>> +INSERT INTO t1 VALUES (3, 'master');
>> +ROLLBACK;
>> +
>> +BEGIN;
>> +INSERT INTO t1 VALUES (4, 'master,slave');
>> +INSERT INTO t2 VALUES (5, 'master,slave');
>> +INSERT INTO t1 VALUES (6, 'master,slave');
>> +ROLLBACK;
>
> Is the purpose of column b to indicate which server the row is 
> supposed to exist on? In that case, the text above should be changed 
> to something like

Well, kind of. Your suggested changes make sense.

>
> +INSERT INTO t1 VALUES (1, '');
> +INSERT INTO t2 VALUES (2, 'master');
> +INSERT INTO t1 VALUES (3, '');
> +ROLLBACK;
> +
> +BEGIN;
> +INSERT INTO t1 VALUES (4, '');
> +INSERT INTO t2 VALUES (5, 'master,slave');
> +INSERT INTO t1 VALUES (6, '');
>
>> +enable_warnings;
>> +
>> +save_master_pos;
>> +
>> +# SHOW BINLOG EVENTS;
>
> I suggest to remove this comment.

[snip]

>
>> +      switch (type_code)
>> +      {
>> +      case QUERY_EVENT:
>> +      {
>> +        Query_log_event* const qev= (Query_log_event*) ev;
>> +        DBUG_PRINT("info", ("QUERY_EVENT { query: '%s', q_len: %u }",
>> +                            qev->query, qev->q_len));
>> +        if (memcmp("BEGIN", qev->query, qev->q_len) == 0)
>> +          thd->options|= OPTION_BEGIN;
>> +        else if (memcmp("COMMIT", qev->query, qev->q_len) == 0 ||
>> +                 memcmp("ROLLBACK", qev->query, qev->q_len) == 0)
>
> Is it safe to assume that no other query can begin with "COMMIT", 
> "ROLLBACK", or "BEGIN"? Otherwise, it should compare qev->q_len+1 bytes.

Right, I'll fix that.

-- 
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com