maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

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

<cut>

We agree to change the name of replicate-ignore-do-not-replicate
variable to just 'replicate' with a default value of 1.
(Double negatives are hard, especially if you test them with !
in the code..)

knielsen>   MWL#234: Support for marking binlog events to not be replicated, and for telling slaves not to replicate events with such mark

knielsen> === modified file 'client/mysqlbinlog.cc'
knielsen> --- a/client/mysqlbinlog.cc	2011-05-03 16:10:10 +0000
knielsen> +++ b/client/mysqlbinlog.cc	2011-08-11 09:38:52 +0000
knielsen> @@ -671,6 +671,31 @@ print_use_stmt(PRINT_EVENT_INFO* pinfo,
 
knielsen>  /**
knielsen> +   Print "SET do_not_replicate=..." statement when needed.
knielsen> +
knielsen> +   Not all servers support this (only MariaDB from some version on). So we
knielsen> +   mark the SET to only execute from the version of MariaDB that supports it,
knielsen> +   and also only output it if we actually see events with the flag set, to not
knielsen> +   get spurious errors on MySQL@Oracle servers of higher version that do not
knielsen> +   support the flag.
knielsen> +
knielsen> +   So we start out assuming @@do_not_replicate is 0, and only output a SET
knielsen> +   statement when it changes.
knielsen> +*/

I have now added to MariaDB 5.3 the following new comment syntax to allow one
to do MariaDB specific executable comments:

/* !M##### */

<cut>

knielsen> @@ -927,6 +958,7 @@ Exit_status process_event(PRINT_EVENT_IN
knielsen>        if (!shall_skip_database(exlq->db))
knielsen>        {
knielsen>          print_use_stmt(print_event_info, exlq);
knielsen> +        print_do_not_replicate_statement(print_event_info, ev);
knielsen>          if (fname)
knielsen>          {

Move the print_do_not_replicate_statement to just before
exlq->print(result_file, print_event_info, fname);

knielsen>            convert_path_to_forward_slashes(fname);
knielsen> @@ -1030,6 +1062,12 @@ Exit_status process_event(PRINT_EVENT_IN
knielsen>        }
knielsen>        /* FALL THROUGH */
knielsen>      }
knielsen> +    case INTVAR_EVENT:
knielsen> +    case RAND_EVENT:
knielsen> +    case USER_VAR_EVENT:
knielsen> +    case XID_EVENT:
knielsen> +      print_do_not_replicate_statement(print_event_info, ev);
knielsen> +      /* Fall through ... */
knielsen>      default:
knielsen>        ev->print(result_file, print_event_info);
knielsen>      }

Would it not be safer to have
print_do_not_replicate_statement(print_event_info, ev);
after the default so that we don't miss anything.
(Having a few sporadic 'do_not_replicate=#' events is better than
missing something.

knielsen> === modified file 'configure.in'
knielsen> --- a/configure.in	2011-06-11 11:28:37 +0000
knielsen> +++ b/configure.in	2011-08-11 09:38:52 +0000
knielsen> @@ -13,7 +13,7 @@ dnl When changing the major version numb
knielsen>  dnl statement in mysqlbinlog::check_master_version().  You may also need
knielsen>  dnl to update version.c in ndb.
 
knielsen> -AC_INIT([MariaDB Server], [5.2.7-MariaDB], [], [mysql])
knielsen> +AC_INIT([MariaDB Server], [5.4.0-MariaDB], [], [mysql])
 
5.4, that was intersting ;)

(Should probably not be part of the patch you send the customer)

knielsen> --- a/sql/mysql_priv.h	2011-07-21 10:15:09 +0000
<cut>

knielsen> @@ -2064,6 +2065,7 @@ extern my_bool opt_old_style_user_limits
knielsen>  extern uint opt_crash_binlog_innodb;
knielsen>  extern char *shared_memory_base_name, *mysqld_unix_port;
knielsen>  extern my_bool opt_enable_shared_memory;
knielsen> +extern my_bool opt_replicate_ignore_do_not_replicate;

Not a perfect name (as we both agree upon :)

<cut>

knielsen> +++ b/sql/mysqld.cc	2011-08-11 09:38:52 +0000
knielsen> @@ -553,6 +553,8 @@ uint    opt_large_page_size= 0;
knielsen>  uint    opt_debug_sync_timeout= 0;
knielsen>  #endif /* defined(ENABLED_DEBUG_SYNC) */
knielsen>  my_bool opt_old_style_user_limits= 0, trust_function_creators= 0;
knielsen> +my_bool opt_replicate_ignore_do_not_replicate;
knielsen> +
knielsen>  /*
knielsen>    True if there is at least one per-hour limit for some user, so we should
knielsen>    check them before each query (and possibly reset counters when hour is
knielsen> @@ -6085,7 +6087,8 @@ enum options_mysqld
knielsen>    OPT_IGNORE_BUILTIN_INNODB,
knielsen>    OPT_BINLOG_DIRECT_NON_TRANS_UPDATE,
knielsen>    OPT_DEFAULT_CHARACTER_SET_OLD,
knielsen> -  OPT_MAX_LONG_DATA_SIZE
knielsen> +  OPT_MAX_LONG_DATA_SIZE,
knielsen> +  OPT_REPLICATE_IGNORE_DO_NOT_REPLICATE

Add the option somewhere close to other replication options.
Benefits:
- Only one row changed
- Will not conflict if MySQL add more options or if we add more
  options in later MariaDB versions.

knielsen> === modified file 'sql/slave.cc'
knielsen> --- a/sql/slave.cc	2011-05-03 16:10:10 +0000
knielsen> +++ b/sql/slave.cc	2011-08-11 09:38:52 +0000
knielsen> @@ -1176,6 +1176,38 @@ when it try to get the value of TIME_ZON
knielsen>      }
knielsen>    }
 
knielsen> +  /*
knielsen> +    Request the master to filter away events with the @@do_not_replicate flag
knielsen> +    set, if we are running with --replicate-ignore-do_not_replicate=1.
knielsen> +  */
knielsen> +  if (opt_replicate_ignore_do_not_replicate)
knielsen> +  {
knielsen> +    if (!mysql_real_query(mysql, STRING_WITH_LEN("SET do_not_replicate=1")))
knielsen> +    {
knielsen> +      err_code= mysql_errno(mysql);
knielsen> +      if (is_network_error(err_code))
knielsen> +      {
knielsen> +        mi->report(ERROR_LEVEL, err_code,
knielsen> +                   "Setting master-side filtering of @@do_not_replicate failed "
knielsen> +                   "with error: %s", mysql_error(mysql));
knielsen> +        goto network_err;
knielsen> +      }
knielsen> +      else if (err_code == ER_UNKNOWN_SYSTEM_VARIABLE)
knielsen> +      {
knielsen> +        /*
knielsen> +          The master is older than the slave and does not support the
knielsen> +          @@do_not_replicate feature.
knielsen> +          This is not a problem, as such master will not generate events with
knielsen> +          the @@do_not_replicate flag set in the first place. We will still
knielsen> +          do slave-side filtering of such events though, to handle the (rare)
knielsen> +          case of downgrading a master and receiving old events generated from
knielsen> +          before the downgrade with the @@do_not_replicate flag set.
knielsen> +        */
knielsen> +        DBUG_PRINT("info", ("Old master does not support master-side filtering "
knielsen> +                            "of @@do_not_replicate events."));
knielsen> +      }
knielsen> +    }
knielsen> +  }

Wouldn't it be better to do:

if (err_code == ER_UNKNOWN_SYSTEM_VARIABLE)
...
else
...

Now you don't print an error if there was some other unknown problem
with setting the variable.

What other errors can we get other than ER_UNKNOWN_SYSTEM_VARIABLE
that would allow us to continue ?

knielsen>  err:
knielsen>    if (errmsg)
knielsen>    {
knielsen> @@ -2114,6 +2146,8 @@ int apply_event_and_update_pos(Log_event
knielsen>    thd->lex->current_select= 0;
knielsen>    if (!ev->when)
knielsen>      ev->when= my_time(0);
knielsen> +  thd->options= (thd->options & ~OPTION_DO_NOT_REPLICATE) |
knielsen> +    (ev->flags & LOG_EVENT_DO_NOT_REPLICATE_F ? OPTION_DO_NOT_REPLICATE : 0);
knielsen>    ev->thd = thd; // because up to this point, ev->thd == 0
 
knielsen>    int reason= ev->shall_skip(rli);
knielsen> @@ -3582,6 +3616,7 @@ static int queue_event(Master_info* mi,c
knielsen>  {
knielsen>    int error= 0;
knielsen>    ulong inc_pos;
knielsen> +  ulong event_pos;
knielsen>    Relay_log_info *rli= &mi->rli;
knielsen>    pthread_mutex_t *log_lock= rli->relay_log.get_log_lock();
knielsen>    DBUG_ENTER("queue_event");
knielsen> @@ -3667,6 +3702,23 @@ static int queue_event(Master_info* mi,c
knielsen>    }
 
knielsen>    /*
knielsen> +    If we filter events master-side (eg. @@do_not_replicate), we will see holes
knielsen> +    in the event positions from the master. If we see such a hole, adjust
knielsen> +    mi->master_log_pos accordingly so we maintain the correct position (for
knielsen> +    reconnect, MASTER_POS_WAIT(), etc.)
knielsen> +  */
knielsen> +  if (inc_pos > 0 &&
knielsen> +      event_len >= LOG_POS_OFFSET+4 &&
knielsen> +      (event_pos= uint4korr(buf+LOG_POS_OFFSET)) > mi->master_log_pos + inc_pos)
knielsen> +  {
knielsen> +    inc_pos= event_pos - mi->master_log_pos;
knielsen> +    DBUG_PRINT("info", ("Adjust master_log_pos %lu->%lu to account for "
knielsen> +                        "master-side filtering",
knielsen> +                        (unsigned long)(mi->master_log_pos + inc_pos),
knielsen> +                        event_pos));
knielsen> +  }

<irc discussion about>

is  'inc_pos= event_pos - mi->master_log_pos' really secure and should
we add
DBUG_ASSERT(mi->master_log_pos == uint4korr(buf+LOG_POS_OFFSET) ||
inc_len == 0)
after
mi->master_log_pos+= inc_pos;

to ensure the correctness.

For the moment we decided to not do this.

<cut>

knielsen> +++ b/sql/sql_repl.cc	2011-08-11 09:38:52 +0000
knielsen> @@ -338,6 +338,41 @@ Increase max_allowed_packet on master";
 
 
knielsen>  /*
knielsen> +  Helper function for mysql_binlog_send() to write an event down the slave
knielsen> +  connection.
knielsen> +
knielsen> +  Returns NULL on success, error message string on error.
knielsen> +*/
knielsen> +static const char *
knielsen> +send_event_to_slave(THD *thd, NET *net, String* const packet)
knielsen> +{
knielsen> +  thd_proc_info(thd, "Sending binlog event to slave");
knielsen> +
knielsen> +  /*
knielsen> +    Skip events with the @@do_not_replicate flag set, if slave requested
knielsen> +    skipping of such events.
knielsen> +  */
knielsen> +  if (thd->options & OPTION_DO_NOT_REPLICATE)
knielsen> +  {
knielsen> +    uint16 flags= uint2korr(&((*packet)[FLAGS_OFFSET+1]));

Please add a comment to explain the +1 above

<cut>

Looks good!

Regards,
Monty