List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:July 2 2009 4:56am
Subject:Re: bzr commit into mysql-5.4 branch (zhenxing.he:2804) Bug#45672
Bug#45673
View as plain text  
Andrei Elkin wrote:
> Zhen Xing, hello.
> 
> I have one question and some small notes.
> 
> > #At file:///media/sdb2/hezx/work/mysql/bzrwork/b45672/azalea-bugfixing/ based on
> revid:alik@stripped
> >
> >  2804 He Zhenxing	2009-06-28
> >       BUG#45672 Semisync repl: ActiveTranx:insert_tranx_node: transaction node
> allocation failed
> >       BUG#45673 Semisynch reports correct operation even if no slave is
> connected
> >       
> >       When semi-sync was enabled on master without any semi-sync slaves
> >       connected, it would still think that semi-sync status is ON and
> >       keep insert tranx node and finally result in tranx_node allocation
> >       error.
> >       
> >       This is fixed by not consider semi-sync master status as ON if
> >       no semi-sync slaves connected.
> 
> As a remark,
> this symptom reminds me on discussion we held on setting the master's and the slave's
> 
> sync/2 ON status from the single source that is the slave via
> extending syntax START SLAVE SYNC/2 or smth.
> Sure disconnecting must switch the status OFF.

Yes, I'd love that too, but the problem is that MySQL uses YACC to do
SQL parsing, and there is no way to extend the syntax in a plugin :(

> 
> >      @ plugin/semisync/semisync_master.cc
> >         do not consider semi-sync master status as ON if no semi-sync slaves
> connected
> >      @ plugin/semisync/semisync_slave_plugin.cc
> >         run slave in async mode if master disabled semi-sync
> >      @ sql/log.cc
> >         set error to 1 when flush binlog or run after_flush hooks fails
> >
> >     M  mysql-test/suite/rpl/r/rpl_semi_sync.result
> >     M  mysql-test/suite/rpl/t/rpl_semi_sync.test
> >     M  plugin/semisync/semisync_master.cc
> >     M  plugin/semisync/semisync_slave_plugin.cc
> >     M  sql/log.cc
> > === modified file 'mysql-test/suite/rpl/r/rpl_semi_sync.result'
> > --- a/mysql-test/suite/rpl/r/rpl_semi_sync.result	2009-06-17 10:37:04 +0000
> > +++ b/mysql-test/suite/rpl/r/rpl_semi_sync.result	2009-06-28 12:04:12 +0000
> > @@ -29,6 +29,33 @@ set global rpl_semi_sync_master_enabled 
> >  show variables like 'rpl_semi_sync_master_enabled';
> >  Variable_name	Value
> >  rpl_semi_sync_master_enabled	ON
> > +[ status of semi-sync on master should be OFF without any semi-sync slaves ]
> > +show status like 'Rpl_semi_sync_master_clients';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_clients	0
> > +show status like 'Rpl_semi_sync_master_status';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_status	OFF
> > +show status like 'Rpl_semi_sync_master_yes_tx';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_yes_tx	0
> > +#
> > +# BUG#45672 Semisync repl: ActiveTranx:insert_tranx_node: transaction node
> allocation failed
> > +# BUG#45673 Semisynch reports correct operation even if no slave is connected
> > +#
> > +[ status of semi-sync on master should be OFF ]
> > +show status like 'Rpl_semi_sync_master_clients';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_clients	0
> > +show status like 'Rpl_semi_sync_master_status';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_status	OFF
> > +show status like 'Rpl_semi_sync_master_yes_tx';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_yes_tx	0
> > +#
> > +# INSTALL PLUGIN semi-sync on slave
> > +#
> >  [ on slave ]
> >  [ default state of semi-sync on slave should be OFF ]
> >  show variables like 'rpl_semi_sync_slave_enabled';
> > @@ -225,10 +252,10 @@ Rpl_semi_sync_master_yes_tx	3
> >  #
> >  # Start semi-sync replication without SUPER privilege
> >  #
> > -reset master;
> >  include/stop_slave.inc
> >  reset slave;
> >  [ on master ]
> > +reset master;
> >  set sql_log_bin=0;
> >  grant replication slave on *.* to rpl@stripped identified by 'rpl';
> >  flush privileges;
> > @@ -238,7 +265,13 @@ grant replication slave on *.* to rpl@12
> >  flush privileges;
> >  change master to master_user='rpl',master_password='rpl';
> >  include/start_slave.inc
> > +show status like 'Rpl_semi_sync_slave_status';
> > +Variable_name	Value
> > +Rpl_semi_sync_slave_status	ON
> >  [ on master ]
> > +show status like 'Rpl_semi_sync_master_clients';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_clients	1
> >  show status like 'Rpl_semi_sync_master_status';
> >  Variable_name	Value
> >  Rpl_semi_sync_master_status	ON
> > @@ -250,6 +283,9 @@ Variable_name	Value
> >  Rpl_semi_sync_master_yes_tx	0
> >  insert into t1 values (4);
> >  insert into t1 values (5);
> > +show status like 'Rpl_semi_sync_master_clients';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_clients	1
> >  show status like 'Rpl_semi_sync_master_status';
> >  Variable_name	Value
> >  Rpl_semi_sync_master_status	ON
> > @@ -268,6 +304,32 @@ SHOW STATUS LIKE 'Rpl_semi_sync_slave_st
> >  Variable_name	Value
> >  Rpl_semi_sync_slave_status	OFF
> >  [ on master ]
> > +show status like 'Rpl_semi_sync_master_clients';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_clients	0
> > +show status like 'Rpl_semi_sync_master_status';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_status	OFF
> > +set global rpl_semi_sync_master_enabled= 0;
> > +[ on slave ]
> > +SHOW VARIABLES LIKE 'rpl_semi_sync_slave_enabled';
> > +Variable_name	Value
> > +rpl_semi_sync_slave_enabled	ON
> > +include/start_slave.inc
> > +[ on master ]
> > +insert into t1 values (8);
> > +show status like 'Rpl_semi_sync_master_clients';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_clients	0
> > +show status like 'Rpl_semi_sync_master_status';
> > +Variable_name	Value
> > +Rpl_semi_sync_master_status	OFF
> > +[ on slave ]
> > +show status like 'Rpl_semi_sync_slave_status';
> > +Variable_name	Value
> > +Rpl_semi_sync_slave_status	OFF
> > +include/stop_slave.inc
> > +[ on master ]
> >  set sql_log_bin=0;
> >  UNINSTALL PLUGIN rpl_semi_sync_master;
> >  set sql_log_bin=1;
> >
> > === modified file 'mysql-test/suite/rpl/t/rpl_semi_sync.test'
> > --- a/mysql-test/suite/rpl/t/rpl_semi_sync.test	2009-06-17 10:37:04 +0000
> > +++ b/mysql-test/suite/rpl/t/rpl_semi_sync.test	2009-06-28 12:04:12 +0000
> > @@ -58,6 +58,50 @@ echo [ enable semi-sync on master ];
> >  set global rpl_semi_sync_master_enabled = 1;
> >  show variables like 'rpl_semi_sync_master_enabled';
> >  
> > +echo [ status of semi-sync on master should be OFF without any semi-sync slaves
> ];
> 
> Good that you comment on `show status' expectation.
> 
> > +show status like 'Rpl_semi_sync_master_clients';
> > +show status like 'Rpl_semi_sync_master_status';
> > +show status like 'Rpl_semi_sync_master_yes_tx';
> > +
> > +--echo #
> > +--echo # BUG#45672 Semisync repl: ActiveTranx:insert_tranx_node: transaction
> node allocation failed
> > +--echo # BUG#45673 Semisynch reports correct operation even if no slave is
> connected
> > +--echo #
> > +
> > +# BUG#45672 When semi-sync is enabled on master, it would allocate
> > +# transaction node even without semi-sync slave connected, and would
> > +# finally result in transaction node allocation error.
> > +#
> > +# Semi-sync master will pre-allocate 'max_connections' transaction
> > +# nodes, so here we do more than that much transactions to check if it
> > +# will fail or not.
> > +# select @@global.max_connections + 1;
> > +let $i= `select @@global.max_connections + 1`;
> > +disable_query_log;
> > +eval create table t1 (a int) engine=$engine_type;
> > +while ($i)
> > +{
> > +  eval insert into t1 values ($i);
> > +  dec $i;
> > +}
> > +drop table t1;
> > +enable_query_log;
> > +
> > +# BUG#45673
> > +echo [ status of semi-sync on master should be OFF ];
> > +show status like 'Rpl_semi_sync_master_clients';
> > +show status like 'Rpl_semi_sync_master_status';
> > +show status like 'Rpl_semi_sync_master_yes_tx';
> > +
> > +disable_query_log;
> > +# reset master to make sure the following test will start with a clean
> environment
> > +reset master;
> > +enable_query_log;
> > +
> > +--echo #
> > +--echo # INSTALL PLUGIN semi-sync on slave
> > +--echo #
> > +
> >  connection slave;
> >  echo [ on slave ];
> >  
> > @@ -237,11 +281,35 @@ echo [ on slave ];
> >  
> >  source include/stop_slave.inc;
> >  reset slave;
> > +
> > +# Kill the dump thread on master for previous slave connection and
> > +# wait for it to exit
> > +connection master;
> > +let $_tid= `select id from information_schema.processlist where command =
> 'Binlog Dump' limit 1`;
> > +if ($_tid)
> > +{
> > +  disable_query_log;
> > +  eval kill query $_tid;
> > +  enable_query_log;
> > +
> > +  # After dump thread exit, Rpl_semi_sync_master_clients will be 0
> > +  let $status_var= Rpl_semi_sync_master_clients;
> > +  let $status_var_value= 0;
> > +  source include/wait_for_status_var.inc;
> > +}
> > +
> > +connection slave;
> >  source include/start_slave.inc;
> >  
> >  connection master;
> >  echo [ on master ];
> >  
> > +# Wait for dump thread to start, Rpl_semi_sync_master_clients will be
> > +# 1 after dump thread started.
> > +let $status_var= Rpl_semi_sync_master_clients;
> > +let $status_var_value= 1;
> > +source include/wait_for_status_var.inc;
> > +
> >  replace_result $engine_type ENGINE_TYPE;
> >  eval create table t1 (a int) engine = $engine_type;
> >  insert into t1 values (1);
> > @@ -262,13 +330,27 @@ show status like 'Rpl_semi_sync_master_y
> >  --echo #
> >  --echo # Start semi-sync replication without SUPER privilege
> >  --echo #
> > -connection master;
> > -reset master;
> > -sync_slave_with_master;
> > +connection slave;
> >  source include/stop_slave.inc;
> >  reset slave;
> >  connection master;
> >  echo [ on master ];
> > +reset master;
> > +
> > +# Kill the dump thread on master for previous slave connection and wait for it
> to exit
> > +let $_tid= `select id from information_schema.processlist where command =
> 'Binlog Dump' limit 1`;
> > +if ($_tid)
> > +{
> > +  disable_query_log;
> > +  eval kill query $_tid;
> > +  enable_query_log;
> > +
> > +  # After dump thread exit, Rpl_semi_sync_master_clients will be 0
> > +  let $status_var= Rpl_semi_sync_master_clients;
> > +  let $status_var_value= 0;
> > +  source include/wait_for_status_var.inc;
> > +}
> > +
> >  # Do not binlog the following statement because it will generate
> >  # different events for ROW and STATEMENT format
> >  set sql_log_bin=0;
> > @@ -281,13 +363,21 @@ grant replication slave on *.* to rpl@12
> >  flush privileges;
> >  change master to master_user='rpl',master_password='rpl';
> >  source include/start_slave.inc;
> > +show status like 'Rpl_semi_sync_slave_status';
> >  connection master;
> >  echo [ on master ];
> > +
> > +# Wait for the semi-sync binlog dump thread to start
> > +let $status_var= Rpl_semi_sync_master_clients;
> > +let $status_var_value= 1;
> > +source include/wait_for_status_var.inc;
> 
> Adding what-do-you-expect-out-of-show-status comments is appreciated.
> 

OK

> > +show status like 'Rpl_semi_sync_master_clients';
> >  show status like 'Rpl_semi_sync_master_status';
> >  show status like 'Rpl_semi_sync_master_no_tx';
> >  show status like 'Rpl_semi_sync_master_yes_tx';
> >  insert into t1 values (4);
> >  insert into t1 values (5);
> > +show status like 'Rpl_semi_sync_master_clients';
> >  show status like 'Rpl_semi_sync_master_status';
> >  show status like 'Rpl_semi_sync_master_no_tx';
> >  show status like 'Rpl_semi_sync_master_yes_tx';
> > @@ -296,6 +386,7 @@ show status like 'Rpl_semi_sync_master_y
> >  --echo # Test semi-sync slave connect to non-semi-sync master
> >  --echo #
> >  
> > +# Disable semi-sync on master
> >  connection slave;
> >  echo [ on slave ];
> >  source include/stop_slave.inc;
> > @@ -303,6 +394,43 @@ SHOW STATUS LIKE 'Rpl_semi_sync_slave_st
> >  
> >  connection master;
> >  echo [ on master ];
> > +
> > +# Kill the dump thread on master for previous slave connection and wait for it
> to exit
> > +let $_tid= `select id from information_schema.processlist where command =
> 'Binlog Dump' limit 1`;
> > +if ($_tid)
> > +{
> > +  disable_query_log;
> > +  eval kill query $_tid;
> > +  enable_query_log;
> > +
> > +  # After dump thread exit, Rpl_semi_sync_master_clients will be 0
> > +  let $status_var= Rpl_semi_sync_master_clients;
> > +  let $status_var_value= 0;
> > +  source include/wait_for_status_var.inc;
> > +}
> 
> > +
> > +show status like 'Rpl_semi_sync_master_clients';
> > +show status like 'Rpl_semi_sync_master_status';
> 
> Same as above.
> 

OK

> 
> > +set global rpl_semi_sync_master_enabled= 0;
> > +
> > +connection slave;
> > +echo [ on slave ];
> > +SHOW VARIABLES LIKE 'rpl_semi_sync_slave_enabled';
> > +source include/start_slave.inc;
> > +connection master;
> > +echo [ on master ];
> > +insert into t1 values (8);
> > +show status like 'Rpl_semi_sync_master_clients';
> > +show status like 'Rpl_semi_sync_master_status';
> > +sync_slave_with_master;
> > +echo [ on slave ];
> > +show status like 'Rpl_semi_sync_slave_status';
> > +
> > +# Uninstall semi-sync plugin on master
> > +connection slave;
> > +source include/stop_slave.inc;
> > +connection master;
> > +echo [ on master ];
> >  set sql_log_bin=0;
> >  UNINSTALL PLUGIN rpl_semi_sync_master;
> >  set sql_log_bin=1;
> >
> > === modified file 'plugin/semisync/semisync_master.cc'
> > --- a/plugin/semisync/semisync_master.cc	2009-06-17 10:37:04 +0000
> > +++ b/plugin/semisync/semisync_master.cc	2009-06-28 12:04:12 +0000
> > @@ -138,6 +138,15 @@ ActiveTranx::TranxNode* ActiveTranx::all
> >      ptr->next_ = NULL;
> >      ptr->hash_next_ = NULL;
> >    }
> > +  else
> > +  {
> > +    /*
> > +      free_pool should never be NULL here, because we have
> > +      max_connections number of pre-allocated nodes.
> > +    */
> 
> > +    sql_print_error("You have encountered a semi-sync bug, please report to the
> developer");
> 
> Sounds like you got used to the server's api in your plugin, good :-)

right

> 
> Please consider s/please report to the developer/please report to bugs.mysql.com/
> 

good

> 
> > +    assert(free_pool_);
> > +  }
> >  
> >    return ptr;
> >  }
> > @@ -821,7 +830,7 @@ int ReplSemiSyncMaster::commitTrx(const 
> >  
> >    l_end:
> >      /* Update the status counter. */
> > -    if (is_on())
> > +    if (is_on() && rpl_semi_sync_master_clients)
> 
> Would it be better/always if/that is_on() checks rpl_semi_sync_master_clients
> inside?
> On the other hand a lot of is_on() without rpl_semi_sync_master_clients concern
> remains.
> 
> 
> >        enabled_transactions_++;
> >      else
> >        disabled_transactions_++;
> > @@ -1083,10 +1092,19 @@ int ReplSemiSyncMaster::writeTranxInBinl
> >      commit_file_name_inited_ = true;
> >    }
> >  
> > -  if (is_on())
> > +  if (is_on() && rpl_semi_sync_master_clients)
> >    {
> >      assert(active_tranxs_ != NULL);
> > -    result = active_tranxs_->insert_tranx_node(log_file_name,
> log_file_pos);
> > +    if(active_tranxs_->insert_tranx_node(log_file_name, log_file_pos))
> > +    {
> > +      /*
> > +        if insert tranx_node failed, print a warning message
> > +        and turn off semi-sync
> > +      */
> > +      sql_print_warning("Semi-sync failed to insert tranx_node for binlog file:
> %s, position: %ul",
> > +                        log_file_name, log_file_pos);
> > +      switch_off();
> > +    }
> >    }
> >  
> >   l_end:
> > @@ -1131,7 +1149,7 @@ void ReplSemiSyncMaster::setExportStats(
> >  {
> >    lock();
> >  
> > -  rpl_semi_sync_master_status           = state_ ? 1 : 0;
> > +  rpl_semi_sync_master_status           = state_ &&
> rpl_semi_sync_master_clients;
> >    rpl_semi_sync_master_yes_transactions = enabled_transactions_;
> >    rpl_semi_sync_master_no_transactions  = disabled_transactions_;
> >    rpl_semi_sync_master_off_times        = switched_off_times_;
> >
> > === modified file 'plugin/semisync/semisync_slave_plugin.cc'
> > --- a/plugin/semisync/semisync_slave_plugin.cc	2009-06-17 10:37:04 +0000
> > +++ b/plugin/semisync/semisync_slave_plugin.cc	2009-06-28 12:04:12 +0000
> > @@ -63,7 +63,7 @@ int repl_semi_slave_request_dump(Binlog_
> >    }
> >  
> >    row= mysql_fetch_row(res);
> > -  if (!row)
> > +  if (!row || strcmp(row[1], "ON"))
> >    {
> >      /* Master does not support or not configured semi-sync */
> >      sql_print_warning("Master server does not support or not configured
> semi-sync replication, fallback to asynchronous");
> >
> > === modified file 'sql/log.cc'
> > --- a/sql/log.cc	2009-06-15 14:00:15 +0000
> > +++ b/sql/log.cc	2009-06-28 12:04:12 +0000
> > @@ -5994,7 +5994,6 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
> >      if (event_info->write(file))
> >        goto err;
> >  
> > -    error=0;
> >      if (file == &log_file) // we are writing to the real log (disk)
> >      {
> >        bool synced;
> > @@ -6003,12 +6002,14 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
> >  
> >        if (RUN_HOOK(binlog_storage, after_flush,
> >                     (thd, log_file_name, file->pos_in_file, synced))) {
> > +        sql_print_error("Failed to run 'after_flush' hooks");
> 
> Should not an error be thrown with my_error(), here and in the other occurrence?
> I don't see a reason why the user should not see the error message by means
> of sql.
> 

There will be a call to mysql_error() later at the end of this function.

> 
> >          goto err;
> >        }
> >  
> >        signal_update();
> >        rotate_and_purge(RP_LOCK_LOG_IS_ALREADY_LOCKED);
> >      }
> > +    error=0;
> >  
> >  err:
> >      if (error)
> > @@ -6383,7 +6384,11 @@ bool MYSQL_BIN_LOG::write(THD *thd, IO_C
> >  
> >        if (RUN_HOOK(binlog_storage, after_flush,
> >                     (thd, log_file_name, log_file.pos_in_file, synced)))
> > +      {
> > +        sql_print_error("Failed to run 'after_flush' hooks");
> > +        write_error=1;
> >          goto err;
> > +      }
> >  
> >        signal_update();
> >      }
> >
> >
> 
> cheers,
> 
> Andrei
> 

Thread
bzr commit into mysql-5.4 branch (zhenxing.he:2804) Bug#45672 Bug#45673He Zhenxing28 Jun
  • Re: bzr commit into mysql-5.4 branch (zhenxing.he:2804) Bug#45672Bug#45673Andrei Elkin30 Jun
    • Re: bzr commit into mysql-5.4 branch (zhenxing.he:2804) Bug#45672Bug#45673He Zhenxing2 Jul