Re: bzr commit into mysql-5.1 branch (mats:2683) Bug#38360

[Mats Kindahl]

Sven Sandberg wrote:
> Hi Mats,
> 
> I can't judge the changes to the server, will have to ask you next time
> we meet in IRC.
> 
> I think we need a test case for the most typical BLACKHOLE scenario.
> Maybe you can ask Serge to write one?

I actually have one already. Will add it.

> I have some small comments on the test cases changes, see below.
> 
> /Sven
> 
> 
> Mats Kindahl wrote:
>> #At file:///home/bzr/bugs/b38360-5.1-5.1-29-rc/
>>
>>  2683 Mats Kindahl	2008-09-22
>>       Bug #38360: BLACKHOLE replication with RBR is broken
>>       
>>       The Blackhole engine did not support row-based replication
>>       since the delete_row() and update_row() functions were not
>>       implemented.
>>       
>>       This patch adds row-based replication support for the
>>       Blackhole engine by implementing the two functions mentioned
>>       above, and making the engine pretend that it has found the
>>       correct row to delete or update when executed from the slave
>>       SQL thread.
>>       
>>       It is necessary to only pretend this for the SQL thread, since
>>       a SELECT executed on the Blackhole engine will otherwise never
>>       return EOF, causing a livelock.
>> modified:
>>   mysql-test/extra/binlog_tests/blackhole.test
>>   mysql-test/suite/binlog/r/binlog_multi_engine.result
>>   mysql-test/suite/binlog/r/binlog_stm_blackhole.result
>>   mysql-test/suite/binlog/t/binlog_multi_engine.test
>>   sql/log_event.cc
>>   storage/blackhole/ha_blackhole.cc
>>   storage/blackhole/ha_blackhole.h
>>
>> per-file messages:
>>   mysql-test/extra/binlog_tests/blackhole.test
>>     Blackhole now handles row-based replication.
>>   mysql-test/suite/binlog/t/binlog_multi_engine.test
>>     Replication now handles row-based replcation.
>>   sql/log_event.cc
>>     Correcting code to only touch filler bits and leave
>>     all other bits alone. It is necessary since there is
>>     no guarantee that the engine will be able to fill in
>>     the bits correctly (e.g., the blackhole engine).
>>   storage/blackhole/ha_blackhole.cc
>>     Adding definitions for update_row() and delete_row() to return OK
>>     when executed from the slave SQL thread with thd->query == NULL
>>     (indicating that row-based replication events are being processed).
>>     
>>     Changing rnd_next(), index_read(), index_read_idx(), and
>>     index_read_last() to return OK when executed from the slave SQL
>>     thread (faking that the row has been found so that processing
>>     proceeds to update/delete the row).
>>   storage/blackhole/ha_blackhole.h
>>     Enabling row capabilities for engine.
>>     Defining write_row(), update_row(), and delete_row().
>>     Making write_row() private (as it should be).
>> === modified file 'mysql-test/extra/binlog_tests/blackhole.test'
> [...]
>> === modified file 'mysql-test/suite/binlog/r/binlog_multi_engine.result'
> [...]
>> === modified file 'mysql-test/suite/binlog/r/binlog_stm_blackhole.result'
> [...]
>> === modified file 'mysql-test/suite/binlog/t/binlog_multi_engine.test'
>> --- a/mysql-test/suite/binlog/t/binlog_multi_engine.test	2008-02-28 21:50:15
> +0000
>> +++ b/mysql-test/suite/binlog/t/binlog_multi_engine.test	2008-09-22 19:00:29
> +0000
>> @@ -69,8 +69,11 @@ UPDATE t1m, t1n SET m = 2, e = 3 WHERE n
>>  
>>  #UPDATE t1m, t1n SET m = 2, e = 3 WHERE n = f;
>>  
>> -error ER_BINLOG_LOGGING_IMPOSSIBLE;
>> -UPDATE t1n, t1b SET e = 2, b = 3 WHERE f = c;
>> +# Blackhole now supports row-based logging, so we have no engine that
>> +# doesn't support it.
>> +
>> +#error ER_BINLOG_LOGGING_IMPOSSIBLE;
>> +#UPDATE t1n, t1b SET e = 2, b = 3 WHERE f = c;
> 
> Did you really mean to comment out the query? I think only the error
> line needs to be commented out.

No, it was intentional. Yes, it is true that the update passes after the fix,
but the point of the test is to get a error because there is one engine that
does not allow statement-based logging and one that does not allow row-based
logging. Since that is no longer the case for Blackhole, the test is pointless
and does not need to be there. We have plenty of tests of update elsewhere, and
a test for NDB together with another engine that can handle both is also
prominent in the tests.

> 
> Also, I'd suggest to remove this line and not replace it by four other
> lines (no need to refer to problems that are fixed, unless this is the
> regression test for the bug, and the comment will be difficult to parse
> when taken out of context of this bug fix).

OK. Good point.

> 
>>  
>>  TRUNCATE t1m;
>>  TRUNCATE t1b;
>> @@ -83,11 +86,18 @@ RESET MASTER;
>>  SET SESSION BINLOG_FORMAT=ROW;
>>  
>>  INSERT INTO t1m VALUES (1,1), (1,2), (2,1), (2,2);
>> -error ER_BINLOG_LOGGING_IMPOSSIBLE;
>> +
>> +# Blackhole now supports row-based logging, so we have no engine that
>> +# doesn't support it.
>> +
>> +#error ER_BINLOG_LOGGING_IMPOSSIBLE;
> 
> I suggest removing the above five lines.

OK

> 
>>  INSERT INTO t1b VALUES (1,1), (1,2), (2,1), (2,2);
>>  INSERT INTO t1n VALUES (1,1), (1,2), (2,1), (2,2);
>>  
>> -error ER_BINLOG_LOGGING_IMPOSSIBLE;
>> +# Blackhole now supports row-based logging, so we have no engine that
>> +# doesn't support it.
>> +
>> +#error ER_BINLOG_LOGGING_IMPOSSIBLE;
> 
> I suggest removing the above four lines.

OK

> 
>>  UPDATE t1m, t1b SET m = 2, b = 3 WHERE n = c;
>>  error ER_BINLOG_LOGGING_IMPOSSIBLE;
>>  UPDATE t1m, t1n SET m = 2, e = 3 WHERE n = f;
>>
>> === modified file 'sql/log_event.cc'
> [...]
> 
> I'll have to ask you about the server changes in IRC...
> 
> 

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