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

[Sven Sandberg]

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 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.

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).

>  
>  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.

>  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.

>  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...


-- 
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com