Mats, hello.
The patch looks good, I have only minor comments about cset comments
and tests. We might have been lucky if changes in log_event.cc
actually fix another bug.
> #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.
Apparently you had to redefine *index*() functions to support blackhole
on the slave side as well.
>
> 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).
Chages in that part does not seem to relate to the current bug, but
maybe they will fix Bug #39753.
> 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'
> --- a/mysql-test/extra/binlog_tests/blackhole.test 2008-03-29 13:00:12 +0000
> +++ b/mysql-test/extra/binlog_tests/blackhole.test 2008-09-22 19:00:29 +0000
> @@ -139,15 +139,6 @@ drop table t1,t2,t3;
> # table
> #
> CREATE TABLE t1(a INT) ENGINE=BLACKHOLE;
> -# NOTE: After exchanging open_ltable() by open_and_lock_tables() in
> -# handle_delayed_insert() to fix problems with MERGE tables (Bug#26379),
> -# problems with INSERT DELAYED and BLACKHOLE popped up. open_ltable()
> -# does not check if the binlogging capabilities of the statement and the
> -# table match. So the below used to succeed. But since INSERT DELAYED
> -# switches to row-based logging in mixed-mode and BLACKHOLE cannot do
> -# row-based logging, it could not really work. Until this problem is
> -# correctly fixed, we have that error here.
> ---error ER_BINLOG_LOGGING_IMPOSSIBLE
> INSERT DELAYED INTO t1 VALUES(1);
> DROP TABLE t1;
>
>
> === modified file 'mysql-test/suite/binlog/r/binlog_multi_engine.result'
> --- a/mysql-test/suite/binlog/r/binlog_multi_engine.result 2008-03-28 12:16:41 +0000
> +++ b/mysql-test/suite/binlog/r/binlog_multi_engine.result 2008-09-22 19:00:29 +0000
> @@ -43,8 +43,6 @@ INSERT INTO t1n VALUES (1,1), (1,2), (2,
> UPDATE t1m, t1b SET m = 2, b = 3 WHERE n = c;
> UPDATE t1m, t1n SET m = 2, e = 3 WHERE n = f;
> ERROR HY000: Binary logging not possible. Message: Statement cannot be written
> atomically since more than one engine involved and at least one engine is self-logging
> -UPDATE t1n, t1b SET e = 2, b = 3 WHERE f = c;
> -ERROR HY000: Binary logging not possible. Message: Statement cannot be written
> atomically since more than one engine involved and at least one engine is self-logging
> TRUNCATE t1m;
> TRUNCATE t1b;
> TRUNCATE t1n;
> @@ -68,20 +66,22 @@ RESET MASTER;
> SET SESSION BINLOG_FORMAT=ROW;
> INSERT INTO t1m VALUES (1,1), (1,2), (2,1), (2,2);
> INSERT INTO t1b VALUES (1,1), (1,2), (2,1), (2,2);
> -ERROR HY000: Binary logging not possible. Message: Row-based format required for
> this statement, but not allowed by this combination of engines
> INSERT INTO t1n VALUES (1,1), (1,2), (2,1), (2,2);
> UPDATE t1m, t1b SET m = 2, b = 3 WHERE n = c;
> -ERROR HY000: Binary logging not possible. Message: Row-based format required for
> this statement, but not allowed by this combination of engines
> UPDATE t1m, t1n SET m = 2, e = 3 WHERE n = f;
> ERROR HY000: Binary logging not possible. Message: Statement cannot be written
> atomically since more than one engine involved and at least one engine is self-logging
> UPDATE t1n, t1b SET e = 2, b = 3 WHERE f = c;
> -ERROR HY000: Binary logging not possible. Message: Row-based format required for
> this statement, but not allowed by this combination of engines
> +ERROR HY000: Binary logging not possible. Message: Statement cannot be written
> atomically since more than one engine involved and at least one engine is self-logging
> show binlog events from <binlog_start>;
> Log_name Pos Event_type Server_id End_log_pos Info
> master-bin.000001 # Query # # use `test`; BEGIN
> master-bin.000001 # Table_map # # table_id: # (test.t1m)
> master-bin.000001 # Write_rows # # table_id: # flags: STMT_END_F
> master-bin.000001 # Query # # use `test`; COMMIT
> +master-bin.000001 # Query # # use `test`; BEGIN
> +master-bin.000001 # Table_map # # table_id: # (test.t1b)
> +master-bin.000001 # Write_rows # # table_id: # flags: STMT_END_F
> +master-bin.000001 # Query # # use `test`; COMMIT
> master-bin.000001 # Query # # BEGIN
> master-bin.000001 # Table_map # # table_id: # (test.t1n)
> master-bin.000001 # Table_map # # table_id: # (mysql.ndb_apply_status)
>
> === modified file 'mysql-test/suite/binlog/r/binlog_stm_blackhole.result'
> --- a/mysql-test/suite/binlog/r/binlog_stm_blackhole.result 2008-03-29 22:54:08
> +0000
> +++ b/mysql-test/suite/binlog/r/binlog_stm_blackhole.result 2008-09-22 19:00:29
> +0000
> @@ -141,7 +141,6 @@ master-bin.000001 # Query # # use `test`
> drop table t1,t2,t3;
> CREATE TABLE t1(a INT) ENGINE=BLACKHOLE;
> INSERT DELAYED INTO t1 VALUES(1);
> -ERROR HY000: Binary logging not possible. Message: Row-based format required for
> this statement, but not allowed by this combination of engines
> DROP TABLE t1;
> CREATE TABLE t1(a INT, b INT) ENGINE=BLACKHOLE;
> DELETE FROM t1 WHERE a=10;
>
> === 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;
I think it's better to remove this snippet instead of commenting-out,
and make sure we have in the test one row-based-loggable sample for each
UPDATE, DELETE, INSERT on a blackhole case.
There is no DELETE case as far as I can see.
>
> 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;
> 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;
> 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'
> --- a/sql/log_event.cc 2008-09-03 10:01:18 +0000
> +++ b/sql/log_event.cc 2008-09-22 19:00:29 +0000
> @@ -8605,10 +8605,10 @@ int Rows_log_event::find_row(const Relay
> the necessary bits on the bytes and don't set the filler bits
> correctly.
> */
> - my_ptrdiff_t const pos=
> - table->s->null_bytes > 0 ? table->s->null_bytes - 1 : 0;
> - table->record[0][pos]= 0xFF;
> -
> + if (table->s->null_bytes > 0)
> + table->record[0][table->s->null_bytes - 1]|=
> + 256U - (1U << table->s->last_null_bit_pos);
> +
As noted earlier, I'd be awesome if you have fixed another bug.
> if ((error= table->file->index_read_map(table->record[0], m_key,
> HA_WHOLE_KEY,
> HA_READ_KEY_EXACT)))
>
> === modified file 'storage/blackhole/ha_blackhole.cc'
> --- a/storage/blackhole/ha_blackhole.cc 2008-03-29 13:00:12 +0000
> +++ b/storage/blackhole/ha_blackhole.cc 2008-09-22 19:00:29 +0000
> @@ -18,6 +18,7 @@
> #pragma implementation // gcc: Class implementation
> #endif
>
> +#define MYSQL_SERVER 1
> #include "mysql_priv.h"
> #include "ha_blackhole.h"
>
> @@ -100,6 +101,24 @@ int ha_blackhole::write_row(uchar * buf)
> DBUG_RETURN(table->next_number_field ? update_auto_increment() : 0);
> }
>
> +int ha_blackhole::update_row(const uchar *old_data, uchar *new_data)
> +{
> + DBUG_ENTER("ha_blackhole::update_row");
> + THD *thd= ha_thd();
> + if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query ==
> NULL)
> + DBUG_RETURN(0);
> + DBUG_RETURN(HA_ERR_WRONG_COMMAND);
> +}
> +
> +int ha_blackhole::delete_row(const uchar *buf)
> +{
> + DBUG_ENTER("ha_blackhole::delete_row");
> + THD *thd= ha_thd();
> + if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query ==
> NULL)
> + DBUG_RETURN(0);
> + DBUG_RETURN(HA_ERR_WRONG_COMMAND);
> +}
> +
> int ha_blackhole::rnd_init(bool scan)
> {
> DBUG_ENTER("ha_blackhole::rnd_init");
> @@ -110,6 +129,9 @@ int ha_blackhole::rnd_init(bool scan)
> int ha_blackhole::rnd_next(uchar *buf)
> {
> DBUG_ENTER("ha_blackhole::rnd_next");
> + THD *thd= ha_thd();
> + if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query ==
> NULL)
> + DBUG_RETURN(0);
> DBUG_RETURN(HA_ERR_END_OF_FILE);
> }
>
> @@ -189,6 +211,9 @@ int ha_blackhole::index_read_map(uchar *
> enum ha_rkey_function find_flag)
> {
> DBUG_ENTER("ha_blackhole::index_read");
> + THD *thd= ha_thd();
> + if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query ==
> NULL)
> + DBUG_RETURN(0);
> DBUG_RETURN(HA_ERR_END_OF_FILE);
> }
>
> @@ -198,6 +223,9 @@ int ha_blackhole::index_read_idx_map(uch
> enum ha_rkey_function find_flag)
> {
> DBUG_ENTER("ha_blackhole::index_read_idx");
> + THD *thd= ha_thd();
> + if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query ==
> NULL)
> + DBUG_RETURN(0);
> DBUG_RETURN(HA_ERR_END_OF_FILE);
> }
>
> @@ -206,6 +234,9 @@ int ha_blackhole::index_read_last_map(uc
> key_part_map keypart_map)
> {
> DBUG_ENTER("ha_blackhole::index_read_last");
> + THD *thd= ha_thd();
> + if (thd->system_thread == SYSTEM_THREAD_SLAVE_SQL && thd->query ==
> NULL)
> + DBUG_RETURN(0);
> DBUG_RETURN(HA_ERR_END_OF_FILE);
> }
>
>
> === modified file 'storage/blackhole/ha_blackhole.h'
> --- a/storage/blackhole/ha_blackhole.h 2007-11-16 10:49:59 +0000
> +++ b/storage/blackhole/ha_blackhole.h 2008-09-22 19:00:29 +0000
> @@ -53,7 +53,7 @@ public:
> ulonglong table_flags() const
> {
> return(HA_NULL_IN_KEY | HA_CAN_FULLTEXT | HA_CAN_SQL_HANDLER |
> - HA_BINLOG_STMT_CAPABLE |
> + HA_BINLOG_STMT_CAPABLE | HA_BINLOG_ROW_CAPABLE |
> HA_CAN_INDEX_BLOBS | HA_AUTO_PART_KEY |
> HA_FILE_BASED | HA_CAN_GEOMETRY | HA_CAN_INSERT_DELAYED);
> }
> @@ -72,7 +72,6 @@ public:
> uint max_supported_key_part_length() const { return BLACKHOLE_MAX_KEY_LENGTH; }
> int open(const char *name, int mode, uint test_if_locked);
> int close(void);
> - int write_row(uchar * buf);
> int rnd_init(bool scan);
> int rnd_next(uchar *buf);
> int rnd_pos(uchar * buf, uchar *pos);
> @@ -94,4 +93,8 @@ public:
> THR_LOCK_DATA **store_lock(THD *thd,
> THR_LOCK_DATA **to,
> enum thr_lock_type lock_type);
> +private:
> + virtual int write_row(uchar *buf);
> + virtual int update_row(const uchar *old_data, uchar *new_data);
> + virtual int delete_row(const uchar *buf);
> };
>
>
regards,
Andrei
