← Back to team overview

maria-developers team mailing list archive

Re: [Commits] ca24c9d: MDEV-9383: Server fails to read master.info after upgrade 10.0 -> 10.1

 

Hi Kristian!

Ok to push. (see a suggestion inline)

Best,
Nirbhay


On Thu, Apr 7, 2016 at 8:55 AM, Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx>
wrote:

> Hi Nirbhay,
>
> Do you want to review this patch for MDEV-9383?
> It concerns your code added for do_domain_ids=(...).
>
>  - Kristian.
>
> Kristian Nielsen <knielsen@xxxxxxxxxxxxxxx> writes:
>
> > revision-id: ca24c9d1671e90e43daa483af32fc19891d1a646
> (mariadb-10.1.13-8-gca24c9d)
> > parent(s): 4b6a3518e4dc9088d1f42cd9bc487d06137d2760
> > committer: Kristian Nielsen
> > timestamp: 2016-04-07 14:44:29 +0200
> > message:
> >
> > MDEV-9383: Server fails to read master.info after upgrade 10.0 -> 10.1
> >
> > In some cases, MariaDB 10.0 could write a master.info file that was read
> > incorrectly by 10.1 and could cause server to fail to start after an
> upgrade.
> >
> > (If writing a new master.info file that is shorter than the old, extra
> > junk may remain at the end of the file. This is handled properly in
> > 10.1 with an END_MARKER line, but this line is not written by
> > 10.0. The fix here is to make 10.1 robust at reading the master.info
> > files written by 10.0).
> >
> > Fix several things around reading master.info and
> read_mi_key_from_file():
> >
> >  - read_mi_key_from_file() did not distinguish between a line with and
> >    without an eqals '=' sign.
> >
> >  - If a line was empty, read_mi_key_from_file() would incorrectly return
> >    the key from the previous call.
> >
> >  - An extra using_gtid=X line left-over by MariaDB 10.0 might incorrectly
> >    be read and overwrite the correct value.
> >
> >  - Fix incorrect usage of strncmp() which should be strcmp().
> >
> >  - Add test cases.
> >
> > ---
> >  mysql-test/std_data/bad2_master.info               |  35 +++++
> >  mysql-test/std_data/bad3_master.info               |  37 +++++
> >  mysql-test/std_data/bad4_master.info               |  35 +++++
> >  mysql-test/std_data/bad5_master.info               |  35 +++++
> >  mysql-test/std_data/bad6_master.info               |  36 +++++
> >  mysql-test/std_data/bad_master.info                |  35 +++++
> >  .../suite/rpl/r/rpl_upgrade_master_info.result     |  88 +++++++++++
> >  .../suite/rpl/t/rpl_upgrade_master_info.test       | 163
> +++++++++++++++++++++
> >  sql/rpl_mi.cc                                      |  81 +++++-----
> >  9 files changed, 512 insertions(+), 33 deletions(-)
> >
> > diff --git a/mysql-test/std_data/bad2_master.info b/mysql-test/std_data/
> bad2_master.info
> > new file mode 100644
> > index 0000000..6172256
> > --- /dev/null
> > +++ b/mysql-test/std_data/bad2_master.info
> > @@ -0,0 +1,35 @@
> > +33
> > +mysql-bin.000001
> > +4
> > +127.0.0.1
> > +root
> > +
> > +3310
> > +60
> > +0
> > +
> > +
> > +
> > +
> > +
> > +0
> > +1800.000
> > +
> > +0
> > +
> > +0
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +using_gtid=1
> > +=0
> > diff --git a/mysql-test/std_data/bad3_master.info b/mysql-test/std_data/
> bad3_master.info
> > new file mode 100644
> > index 0000000..6e632cd
> > --- /dev/null
> > +++ b/mysql-test/std_data/bad3_master.info
> > @@ -0,0 +1,37 @@
> > +33
> > +mysql-bin.000001
> > +4
> > +127.0.0.1
> > +root
> > +
> > +3310
> > +60
> > +0
> > +
> > +
> > +
> > +
> > +
> > +0
> > +1800.000
> > +
> > +0
> > +
> > +0
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +using_gtid=1
> > +
> > +
> > +0
> > diff --git a/mysql-test/std_data/bad4_master.info b/mysql-test/std_data/
> bad4_master.info
> > new file mode 100644
> > index 0000000..87572ef
> > --- /dev/null
> > +++ b/mysql-test/std_data/bad4_master.info
> > @@ -0,0 +1,35 @@
> > +33
> > +mysql-bin.000001
> > +4
> > +127.0.0.1
> > +root
> > +
> > +3310
> > +60
> > +0
> > +
> > +
> > +
> > +
> > +
> > +0
> > +1800.000
> > +
> > +0
> > +
> > +0
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +using_gtid=1
> > +d=1
> > diff --git a/mysql-test/std_data/bad5_master.info b/mysql-test/std_data/
> bad5_master.info
> > new file mode 100644
> > index 0000000..4ea8113
> > --- /dev/null
> > +++ b/mysql-test/std_data/bad5_master.info
> > @@ -0,0 +1,35 @@
> > +33
> > +mysql-bin.000001
> > +4
> > +127.0.0.1
> > +root
> > +
> > +3310
> > +60
> > +0
> > +
> > +
> > +
> > +
> > +
> > +0
> > +1800.000
> > +
> > +0
> > +
> > +0
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +using_gtid=1
> > +using_gtid
> > diff --git a/mysql-test/std_data/bad6_master.info b/mysql-test/std_data/
> bad6_master.info
> > new file mode 100644
> > index 0000000..0f48f48
> > --- /dev/null
> > +++ b/mysql-test/std_data/bad6_master.info
> > @@ -0,0 +1,36 @@
> > +33
> > +mysql-bin.000001
> > +4
> > +127.0.0.1
> > +root
> > +
> > +3310
> > +60
> > +0
> > +
> > +
> > +
> > +
> > +
> > +0
> > +1800.000
> > +
> > +0
> > +
> > +0
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +using_gtid=1
> > +END_MARKER
> > +do_domain_ids=20 Hulubulu!!?!
> > diff --git a/mysql-test/std_data/bad_master.info b/mysql-test/std_data/
> bad_master.info
> > new file mode 100644
> > index 0000000..1541fdf
> > --- /dev/null
> > +++ b/mysql-test/std_data/bad_master.info
> > @@ -0,0 +1,35 @@
> > +33
> > +mysql-bin.000001
> > +4
> > +127.0.0.1
> > +root
> > +
> > +3310
> > +60
> > +0
> > +
> > +
> > +
> > +
> > +
> > +0
> > +1800.000
> > +
> > +0
> > +
> > +0
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +
> > +using_gtid=1
> > +
> > diff --git a/mysql-test/suite/rpl/r/rpl_upgrade_master_info.result
> b/mysql-test/suite/rpl/r/rpl_upgrade_master_info.result
> > new file mode 100644
> > index 0000000..f966f18
> > --- /dev/null
> > +++ b/mysql-test/suite/rpl/r/rpl_upgrade_master_info.result
> > @@ -0,0 +1,88 @@
> > +include/master-slave.inc
> > +[connection master]
> > +*** MDEV-9383: Server fails to read master.info after upgrade 10.0 ->
> 10.1 ***
> > +include/stop_slave.inc
> > +CHANGE MASTER TO master_use_gtid=CURRENT_POS;
> > +include/rpl_stop_server.inc [server_number=2]
> > +include/rpl_start_server.inc [server_number=2]
> > +CREATE TABLE t1 (a INT PRIMARY KEY);
> > +INSERT INTO t1 VALUES (1);
> > +include/save_master_gtid.inc
> > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1;
> > +include/start_slave.inc
> > +include/sync_with_master_gtid.inc
> > +SELECT * FROM t1;
> > +a
> > +1
> > +include/stop_slave.inc
> > +include/rpl_stop_server.inc [server_number=2]
> > +include/rpl_start_server.inc [server_number=2]
> > +INSERT INTO t1 VALUES (2);
> > +include/save_master_gtid.inc
> > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1;
> > +include/start_slave.inc
> > +include/sync_with_master_gtid.inc
> > +SELECT * FROM t1 ORDER BY a;
> > +a
> > +1
> > +2
> > +include/stop_slave.inc
> > +include/rpl_stop_server.inc [server_number=2]
> > +include/rpl_start_server.inc [server_number=2]
> > +INSERT INTO t1 VALUES (3);
> > +include/save_master_gtid.inc
> > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1;
> > +include/start_slave.inc
> > +include/sync_with_master_gtid.inc
> > +SELECT * FROM t1 ORDER BY a;
> > +a
> > +1
> > +2
> > +3
> > +include/stop_slave.inc
> > +include/rpl_stop_server.inc [server_number=2]
> > +include/rpl_start_server.inc [server_number=2]
> > +INSERT INTO t1 VALUES (4);
> > +include/save_master_gtid.inc
> > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1;
> > +include/start_slave.inc
> > +include/sync_with_master_gtid.inc
> > +SELECT * FROM t1 ORDER BY a;
> > +a
> > +1
> > +2
> > +3
> > +4
> > +include/stop_slave.inc
> > +include/rpl_stop_server.inc [server_number=2]
> > +include/rpl_start_server.inc [server_number=2]
> > +INSERT INTO t1 VALUES (5);
> > +include/save_master_gtid.inc
> > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1;
> > +include/start_slave.inc
> > +include/sync_with_master_gtid.inc
> > +SELECT * FROM t1 ORDER BY a;
> > +a
> > +1
> > +2
> > +3
> > +4
> > +5
> > +include/stop_slave.inc
> > +include/rpl_stop_server.inc [server_number=2]
> > +include/rpl_start_server.inc [server_number=2]
> > +INSERT INTO t1 VALUES (6);
> > +include/save_master_gtid.inc
> > +CHANGE MASTER TO master_host='127.0.0.1', master_port=SERVER_MYPORT_1;
> > +include/start_slave.inc
> > +include/sync_with_master_gtid.inc
> > +SELECT * FROM t1 ORDER BY a;
> > +a
> > +1
> > +2
> > +3
> > +4
> > +5
> > +6
> > +DROP TABLE t1;
> > +include/rpl_end.inc
> > diff --git a/mysql-test/suite/rpl/t/rpl_upgrade_master_info.test
> b/mysql-test/suite/rpl/t/rpl_upgrade_master_info.test
> > new file mode 100644
> > index 0000000..e81e7c0
> > --- /dev/null
> > +++ b/mysql-test/suite/rpl/t/rpl_upgrade_master_info.test
> > @@ -0,0 +1,163 @@
> > +--source include/master-slave.inc
> > +
> > +--echo *** MDEV-9383: Server fails to read master.info after upgrade
> 10.0 -> 10.1 ***
> > +
> > +--connection slave
> > +--source include/stop_slave.inc
> > +CHANGE MASTER TO master_use_gtid=CURRENT_POS;
> > +--let $datadir= `SELECT @@datadir`
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_stop_server.inc
> > +
> > +--remove_file $datadir/master.info
> > +--copy_file $MYSQL_TEST_DIR/std_data/bad_master.info $datadir/
> master.info
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_start_server.inc
> > +
> > +--source include/wait_until_connected_again.inc
> > +
> > +--connection master
> > +CREATE TABLE t1 (a INT PRIMARY KEY);
> > +INSERT INTO t1 VALUES (1);
> > +--source include/save_master_gtid.inc
> > +
> > +--connection slave
> > +# Fix the port after we replaced master.info.
> > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1
> > +eval CHANGE MASTER TO master_host='127.0.0.1',
> master_port=$SERVER_MYPORT_1;
> > +--source include/start_slave.inc
> > +--source include/sync_with_master_gtid.inc
> > +SELECT * FROM t1;
> > +
> > +--source include/stop_slave.inc
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_stop_server.inc
> > +
> > +--remove_file $datadir/master.info
> > +--copy_file $MYSQL_TEST_DIR/std_data/bad2_master.info $datadir/
> master.info
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_start_server.inc
> > +
> > +--source include/wait_until_connected_again.inc
> > +
> > +--connection master
> > +INSERT INTO t1 VALUES (2);
> > +--source include/save_master_gtid.inc
> > +
> > +--connection slave
> > +# Fix the port after we replaced master.info.
> > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1
> > +eval CHANGE MASTER TO master_host='127.0.0.1',
> master_port=$SERVER_MYPORT_1;
> > +--source include/start_slave.inc
> > +--source include/sync_with_master_gtid.inc
> > +SELECT * FROM t1 ORDER BY a;
> > +
> > +--source include/stop_slave.inc
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_stop_server.inc
> > +
> > +--remove_file $datadir/master.info
> > +--copy_file $MYSQL_TEST_DIR/std_data/bad3_master.info $datadir/
> master.info
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_start_server.inc
> > +
> > +--source include/wait_until_connected_again.inc
> > +
> > +--connection master
> > +INSERT INTO t1 VALUES (3);
> > +--source include/save_master_gtid.inc
> > +
> > +--connection slave
> > +# Fix the port after we replaced master.info.
> > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1
> > +eval CHANGE MASTER TO master_host='127.0.0.1',
> master_port=$SERVER_MYPORT_1;
> > +--source include/start_slave.inc
> > +--source include/sync_with_master_gtid.inc
> > +SELECT * FROM t1 ORDER BY a;
> > +
> > +--source include/stop_slave.inc
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_stop_server.inc
> > +
> > +--remove_file $datadir/master.info
> > +--copy_file $MYSQL_TEST_DIR/std_data/bad4_master.info $datadir/
> master.info
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_start_server.inc
> > +
> > +--source include/wait_until_connected_again.inc
> > +
> > +--connection master
> > +INSERT INTO t1 VALUES (4);
> > +--source include/save_master_gtid.inc
> > +
> > +--connection slave
> > +# Fix the port after we replaced master.info.
> > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1
> > +eval CHANGE MASTER TO master_host='127.0.0.1',
> master_port=$SERVER_MYPORT_1;
> > +--source include/start_slave.inc
> > +--source include/sync_with_master_gtid.inc
> > +SELECT * FROM t1 ORDER BY a;
> > +
> > +--source include/stop_slave.inc
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_stop_server.inc
> > +
> > +--remove_file $datadir/master.info
> > +--copy_file $MYSQL_TEST_DIR/std_data/bad5_master.info $datadir/
> master.info
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_start_server.inc
> > +
> > +--source include/wait_until_connected_again.inc
> > +
> > +--connection master
> > +INSERT INTO t1 VALUES (5);
> > +--source include/save_master_gtid.inc
> > +
> > +--connection slave
> > +# Fix the port after we replaced master.info.
> > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1
> > +eval CHANGE MASTER TO master_host='127.0.0.1',
> master_port=$SERVER_MYPORT_1;
> > +--source include/start_slave.inc
> > +--source include/sync_with_master_gtid.inc
> > +SELECT * FROM t1 ORDER BY a;
> > +
> > +--source include/stop_slave.inc
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_stop_server.inc
> > +
> > +--remove_file $datadir/master.info
> > +--copy_file $MYSQL_TEST_DIR/std_data/bad6_master.info $datadir/
> master.info
> > +
> > +--let $rpl_server_number= 2
> > +--source include/rpl_start_server.inc
> > +
> > +--source include/wait_until_connected_again.inc
> > +
> > +--connection master
> > +INSERT INTO t1 VALUES (6);
> > +--source include/save_master_gtid.inc
> > +
> > +--connection slave
> > +# Fix the port after we replaced master.info.
> > +--replace_result $SERVER_MYPORT_1 SERVER_MYPORT_1
> > +eval CHANGE MASTER TO master_host='127.0.0.1',
> master_port=$SERVER_MYPORT_1;
> > +--source include/start_slave.inc
> > +--source include/sync_with_master_gtid.inc
> > +SELECT * FROM t1 ORDER BY a;
> > +
> > +
> > +# Cleanup
> > +--connection master
> > +DROP TABLE t1;
> > +--source include/rpl_end.inc
> > diff --git a/sql/rpl_mi.cc b/sql/rpl_mi.cc
> > index df72134..706824c 100644
> > --- a/sql/rpl_mi.cc
> > +++ b/sql/rpl_mi.cc
> > @@ -205,43 +205,56 @@ void init_master_log_pos(Master_info* mi)
> >
> >  /**
> >    Parses the IO_CACHE for "key=" and returns the "key".
> > +  If no '=' found, returns the whole line (for END_MARKER).
> >
> >    @param key      [OUT]               Key buffer
> >    @param max_size [IN]                Maximum buffer size
> >    @param f        [IN]                IO_CACHE file
> > +  @param found_equal [OUT]            Set true if a '=' was found.
> >
> >    @retval 0                           Either "key=" or '\n' found
> >    @retval 1                           EOF
> >  */
> > -static int read_mi_key_from_file(char *key, int max_size, IO_CACHE *f)
> > +static int read_mi_key_from_file(char *key, int max_size, IO_CACHE *f,
> > +                                 bool *found_equal)
>


