Re: bzr commit into mysql-5.1-rpl branch (hezx:2611) Bug#35542, Bug#31665

[He Zhenxing]

Mats Kindahl wrote:
> He Zhenxing wrote:
> > Mats Kindahl wrote:
> >> Hi Jason and Sinisa!
> >>
> >> Here are my comments on the patch...
> >>
> >> Just my few cents,
> >> Mats Kindahl
> >>
> >> He Zhenxing wrote:
> >>> #At file:///media/sda3/work/mysql/bzrwork/b31665/5.1-rpl/
> >> Just a note, to avoid misunderstandings: according to bug report, this
> >> patch is to be for 6.0, not 5.1.  I would also advice against
> >> introducing a patch of this kind into 5.1 since it risks de-stabilizing
> >> the server.
> >>
> > 
> > Why do you think this patch is risky?
> 
> It adds a feature, i.e., a new server variable.
> 

apart from that, do you see anything suspect? Lars said that if this
patch is 'quite risk-free', we can consider it for 5.1. 

> [snip]
> 
> >>> @@ -49,8 +49,7 @@
> >>>  
> >>>  LOGGER logger;
> >>>  
> >>> -MYSQL_BIN_LOG mysql_bin_log;
> >>> -ulong sync_binlog_counter= 0;
> >>> +MYSQL_BIN_LOG mysql_bin_log(&sync_binlog_period);
> >> Note: there is no mutex or other form of lock protecting this variable.
> >> In the event that the ulong is split into several registers, the two
> >> parts might be read and written separately.
> >>
> > 
> > yes, will be fixed
> 
> OK.
> 
> [snip]
> 
> >>> @@ -3512,9 +3516,9 @@ bool MYSQL_BIN_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)
> >>> +  if (*sync_period_ptr && ++sync_counter >=
> *sync_period_ptr)
> >>>    {
> >>> -    sync_binlog_counter= 0;
> >>> +    sync_counter= 0;
> >>>      err=my_sync(fd, MYF(MY_WME));
> >>>    }
> >>>    return err;
> >> If the variable is written at the same time as it is read, this might
> >> cause a flush to be missed or an extra flush to be executed. Thanks to
> >> the fact that >= is used instead of ==, it will not cease flushing the
> >> relay log in the case of a race condition. However, optimization might
> >> change the code in unpredictable ways, so I would suggest adding a mutex
> >> to protect the variable and add that as an argument to the MYSQL_BIN_LOG
> >> constructor, or use the mutex associated with the
> >> sys_var_sync_relaylog_period introduced below (which is
> >> LOCK_global_system_variables).
> >>
> > 
> > right
> > 
> >> An alternative would be to make the variable fully internal in the
> >> MYSQL_BIN_LOG class, and then change its value through getters and
> >> setters, which can then appropriately protect the variable from race
> >> conditions by using a fully internal mutex or rwlock, whichever seems
> >> most appropriate.
> >>
> > 
> > After dive into the code a little bit, it seems that this approach is
> > not possible because when showing the value of the variable, it is
> > always guarded by LOCK_global_system_variables.
> 
> Sorry, I don't follow. Is it not possible to execute a bit of code
> inside the ::update function that calls a setter for the MYSQL_BIN_LOG
> object in question?
> 

It is possible to guard the code with another lock in the update method,
but when running commands like SHOW VARIALBLES or SELECT, the code to
get the value is always guarded by LOCK_global_system_variables, even if
the SHOW_TYPE is SHOW_FUNC, the show function can only return a pointer
to the value of the system variable, and then the code to get the value
from the pointer is only guarded by LOCK_global_system_variables. So if
we use our internal lock, the value returned by SHOW or SELECT command
might be incorrect.

> [snip]
> 
> >>> @@ -285,7 +292,7 @@ public:
> >>>    Format_description_log_event *description_event_for_exec,
> >>>      *description_event_for_queue;
> >>>  
> >>> -  MYSQL_BIN_LOG();
> >>> +  MYSQL_BIN_LOG(ulong *sync_period);
> >>>    /*
> >>>      note that there's no destructor ~MYSQL_BIN_LOG() !
> >>>      The reason is that we don't want it to be automatically called
> >> Uh, oh! .. not your code, but I haven't seen this before. There is
> >> always a destructor, not specifying one means you get the "free" one
> >> (default destructor).
> >>
> > 
> > yes, I think maybe should define a private destructor here.
> 
> Yes, I agree.
> 
> [snip]
> 
> >>> === modified file 'sql/sql_repl.cc'
> >>> --- a/sql/sql_repl.cc	2008-02-19 12:58:08 +0000
> >>> +++ b/sql/sql_repl.cc	2008-07-07 08:51:09 +0000
> >>> @@ -1667,6 +1667,7 @@ static sys_var_long_ptr	sys_slave_net_ti
> >>>  static sys_var_long_ptr	sys_slave_trans_retries(&vars,
> "slave_transaction_retries",
> >>>  						&slave_trans_retries);
> >>>  static sys_var_sync_binlog_period sys_sync_binlog_period(&vars,
> "sync_binlog", &sync_binlog_period);
> >>> +static sys_var_sync_binlog_period sys_sync_relaylog_period(&vars,
> "sync_relaylog", &sync_relaylog_period);
> >>>  static sys_var_slave_skip_counter sys_slave_skip_counter(&vars,
> "sql_slave_skip_counter");
> >>>  
> >>>  static int show_slave_skip_errors(THD *thd, SHOW_VAR *var, char
> *buff);
> >>> @@ -1759,7 +1760,7 @@ bool sys_var_slave_skip_counter::update(
> >>>  
> >>>  bool sys_var_sync_binlog_period::update(THD *thd, set_var *var)
> >>>  {
> >>> -  sync_binlog_period= (ulong) var->save_result.ulonglong_value;
> >>> +  *value= (ulong) var->save_result.ulonglong_value;
> >> Assume that there are two threads modifying this variable at the same
> >> time (say, to 0x8000 and 0x0040) as it is being read, and also that the
> >> variable is split into two parts for some reason. We can then have the
> >> following executions:
> >>
> > 
> > yes, and the code before my patch should suffer the same problem. Remove
> > this and use the inherited update method will solve this. 
> 
> Correct.
> 
> > I am wondering why this method was overrided at first place.
> 
> I'm not surprised that is was overridde, but rather how. The code for
> the replication variables does not acquire the mutex as the ::update
> functions in set_var.cc does.
> 
> Just my few cents,
> Mats Kindahl
> 
> -- 
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:    http://lists.mysql.com/commits?unsub=1