Hello Jesper,
Thank you for this contribution. It's a good idea to add the
--thread-id flag to mysqlbinlog. The source patch is well written, it
works for both SBL and RBL, and I see no immediate problems with it.
The feature is not very intrusive either; it is unlikely to be in the
way of possible future work such as scripted or pluggable filters.
I would suggest two improvements:
1. If the binary log contains events originating from more than one
server, then it is not safe to filter by only thread_id, because
two unrelated threads on two different servers may have the same
thread_id. Hence, we should generate a warning when --thread-id
is set and --server-id is not set.
2. The test case needs to be improved to cover more cases and to
reduce future maintenance. I will be quite rigorous and require
several changes. Many of our existing tests don't follow such
strict guidelines, but we try to enforce them when we add or
update tests. This is important because we spend a lot of time
maintaining test cases. So we need to write tests that don't need
to be updated when something in the server changes.
1. Test that --thread-id without --server-id gives a warning.
2. Test the same thing for a binary log that contains row events
and a binary log that contains statement events. It may be
easiest to move the test to the mysql-test/suite/binlog/t;
tests in that directory are run once for each binlog_format
(you have to remove 'source
include/have_binlog_format_statement.inc').
3. Don't assume that the binlog name is master-bin.000001. Read
the binlog name like this:
--let $binlog_file= query_get_value(SHOW MASTER STATUS, File, 1)
4. Please avoid relying on comparison with the result file. See
the suggested fixes below: if the feature does not work as
specified, we will get an error in diff_tables instead of a
result file mismatch. This makes the test much easier to
maintain in the future.
5. Test mysqlbinlog with no flag, with --thread-id alone, with
--server-id alone, and with --thread-id and --server-id
together; and in each case with ids that exist in the binlog
and ids that don't exist in the binlog. I'd suggest something
like:
CREATE TABLE t1 (_server_id INT, _thread_id INT);
FLUSH LOGS;
SET @@GLOBAL.SERVER_ID = 1, @@SESSION.PSEUDO_THREAD_ID = 1;
INSERT INTO t1 SET _server_id=1, _thread_id=1;
SET @@GLOBAL.SERVER_ID = 1, @@SESSION.PSEUDO_THREAD_ID = 2;
INSERT INTO t1 SET _server_id=1, _thread_id=2;
SET @@GLOBAL.SERVER_ID = 2, @@SESSION.PSEUDO_THREAD_ID = 1;
INSERT INTO t1 SET _server_id=2, _thread_id=1;
SET @@GLOBAL.SERVER_ID = 2, @@SESSION.PSEUDO_THREAD_ID = 2;
INSERT INTO t1 SET _server_id=2, _thread_id=2;
FLUSH LOGS;
CREATE TABLE data SELECT * FROM t1;
CREATE TABLE t2 LIKE t1;
# Test all combinations of --server-id and --thread-id.
--let $server_id= 0
while ($server_id <= 3)
{
--let $thread_id= 0
while ($thread_id <= 3)
{
--source extra/binlog_tests/binlog_mysqlbinlog_thread_id.inc
--inc $thread_id
}
--inc $server_id
}
Then, create the file
mysql-test/extra/binlog_tests/binlog_mysqlbinlog_thread_id.inc
that contains:
# ==== Purpose ====
#
# Auxiliary file used by binlog_mysqlbinlog_thread_id. Runs
# mysqlbinlog [--thread-id=X] [--server-id=Y]
# and checks that the correct statements were filtered.
#
# ==== Usage ====
#
# --let $thread_id= X
# --let $server_id= Y
# --source extra/binlog_tests/binlog_mysqlbinlog_thread_id.inc
#
# If $thread_id is set, passes --thread-id=$thread_id to
# mysqlbinlog. If $server_id is set, passes
# --server-id=$server_id to mysqlbinlog.
DELETE FROM t1;
DELETE FROM t2;
--let $options=
--let $where=
if ($thread_id)
{
--let $options= $options --thread-id=$thread_id
--let $where= $where _thread_id=$thread_id
}
if ($server_id)
{
--let $options= $options --server-id=$server_id
--let $where= $where _server_id=$server_id
}
if ($where)
{
--let $where= WHERE $where
}
--exec $MYSQL_BINLOG $options $MYSQLD_DATADIR/$binlog_file |
$MYSQL
INSERT INTO t2 SELECT * FROM data $where;
--let $diff_tables= t1, t2
--source include/diff_tables.inc
6. Please add a comment to the top of the test case. Something
like:
# ==== Purpose ====
#
# Verify that mysqlbinlog --thread-id works as expected. The
# flag is tested both alone and in conjuction with --server-id,
# and both with id that exists and that don't exist in the
# binary log. Also test that --thread-id without --server-id
# gives a warning.
#
# ==== Method ====
#
# Create tables t1, t2, and data, with two int columns. Insert
# the current values of pseudo_thread_id and server_id into t1
# for some different values. Record the inserts in a separate
# binlog. Copy the contents of t1 to data and delete all rows
# from t1. Run mysqlbinlog with several different combinations
# of --thread-id and --server-id and pipe the output to a
# client. For each combination, copy the rows that we expect
# mysqlbinlog to filter in from data to t2 and then compare
# t1 and t2.
#
# ==== References ====
#
# BUG#59529: Add support for filtering by thread id in mysqlbinlog
7. We always guarantee that tests start from a clean state, so
there is no need to drop tables or call RESET MASTER at the
beginning of the test.
8. Please don't disable warnings in the test. In the end of the
test, drop exactly the tables that were created, without
IF EXISTS.
9. Please restore the value of SERVER_ID at the end of the test.
Best regards,
Sven Sandberg
Software engineer
Oracle / MySQL Replication team