^^ Looks like this line is not properly intended.


> >  {
> >    int i= 0, c;
> > -  char *last_p;
> >
> >    DBUG_ENTER("read_key_from_file");
> >
> > -  while (((c= my_b_get(f)) != '\n') && (c != my_b_EOF))
> > +  *found_equal= false;
> > +  if (max_size <= 0)
> > +    DBUG_RETURN(1);
> > +  for (;;)
> >    {
> > -    last_p= key + i;
> > -
> > -    if (i < max_size)
> > +    if (i >= max_size-1)
> >      {
> > -      if (c == '=')
> > -      {
> > -        /* We found '=', replace it by 0 and return. */
> > -        *last_p= 0;
> > -        DBUG_RETURN(0);
> > -      }
> > -      else
> > -        *last_p= c;
> > +      key[i] = '\0';
> > +      DBUG_RETURN(0);
> > +    }
> > +    c= my_b_get(f);
> > +    if (c == my_b_EOF)
> > +    {
> > +      DBUG_RETURN(1);
> > +    }
> > +    else if (c == '\n')
> > +    {
> > +      key[i]= '\0';
> > +      DBUG_RETURN(0);
> > +    }
> > +    else if (c == '=')
> > +    {
> > +      key[i]= '\0';
> > +      *found_equal= true;
> > +      DBUG_RETURN(0);
> > +    }
> > +    else
> > +    {
> > +      key[i]= c;
> > +      ++i;
> >      }
> > -    ++i;
> >    }
> > -
> > -  if (c == my_b_EOF)
> > -    DBUG_RETURN(1);
> > -
> > -  DBUG_RETURN(0);
> > +  /* NotReached */
> >  }
> >
> >  enum {
> > @@ -539,6 +552,10 @@ file '%s')", fname);
> >        if (lines >= LINE_FOR_LAST_MYSQL_FUTURE)
> >        {
> >          uint i;
> > +        bool got_eq;
> > +        bool seen_using_gtid= false;
> > +        bool seen_do_domain_ids=false, seen_ignore_domain_ids=false;
> > +
> >          /* Skip lines used by / reserved for MySQL >= 5.6. */
> >          for (i= LINE_FOR_FIRST_MYSQL_5_6; i <=
> LINE_FOR_LAST_MYSQL_FUTURE; ++i)
> >          {
> > @@ -551,11 +568,12 @@ file '%s')", fname);
> >            for "key=" and returns the "key" if found. The "value" can
> then the
> >            parsed on case by case basis. The "unknown" lines would be
> ignored to
> >            facilitate downgrades.
> > +          10.0 does not have the END_MARKER before any left-overs at
> the end
> > +          of the file. So ignore any but the first occurrence of a key.
> >          */
> > -        while (!read_mi_key_from_file(buf, sizeof(buf), &mi->file))
> > +        while (!read_mi_key_from_file(buf, sizeof(buf), &mi->file,
> &got_eq))
> >          {
> > -          /* using_gtid */
> > -          if (!strncmp(buf, STRING_WITH_LEN("using_gtid")))
> > +          if (got_eq && !seen_using_gtid && !strcmp(buf, "using_gtid"))
> >            {
> >              int val;
> >              if (!init_intvar_from_file(&val, &mi->file, 0))
> > @@ -566,15 +584,13 @@ file '%s')", fname);
> >                  mi->using_gtid= Master_info::USE_GTID_SLAVE_POS;
> >                else
> >                  mi->using_gtid= Master_info::USE_GTID_NO;
> > -              continue;
> > +              seen_using_gtid= true;
> >              } else {
> >                sql_print_error("Failed to initialize master info
> using_gtid");
> >                goto errwithmsg;
> >              }
> >            }
> > -
> > -          /* DO_DOMAIN_IDS */
> > -          if (!strncmp(buf, STRING_WITH_LEN("do_domain_ids")))
> > +          else if (got_eq && !seen_do_domain_ids && !strcmp(buf,
> "do_domain_ids"))
> >            {
> >              if (mi->domain_id_filter.init_ids(&mi->file,
> >
> Domain_id_filter::DO_DOMAIN_IDS))
> > @@ -582,11 +598,10 @@ file '%s')", fname);
> >                sql_print_error("Failed to initialize master info
> do_domain_ids");
> >                goto errwithmsg;
> >              }
> > -            continue;
> > +            seen_do_domain_ids= true;
> >            }
> > -
> > -          /* IGNORE_DOMAIN_IDS */
> > -          if (!strncmp(buf, STRING_WITH_LEN("ignore_domain_ids")))
> > +          else if (got_eq && !seen_ignore_domain_ids &&
> > +                   !strcmp(buf, "ignore_domain_ids"))
> >            {
> >              if (mi->domain_id_filter.init_ids(&mi->file,
> >
> Domain_id_filter::IGNORE_DOMAIN_IDS))
> > @@ -595,9 +610,9 @@ file '%s')", fname);
> >                                "ignore_domain_ids");
> >                goto errwithmsg;
> >              }
> > -            continue;
> > +            seen_ignore_domain_ids= true;
> >            }
> > -          else if (!strncmp(buf, STRING_WITH_LEN("END_MARKER")))
> > +          else if (!got_eq && !strcmp(buf, "END_MARKER"))
> >            {
> >              /*
> >                Guard agaist extra left-overs at the end of file, in case
> a later
> > _______________________________________________
> > commits mailing list
> > commits@xxxxxxxxxxx
> > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits
>

Follow ups

References