Re: bzr commit into mysql-5.0-bugteam branch (alfranio.correia:2704) Bug#31665 Bug#35542 Bug#40337

[He Zhenxing]

alfranio correia wrote: 
> Alfranio Correia wrote:
> > #At
> file:///home/acorreia/workspace.sun/playground.mysql/mysql-5.0.68-custom_build/
> >
> >  2704 Alfranio Correia	2008-11-07
> >       Added option to sync master.info (--sync-master-info, integer) 
> >       and relay log (--sync-relay-log, integer) to disk after 
> >       every event and the relay info (--sync-relay-log-info, boolean) per
> transaction. 
> >       This supersedes the patch proposed 
> >       by Sinisa and modified by He Zhenxing to fix bugs: BUG#35542 and
> BUG#31665. This is an update to BUG#40337.
> > modified:
> >   sql/log.cc
> >   sql/mysql_priv.h
> >   sql/mysqld.cc
> >   sql/set_var.cc
> >   sql/set_var.h
> >   sql/slave.cc
> >   sql/sql_class.h
> >
> > === modified file 'sql/log.cc'
> > --- a/sql/log.cc	2008-07-24 12:28:21 +0000
> > +++ b/sql/log.cc	2008-11-07 19:21:48 +0000
> > @@ -32,8 +32,7 @@
> >  #include "message.h"
> >  #endif
> >  
> > -MYSQL_LOG mysql_log, mysql_slow_log, mysql_bin_log;
> > -ulong sync_binlog_counter= 0;
> > +MYSQL_LOG mysql_log, mysql_slow_log, mysql_bin_log(&sync_binlog_period);
> >  
> >  static Muted_query_log_event invisible_commit;
> >  
> > @@ -402,10 +401,11 @@ static int find_uniq_filename(char *name
> >  }
> >  
> >  
> > -MYSQL_LOG::MYSQL_LOG()
> > +MYSQL_LOG::MYSQL_LOG(uint *sync_period)
> >    :bytes_written(0), last_time(0), query_start(0), name(0),
> >     prepared_xids(0), log_type(LOG_CLOSED), file_id(1), open_count(1),
> >     write_error(FALSE), inited(FALSE), need_start_event(TRUE),
> > +   sync_period_ptr(sync_period),
> >     description_event_for_exec(0), description_event_for_queue(0)
> >  {
> >    /*
> > @@ -1622,6 +1622,8 @@ bool MYSQL_LOG::append(Log_event* ev)
> >    }
> >    bytes_written+= ev->data_written;
> >    DBUG_PRINT("info",("max_size: %lu",max_size));
> > +  if (flush_and_sync())
> > +    goto err;
> >    if ((uint) my_b_append_tell(&log_file) > max_size)
> >      new_file(0);
> >  
> > @@ -1652,6 +1654,8 @@ bool MYSQL_LOG::appendv(const char* buf,
> >      bytes_written += len;
> >    } while ((buf=va_arg(args,const char*)) && (len=va_arg(args,uint)));
> >    DBUG_PRINT("info",("max_size: %lu",max_size));
> > +  if (flush_and_sync())
> > +    goto err;
> >    if ((uint) my_b_append_tell(&log_file) > max_size)
> >      new_file(0);
> >  
> > @@ -1758,9 +1762,10 @@ bool MYSQL_LOG::flush_and_sync()
> >    safe_mutex_assert_owner(&LOCK_log);
> >    if (flush_io_cache(&log_file))
> >      return 1;
> > -  if (++sync_binlog_counter >= sync_binlog_period &&
> sync_binlog_period)
> > +  uint sync_period= get_sync_period();
> > +  if (sync_period && ++sync_counter >= sync_period)
> >    {
> > -    sync_binlog_counter= 0;
> > +    sync_counter= 0;
> >      err=my_sync(fd, MYF(MY_WME));
> >    }
> >    return err;
> >
> > === modified file 'sql/mysql_priv.h'
> > --- a/sql/mysql_priv.h	2008-10-02 11:57:52 +0000
> > +++ b/sql/mysql_priv.h	2008-11-07 19:21:48 +0000
> > @@ -1310,7 +1310,8 @@ extern ulong max_binlog_size, max_relay_
> >  extern ulong rpl_recovery_rank, thread_cache_size;
> >  extern ulong back_log;
> >  extern ulong specialflag, current_pid;
> > -extern ulong expire_logs_days, sync_binlog_period, sync_binlog_counter;
> > +extern ulong expire_logs_days;
> > +extern uint sync_binlog_period, sync_relaylog_period, sync_relayloginfo_period,
> sync_masterinfo_period;
> >   
> Jasonh, any reason to change from ulong to uint?
> 

read and write int are supposed to be atomic on all platforms supported
by MySQL, so we don't need to add LOCKs to protect the access of these
variables. And we cannot expect the same for long.

> >  extern ulong opt_tc_log_size, tc_log_max_pages_used, tc_log_page_size;
> >  extern ulong tc_log_page_waits;
> >  extern my_bool relay_log_purge, opt_innodb_safe_binlog, opt_innodb;
> >
> > === modified file 'sql/mysqld.cc'
> > --- a/sql/mysqld.cc	2008-08-26 08:32:43 +0000
> > +++ b/sql/mysqld.cc	2008-11-07 19:21:48 +0000
> > @@ -466,7 +466,9 @@ ulong max_prepared_stmt_count;
> >  */
> >  ulong prepared_stmt_count=0;
> >  ulong thread_id=1L,current_pid;
> > -ulong slow_launch_threads = 0, sync_binlog_period;
> > +ulong slow_launch_threads = 0;
> > +uint sync_binlog_period= 0, sync_relaylog_period= 0, 
> > +  sync_relayloginfo_period= 0, sync_masterinfo_period= 0;
> >  ulong expire_logs_days = 0;
> >  ulong rpl_recovery_rank=0;
> >  
> > @@ -4995,7 +4997,10 @@ enum options_mysqld
> >    OPT_SECURE_FILE_PRIV,
> >    OPT_KEEP_FILES_ON_CREATE,
> >    OPT_INNODB_ADAPTIVE_HASH_INDEX,
> > -  OPT_FEDERATED
> > +  OPT_FEDERATED,
> > +  OPT_SYNC_RELAY_LOG,
> > +  OPT_SYNC_RELAY_LOG_INFO,
> > +  OPT_SYNC_MASTER_INFO
> >  };
> >  
> >  
> > @@ -6342,8 +6347,23 @@ The minimum value for this variable is 4
> >    {"sync-binlog", OPT_SYNC_BINLOG,
> >     "Synchronously flush binary log to disk after every #th event. "
> >     "Use 0 (default) to disable synchronous flushing.",
> > -   (gptr*) &sync_binlog_period, (gptr*) &sync_binlog_period, 0,
> GET_ULONG,
> > -   REQUIRED_ARG, 0, 0, ULONG_MAX, 0, 1, 0},
> > +   (gptr*) &sync_binlog_period, (gptr*) &sync_binlog_period, 0,
> GET_UINT,
> > +   REQUIRED_ARG, 0, 0, UINT_MAX, 0, 1, 0},
> > +  {"sync-relay-log", OPT_SYNC_RELAY_LOG,
> > +   "Synchronously flush relay log to disk after every #th event. "
> > +   "Use 0 (default) to disable synchronous flushing.",
> > +   (gptr *) &sync_relaylog_period, (gptr *) &sync_relaylog_period, 0,
> GET_UINT,
> > +   REQUIRED_ARG, 0, 0, UINT_MAX, 0, 1, 0},
> > +  {"sync-relay-log-info", OPT_SYNC_RELAY_LOG_INFO,
> > +   "Synchronously flush relay log info to disk after every commit/rollback. "
> > +   "Use 0 (default) to disable synchronous flushing.",
> > +   (gptr *) &sync_relayloginfo_period, (gptr *)
> &sync_relayloginfo_period, 0, GET_BOOL,
> > +   NO_ARG, 0, 0, 1, 0, 1, 0},
> > +  {"sync-master-info", OPT_SYNC_MASTER_INFO,
> > +   "Synchronously flush master info to disk after every #th event. "
> > +   "Use 0 (default) to disable synchronous flushing.",
> > +   (gptr *) &sync_masterinfo_period, (gptr *) &sync_masterinfo_period,
> 0, GET_UINT,
> > +   REQUIRED_ARG, 0, 0, UINT_MAX, 0, 1, 0},
> >    {"sync-frm", OPT_SYNC_FRM, "Sync .frm to disk on create. Enabled by
> default.",
> >     (gptr*) &opt_sync_frm, (gptr*) &opt_sync_frm, 0, GET_BOOL, NO_ARG,
> 1, 0,
> >     0, 0, 0, 0},
> >
> > === modified file 'sql/set_var.cc'
> > --- a/sql/set_var.cc	2008-08-25 12:11:59 +0000
> > +++ b/sql/set_var.cc	2008-11-07 19:21:48 +0000
> > @@ -402,7 +402,10 @@ sys_var_thd_table_type  sys_table_type("
> >  sys_var_thd_storage_engine sys_storage_engine("storage_engine",
> >  				       &SV::table_type);
> >  #ifdef HAVE_REPLICATION
> > -sys_var_sync_binlog_period sys_sync_binlog_period("sync_binlog",
> &sync_binlog_period);
> > +sys_var_int_ptr sys_sync_binlog_period("sync_binlog",
> &sync_binlog_period);
> > +sys_var_int_ptr sys_sync_relaylog_period("sync_relay_log",
> &sync_relaylog_period);
> > +sys_var_int_ptr sys_sync_relayloginfo_period("sync_relay_log_info",
> &sync_relayloginfo_period);
> > +sys_var_int_ptr sys_sync_masterinfo_period("sync_master_info",
> &sync_masterinfo_period);
> >  #endif
> >  sys_var_bool_ptr	sys_sync_frm("sync_frm", &opt_sync_frm);
> >  sys_var_const_str	sys_system_time_zone("system_time_zone",
> > @@ -762,6 +765,9 @@ sys_var *sys_variables[]=
> >    &sys_storage_engine,
> >  #ifdef HAVE_REPLICATION
> >    &sys_sync_binlog_period,
> > +  &sys_sync_relaylog_period,
> > +  &sys_sync_relayloginfo_period,
> > +  &sys_sync_masterinfo_period,
> >  #endif
> >    &sys_sync_frm,
> >    &sys_system_time_zone,
> > @@ -1090,6 +1096,9 @@ struct show_var_st init_vars[]= {
> >    {sys_storage_engine.name,   (char*) &sys_storage_engine,         
> SHOW_SYS},
> >  #ifdef HAVE_REPLICATION
> >    {sys_sync_binlog_period.name,(char*) &sys_sync_binlog_period,    
> SHOW_SYS},
> > +  {sys_sync_relaylog_period.name,(char*) &sys_sync_relaylog_period,    
> SHOW_SYS},
> > +  {sys_sync_relayloginfo_period.name,(char*) &sys_sync_relayloginfo_period,
>     SHOW_SYS},
> > +  {sys_sync_masterinfo_period.name,(char*) &sys_sync_masterinfo_period,    
> SHOW_SYS},
> >  #endif
> >    {sys_sync_frm.name,         (char*) &sys_sync_frm,              
> SHOW_SYS},
> >  #ifdef HAVE_TZNAME
> > @@ -1486,6 +1495,23 @@ static bool get_unsigned(THD *thd, set_v
> >  }
> >  
> >  
> > +bool sys_var_int_ptr::check(THD *thd, set_var *var)
> > +{
> > +  var->save_result.ulong_value= (ulong) var->value->val_int();
> > +  return 0;
> > +}
> > +
> > +bool sys_var_int_ptr::update(THD *thd, set_var *var)
> > +{
> > +  *value= (uint) var->save_result.ulong_value;
> > +  return 0;
> > +}
> > +
> > +void sys_var_int_ptr::set_default(THD *thd, enum_var_type type)
> > +{
> > +  *value= (uint) option_limits->def_value;
> > +}
> > +
> >  sys_var_long_ptr::
> >  sys_var_long_ptr(const char *name_arg, ulong *value_ptr_arg,
> >                   sys_after_update_func after_update_arg)
> > @@ -2759,13 +2785,6 @@ bool sys_var_slave_skip_counter::update(
> >    pthread_mutex_unlock(&LOCK_active_mi);
> >    return 0;
> >  }
> > -
> > -
> > -bool sys_var_sync_binlog_period::update(THD *thd, set_var *var)
> > -{
> > -  sync_binlog_period= (ulong) var->save_result.ulonglong_value;
> > -  return 0;
> > -}
> >  #endif /* HAVE_REPLICATION */
> >  
> >  bool sys_var_rand_seed1::update(THD *thd, set_var *var)
> >
> > === modified file 'sql/set_var.h'
> > --- a/sql/set_var.h	2008-01-23 15:03:58 +0000
> > +++ b/sql/set_var.h	2008-11-07 19:21:48 +0000
> > @@ -110,6 +110,26 @@ public:
> >    { return (byte*) value; }
> >  };
> >  
> > +/**
> > +   Unsigned int system variable class
> > + */
> > +class sys_var_int_ptr :public sys_var
> > +{
> > +public:
> > +  sys_var_int_ptr(const char *name_arg, uint *value_ptr_arg,
> > +                  sys_after_update_func after_update_arg= NULL)
> > +    :sys_var(name_arg, after_update_arg),
> > +     value(value_ptr_arg)
> > +  {}
> > +  bool check(THD *thd, set_var *var);
> > +  bool update(THD *thd, set_var *var);
> > +  void set_default(THD *thd, enum_var_type type);
> > +  SHOW_TYPE show_type() { return SHOW_INT; }
> > +  byte *value_ptr(THD *thd, enum_var_type type, LEX_STRING *base)
> > +  { return (byte*) value; }
> > +private:
> > +  uint *value;
> > +};
> >  
> >  /*
> >    A global ulong variable that is protected by LOCK_global_system_variables
> > @@ -549,14 +569,6 @@ public:
> >      type() or value_ptr()
> >    */
> >  };
> > -
> > -class sys_var_sync_binlog_period :public sys_var_long_ptr
> > -{
> > -public:
> > -  sys_var_sync_binlog_period(const char *name_arg, ulong *value_ptr_arg)
> > -    :sys_var_long_ptr(name_arg,value_ptr_arg) {}
> > -  bool update(THD *thd, set_var *var);
> > -};
> >   
> I've removed this code and it is not being
> used.
> 
> Jasonh, do you I agree?
> 

Yes, please

> >  #endif
> >  
> >  class sys_var_rand_seed1 :public sys_var
> >
> > === modified file 'sql/slave.cc'
> > --- a/sql/slave.cc	2008-03-28 20:01:05 +0000
> > +++ b/sql/slave.cc	2008-11-07 19:21:48 +0000
> > @@ -2582,6 +2582,7 @@ int flush_master_info(MASTER_INFO* mi, b
> >  {
> >    IO_CACHE* file = &mi->file;
> >    char lbuf[22];
> > +  int err= 0;
> >    DBUG_ENTER("flush_master_info");
> >    DBUG_PRINT("enter",("master_pos: %ld", (long) mi->master_log_pos));
> >  
> > @@ -2597,9 +2598,12 @@ int flush_master_info(MASTER_INFO* mi, b
> >      When we come to this place in code, relay log may or not be initialized;
> >      the caller is responsible for setting 'flush_relay_log_cache' accordingly.
> >    */
> > -  if (flush_relay_log_cache &&
> > -      flush_io_cache(mi->rli.relay_log.get_log_file()))
> > -    DBUG_RETURN(2);
> > +  if (flush_relay_log_cache)
> > +  {
> > +    IO_CACHE *log_file= mi->rli.relay_log.get_log_file();
> > +    if (flush_io_cache(log_file))
> > +      DBUG_RETURN(2);
> > +  }
> >   
> Jasonh, any reason to do this refactoring?
> I agree with the last comment made by Mats.
> This may generate unnecessary conflicts in the feature.

The refactoring is the leftover of a previous patch, in which I added
some codes. Since those codes are removed, you should discard the
refactoring too.

> 
> >  
> >    /*
> >      We flushed the relay log BEFORE the master.info file, because if we crash
> > @@ -2626,12 +2630,20 @@ int flush_master_info(MASTER_INFO* mi, b
> >  	      mi->password, mi->port, mi->connect_retry,
> >                (int)(mi->ssl), mi->ssl_ca, mi->ssl_capath,
> mi->ssl_cert,
> >                mi->ssl_cipher, mi->ssl_key);
> > -  DBUG_RETURN(-flush_io_cache(file));
> > +  err= flush_io_cache(file);
> > +  static unsigned int sync_counter = 0;
> > +  if (sync_masterinfo_period && !err && ++sync_counter >=
> sync_masterinfo_period)
> > +  {
> > +    err= my_sync(mi->fd, MYF(MY_WME));
> > +    sync_counter= 0;
> > +  }
> > +  DBUG_RETURN(-err);
> >  }
> >  
> >  
> >  st_relay_log_info::st_relay_log_info()
> > -  :info_fd(-1), cur_log_fd(-1), save_temporary_tables(0),
> > +  :info_fd(-1), cur_log_fd(-1), relay_log(&sync_relaylog_period),
> > +   save_temporary_tables(0),
> >     cur_log_old_open_count(0), group_master_log_pos(0), log_space_total(0),
> >     ignore_log_space_limit(0), last_master_timestamp(0), slave_skip_counter(0),
> >     abort_pos_wait(0), slave_run_id(0), sql_thd(0), last_slave_errno(0),
> > @@ -4872,6 +4884,12 @@ bool flush_relay_log_info(RELAY_LOG_INFO
> >      error=1;
> >    if (flush_io_cache(file))
> >      error=1;
> > +  if (sync_relayloginfo_period &&
> > +      !error)
> > +  {
> > +    if (my_sync(rli->info_fd, MYF(MY_WME)))
> > +      error=1;
> > +  }
> >    /* Flushing the relay log is done by the slave I/O thread */
> >    return error;
> >  }
> >
> > === modified file 'sql/sql_class.h'
> > --- a/sql/sql_class.h	2008-09-17 06:34:00 +0000
> > +++ b/sql/sql_class.h	2008-11-07 19:21:48 +0000
> > @@ -238,6 +238,18 @@ class MYSQL_LOG: public TC_LOG
> >    bool no_auto_events;
> >    friend class Log_event;
> >  
> > +  /* pointer to the sync period variable, for binlog this will be
> > +     sync_binlog_period, for relay log this will be
> > +     sync_relay_log_period
> > +  */
> > +  uint *sync_period_ptr;
> > +  uint sync_counter;
> > +
> > +  inline uint get_sync_period()
> > +  {
> > +    return *sync_period_ptr;
> > +  }
> > +
> >  public:
> >    /*
> >      These describe the log's format. This is used only for relay logs.
> > @@ -250,7 +262,7 @@ public:
> >    Format_description_log_event *description_event_for_exec,
> >      *description_event_for_queue;
> >  
> > -  MYSQL_LOG();
> > +  MYSQL_LOG(uint *sync_period=0);
> >    /*
> >      note that there's no destructor ~MYSQL_LOG() !
> >      The reason is that we don't want it to be automatically called
> >
> >
> >