List:Internals« Previous MessageNext Message »
From:Sven Sandberg Date:March 15 2011 6:14pm
Subject:Re: Review of contributed patch for BUG#59529
View as plain text  
Hi Jesper,

Thank you for finding BUG#59530. The bug only needs a three-line fix, 
see below. I will fix it, but it may take some time for the patch to 
come through to the public source trees.

The problem appears when the --server-id option prevents mysqlbinlog 
from processing the initial Format_description_log_event. It is 
essential to process the Format_description_log_event before other 
events are processed. So the solution is to never filter out 
Format_description_log_events. Here is the patch:

=== modified file 'client/mysqlbinlog.cc'
--- client/mysqlbinlog.cc	2011-01-18 12:09:03 +0000
+++ client/mysqlbinlog.cc	2011-03-15 15:59:23 +0000
@@ -683,10 +683,10 @@ Exit_status process_event(PRINT_EVENT_IN
        */
        start_datetime= 0;
        offset= 0; // print everything and protect against cycling rec_count
+      if (server_id && (server_id != ev->server_id))
+        /* skip just this event, continue processing the log. */
+        goto end;
      }
-    if (server_id && (server_id != ev->server_id))
-      /* skip just this event, continue processing the log. */
-      goto end;
      if (((my_time_t)(ev->when) >= stop_datetime)
          || (pos >= stop_position_mot))
      {


You will also have to do something similar in your patch: --thread-id 
should not be applied to Format_description_log_events. So your patch 
plus the above patch should look like this:

@@ -683,10 +685,21 @@ Exit_status process_event(PRINT_EVENT_IN
        */
        start_datetime= 0;
        offset= 0; // print everything and protect against cycling rec_count
+      if (server_id && (server_id != ev->server_id))
+        /* skip just this event, continue processing the log. */
+        goto end;
+      if (thread_id) {
+        if (ev_type == QUERY_EVENT)
+        {
+          current_thread_id = ((Query_log_event*)ev)->thread_id;
+        }
+        if (current_thread_id && (thread_id != current_thread_id))
+        {
+          /* skip just this event, continue processing the log. */
+          goto end;
+        }
+      }
      }
-    if (server_id && (server_id != ev->server_id))
-      /* skip just this event, continue processing the log. */
-      goto end;
      if (((my_time_t)(ev->when) >= stop_datetime)
          || (pos >= stop_position_mot))
      {


Best regards,
Sven Sandberg
Software engineer
Oracle / MySQL Replication team


On 03/06/2011 01:20 AM, Jesper Wisborg Krogh wrote:
> Hi Sven,
>
> Thanks for your feedback. Your suggestions sounds reasonable and I will take a look
> at it.
>
> There is one issue with adding the test cases for filtering by server id. I actually
> initially was looking at adding some tests that included filtering by server id, however
> the result was that I discovered BUG#59530, so those tests will in general not pass.
>
> Thanks,
> Jesper
>
> On 01/03/2011, at 8:23 PM, Sven Sandberg wrote:
>
>> 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
>

Thread
Review of contributed patch for BUG#59529Sven Sandberg1 Mar
  • Re: Review of contributed patch for BUG#59529Jesper Wisborg Krogh6 Mar
    • Re: Review of contributed patch for BUG#59529Sven Sandberg15 Mar