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

[alfranio correia]

Hi,


In-line comments.

He Zhenxing wrote:
> 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.
>
>   
Do we really need this?
I think we can survive without the atomicity in this case.

>>>  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
>>>
>>>
>>>   
>>>       
>
>
>