maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "knielsen" == knielsen  <knielsen@xxxxxxxxxxxxxxx> writes:

knielsen> At http://bazaar.launchpad.net/~maria-captains/maria/5.1
knielsen> ------------------------------------------------------------
knielsen> revno: 2864
knielsen> revision-id: knielsen@xxxxxxxxxxxxxxx-20101107213743-3luszsivft2vt7t6
knielsen> parent: knielsen@xxxxxxxxxxxxxxx-20101103155438-vdou0fngj6hpgpsd
knielsen> committer: knielsen@xxxxxxxxxxxxxxx
knielsen> branch nick: work-5.1-mwl136
knielsen> timestamp: Sun 2010-11-07 22:37:43 +0100
knielsen> message:
knielsen>   MWL#136: Cross-engine consistency for START TRANSACTION WITH CONSISTENT SNAPSHOT
  
<cut>

knielsen> === modified file 'client/mysqldump.c'

<cut>
 
knielsen> -static int do_show_master_status(MYSQL *mysql_con)
knielsen> +static int do_show_master_status(MYSQL *mysql_con, int consistent_binlog_pos)
knielsen>  {
knielsen>    MYSQL_ROW row;
knielsen>    MYSQL_RES *master;
knielsen> +  char binlog_pos_file[FN_REFLEN];
knielsen> +  char binlog_pos_offset[LONGLONG_LEN+1];
knielsen> +  char *file, *offset;
knielsen>    const char *comment_prefix=
knielsen>      (opt_master_data == MYSQL_OPT_MASTER_DATA_COMMENTED_SQL) ? "-- " : "";
knielsen> -  if (mysql_query_with_error_report(mysql_con, &master, "SHOW MASTER STATUS"))
knielsen> +
knielsen> +  if (consistent_binlog_pos)
knielsen>    {
knielsen> -    return 1;
knielsen> +    if(!check_consistent_binlog_pos(binlog_pos_file, binlog_pos_offset))
knielsen> +      return 1;
knielsen> +    file= binlog_pos_file;
knielsen> +    offset= binlog_pos_offset;
knielsen>    }
knielsen>    else
knielsen>    {
knielsen> +    if (mysql_query_with_error_report(mysql_con, &master, "SHOW MASTER STATUS"))
knielsen> +      return 1;
knielsen> +
knielsen>      row= mysql_fetch_row(master);
knielsen>      if (row && row[0] && row[1])
knielsen>      {
knielsen> +      file= row[0];
knielsen> +      offset= row[1];
knielsen>      }
knielsen> +    else
knielsen>      {

You need a mysql_free_result(master) here.

knielsen> +      if (!ignore_errors)
knielsen> +      {
knielsen> +        /* SHOW MASTER STATUS reports nothing and --force is not enabled */
knielsen> +        my_printf_error(0, "Error: Binlogging on server not active",
knielsen> +                        MYF(0));
knielsen> +        maybe_exit(EX_MYSQLERR);
knielsen> +        return 1;
knielsen> +      }
knielsen> +      else
knielsen> +      {
knielsen> +        return 0;
knielsen> +      }
knielsen>      }
knielsen>    }

<cut>

knielsen> === modified file 'sql/log.cc'
knielsen> --- a/sql/log.cc	2010-11-02 07:40:27 +0000
knielsen> +++ b/sql/log.cc	2010-11-07 21:37:43 +0000
knielsen> @@ -62,6 +62,7 @@ static int binlog_savepoint_rollback(han
knielsen>  static int binlog_commit(handlerton *hton, THD *thd, bool all);
knielsen>  static int binlog_rollback(handlerton *hton, THD *thd, bool all);
knielsen>  static int binlog_prepare(handlerton *hton, THD *thd, bool all);
knielsen> +static int binlog_start_consistent_snapshot(handlerton *hton, THD *thd);
 
knielsen>  /**
knielsen>    Silence all errors and warnings reported when performing a write
knielsen> @@ -155,9 +156,10 @@ class binlog_trx_data {
knielsen>  public:
knielsen>    binlog_trx_data()
knielsen>      : at_least_one_stmt_committed(0), incident(FALSE), m_pending(0),
knielsen> -    before_stmt_pos(MY_OFF_T_UNDEF), commit_bin_log_file_pos(0), using_xa(0)
knielsen> +    before_stmt_pos(MY_OFF_T_UNDEF), last_commit_pos_offset(0), using_xa(0)
knielsen>    {
knielsen>      trans_log.end_of_file= max_binlog_cache_size;
knielsen> +    strcpy(last_commit_pos_file, "");

strcpy() -> strmov() or even better

last_commit_pos_file[0]= 0;

(I want to get rid of all strcpy() and replace with strmov() as
strcpy() is often used wrongly;   I would like to have this thing
changed to not get 'false positives' when searching for strcpy() to remove.

knielsen>    }
 
knielsen>    ~binlog_trx_data()
knielsen> @@ -215,7 +217,8 @@ public:
knielsen>      incident= FALSE;
knielsen>      trans_log.end_of_file= max_binlog_cache_size;
knielsen>      using_xa= FALSE;
knielsen> -    commit_bin_log_file_pos= 0;
knielsen> +    strcpy(last_commit_pos_file, "");

strcpy -> strmov or last_commit_pos_file[0]= 0;

knielsen> +    last_commit_pos_offset= 0;
knielsen>      DBUG_ASSERT(empty());
knielsen>    }
 
knielsen>  /*
knielsen>    Write a table map to the binary log.
knielsen> @@ -4337,6 +4369,9 @@ MYSQL_BIN_LOG::flush_and_set_pending_row
knielsen>          rotate_and_purge(RP_LOCK_LOG_IS_ALREADY_LOCKED);
knielsen>        }
 
Add a comment that this mutex is a protection for partial reads of
last_commit_pos_offset on 32 bit systems. (at least I can't come up
with any better reason for this mutex).

Maybe we should introduce
pthread_mutex_lock_for_32_bit() that would be void on 64 bit systems
in cases like this.


knielsen> +      pthread_mutex_lock(&LOCK_commit_ordered);
knielsen> +      last_commit_pos_offset= my_b_tell(&log_file);
knielsen> +      pthread_mutex_unlock(&LOCK_commit_ordered);
knielsen>        pthread_mutex_unlock(&LOCK_log);
knielsen>      }
 
knielsen> @@ -4526,7 +4561,12 @@ bool MYSQL_BIN_LOG::write(Log_event *eve
 
knielsen>  err_unlock:
knielsen>      if (file == &log_file)
knielsen> +    {
knielsen> +      pthread_mutex_lock(&LOCK_commit_ordered);
knielsen> +      last_commit_pos_offset= my_b_tell(&log_file);
knielsen> +      pthread_mutex_unlock(&LOCK_commit_ordered);
knielsen>        pthread_mutex_unlock(&LOCK_log);
knielsen> +    }

Same comment needed for this.
 
knielsen>  err:
knielsen>      if (error)
knielsen> @@ -4827,6 +4867,9 @@ bool MYSQL_BIN_LOG::write_incident(THD *
knielsen>      signal_update();
knielsen>      rotate_and_purge(RP_LOCK_LOG_IS_ALREADY_LOCKED);
knielsen>    }
knielsen> +  pthread_mutex_lock(&LOCK_commit_ordered);
knielsen> +  last_commit_pos_offset= my_b_tell(&log_file);
knielsen> +  pthread_mutex_unlock(&LOCK_commit_ordered);
knielsen>    pthread_mutex_unlock(&LOCK_log);
 
and here...

knielsen>  static ulonglong binlog_status_var_num_commits;
knielsen>  static ulonglong binlog_status_var_num_group_commits;
knielsen> +static char binlog_trx_file[FN_REFLEN];
knielsen> +static ulonglong binlog_trx_position;
 
knielsen>  static SHOW_VAR binlog_status_vars_detail[]=
knielsen>  {
knielsen> @@ -6562,12 +6609,16 @@ static SHOW_VAR binlog_status_vars_detai
knielsen>      (char *)&binlog_status_var_num_commits, SHOW_LONGLONG},
knielsen>    {"group_commits",
knielsen>      (char *)&binlog_status_var_num_group_commits, SHOW_LONGLONG},
knielsen> +  {"trx_file",
knielsen> +    (char *)&binlog_trx_file, SHOW_CHAR},
knielsen> +  {"trx_position",
knielsen> +   (char *)&binlog_trx_position, SHOW_LONGLONG},
knielsen>    {NullS, NullS, SHOW_LONG}
knielsen>  };
 
knielsen>  static int show_binlog_vars(THD *thd, SHOW_VAR *var, char *buff)
knielsen>  {
knielsen> -  mysql_bin_log.set_status_variables();
knielsen> +  mysql_bin_log.set_status_variables(thd);
knielsen>    var->type= SHOW_ARRAY;
knielsen>    var->value= (char *)&binlog_status_vars_detail;
knielsen>    return 0;
knielsen> @@ -6606,17 +6657,31 @@ static struct st_mysql_sys_var *binlog_s
knielsen>    This is called only under LOCK_status, so we can fill in a static array.
knielsen>  */
knielsen>  void
knielsen> -TC_LOG_BINLOG::set_status_variables()
knielsen> +TC_LOG_BINLOG::set_status_variables(THD *thd)
knielsen>  {
knielsen> -  ulonglong num_commits, num_group_commits;
knielsen> +  binlog_trx_data *trx_data;
knielsen> +
knielsen> +  if (thd)
knielsen> +    trx_data= (binlog_trx_data*) thd_get_ha_data(thd, binlog_hton);
knielsen> +  else
knielsen> +    trx_data= NULL;
 
knielsen>    pthread_mutex_lock(&LOCK_commit_ordered);
knielsen> -  num_commits= this->num_commits;
knielsen> -  num_group_commits= this->num_group_commits;
knielsen> +  binlog_status_var_num_commits= this->num_commits;
knielsen> +  binlog_status_var_num_group_commits= this->num_group_commits;
knielsen> +  if (!trx_data || 0 == strcmp(trx_data->last_commit_pos_file, ""))

-> if (!trx_data || trx_data->last_commit_pos_file[0] == 0)


knielsen> +  {
knielsen> +    strmake(binlog_trx_file, last_commit_pos_file, sizeof(binlog_trx_file)-1);
knielsen> +    binlog_trx_position= last_commit_pos_offset;
knielsen> +  }
knielsen>    pthread_mutex_unlock(&LOCK_commit_ordered);
 
knielsen> +  if (trx_data && 0 != strcmp(trx_data->last_commit_pos_file, ""))

-> if (trx_data && trx_data->last_commit_pos_file[0] != 0)

It was a bit tricky to see that this was a negative test for the
earlier one.  I would sugges to change first to:


-> if ((binlog_pos_set= (!trx_data || trx_data->last_commit_pos_file[0] == 0))

and the later to

if (!binlog_pos_set)

knielsen> +  {
knielsen> +    strmake(binlog_trx_file, trx_data->last_commit_pos_file,
knielsen> +            sizeof(binlog_trx_file)-1);
knielsen> +    binlog_trx_position= trx_data->last_commit_pos_offset;
knielsen> +  }
knielsen>  }
 
<cut>

Regards,
Monty