Hi Kay.
On 6 Jun 2007, at 06:08, Kay Roepke wrote:
> Below is a patch for the new replication delay feature (see <http://
> bugs.mysql.com/bug.php?id=28760>, <http://forge.mysql.com/worklog/
> task.php?id=344> for implementation details).
Fantastic! Thank you.
> The diff is against mysql-5.1-bk from last night, so it should
> apply cleanly.
> It implements setting the replication delay by adding a new option
> to the CHANGE MASTER statement, but currently doesn't add any new
> options (like connect_retry and the like are).
>
> The delay works as follows:
> If the slave is already lagging behind, the delay will be the
> difference between the current lag and delay time specified via
> CHANGE MASTER TO MASTER_DELAY=xxx. Essentially the delay is a lower
> bound for the replication lag.
>
> There are some points I'd like to clarify before I consider the
> patch "ready":
> a) Should the delay setting be written to master.info and if it
> should, in which line should it go.
> The reason I ask is because the SSL settings are optional in that
> file and adding 'delay' at the end could cause compatibility problems.
> However adding it in the middle seems like a worse idea still.
Yes, you must write it to master.info so that it's restored on recovery.
It doesn't understand items that are not in order, and this includes
SSL. The delay setting must go at the end, after SSL settings, which
must exist as a prerequisite.
> b) While testing I noticed that the first log_event received by the
> slave is delayed by a shorter time than the delay parameter, but
> the second event will be delayed by the full amount specified. From
> the relay logs it seemed to me that rotate_log events where delayed
> also, does that make sense? At first sight I couldn't see a way to
> differentiate between query and other events, but I might have
> missed something.
You should be able to peek inside the event to see what type it is.
Lars notes that rotate-log events can be created by the I/O thread,
and so shouldn't be delayed.
> The test scenario I used was:
> preparation) master/slave: use test; create table test (a int);
> 1) start master on port 33306
> 2) start slave (port 33307) replicating db 'test' with a
> master.info already configured.
> 3) slave: stop slave;
> 4) slave: change master to master_delay=30; # because that info
> is not yet in master.info
> 5) slave: start slave;
> 6) master: insert into test set a=42;
> 7) slave: delaying some event (looks like rotate_log to me) by some
> 3-8 secs, afterwards delaying the insert for the full 30 secs.
>
> This hasn't happened afterwards, though I have not yet generated a
> big amount of statements to cause log rotation. I'd like to gather
> some advice first ;)
>
> c) Should there be a command line option for master_delay?
No. Slaves will eventually be permitted to have many masters, and
that wouldn't map very well.
> d) Is the format of the patch ok, I had problems sending
> attachments to the list, so I put it in the body...
Perhaps there were problems: I notice several places where there are
tabs, and our style guidelines forbid them in code. I noted them.
Outside of that, I have only minor complaints with this first
version. I'm now eager to see the finished product.
- chad
> diff -urN mysql-5.1-bk-clean/sql/lex.h mysql-5.1-bk/sql/lex.h
> --- mysql-5.1-bk-clean/sql/lex.h 2007-04-02 10:54:27.000000000 +0200
> +++ mysql-5.1-bk/sql/lex.h 2007-06-06 01:11:43.000000000 +0200
> @@ -300,6 +300,7 @@
> { "LOW_PRIORITY", SYM(LOW_PRIORITY)},
> { "MASTER", SYM(MASTER_SYM)},
> { "MASTER_CONNECT_RETRY", SYM(MASTER_CONNECT_RETRY_SYM)},
> + { "MASTER_DELAY", SYM(MASTER_DELAY_SYM)},
> { "MASTER_HOST", SYM(MASTER_HOST_SYM)},
> { "MASTER_LOG_FILE", SYM(MASTER_LOG_FILE_SYM)},
> { "MASTER_LOG_POS", SYM(MASTER_LOG_POS_SYM)},
> diff -urN mysql-5.1-bk-clean/sql/mysqld.cc mysql-5.1-bk/sql/mysqld.cc
> --- mysql-5.1-bk-clean/sql/mysqld.cc 2007-06-04 12:14:18.000000000
> +0200
> +++ mysql-5.1-bk/sql/mysqld.cc 2007-06-06 03:02:56.000000000 +0200
> @@ -574,7 +574,7 @@
> File_parser_dummy_hook file_parser_dummy_hook;
> /* replication parameters, if master_host is not NULL, we are a
> slave */
> -uint master_port= MYSQL_PORT, master_connect_retry = 60;
> +uint master_port= MYSQL_PORT, master_connect_retry= 60,
> master_delay= 0;
Think of the resulting diff when changing lines. Avoid removing a
line and adding a line, because a change to the removed line would
cause merge conflicts. There are a lot of developers working on
mysql source each with dozens of trees, and this is a real problem.
> uint report_port= MYSQL_PORT;
> ulong master_retry_count=0;
> char *master_user, *master_password, *master_host, *master_info_file;
> diff -urN mysql-5.1-bk-clean/sql/rpl_mi.cc mysql-5.1-bk/sql/rpl_mi.cc
> --- mysql-5.1-bk-clean/sql/rpl_mi.cc 2007-04-16 18:16:13.000000000
> +0200
> +++ mysql-5.1-bk/sql/rpl_mi.cc 2007-06-06 03:03:36.000000000 +0200
> @@ -30,7 +30,8 @@
> MASTER_INFO::MASTER_INFO()
> :ssl(0), fd(-1), io_thd(0), inited(0),
> abort_slave(0),slave_running(0),
> - ssl_verify_server_cert(0), slave_run_id(0)
> + ssl_verify_server_cert(0), slave_run_id(0),
> + delay(0)
Same here. "delay(0)," before the end would avert a merge conflict.
> {
> host[0] = 0; user[0] = 0; password[0] = 0;
> ssl_ca[0]= 0; ssl_capath[0]= 0; ssl_cert[0]= 0;
> @@ -67,8 +68,9 @@
> strmake(mi->user, master_user, sizeof(mi->user) - 1);
> if (master_password)
> strmake(mi->password, master_password, MAX_PASSWORD_LENGTH);
> - mi->port = master_port;
> - mi->connect_retry = master_connect_retry;
> + mi->port= master_port;
> + mi->connect_retry= master_connect_retry;
I see you're also changing old code to conform with our coding
standards. Thank you.
> + mi->delay= master_delay;
> mi->ssl= master_ssl;
> if (master_ssl_ca)
> diff -urN mysql-5.1-bk-clean/sql/rpl_mi.h mysql-5.1-bk/sql/rpl_mi.h
> --- mysql-5.1-bk-clean/sql/rpl_mi.h 2007-04-12 09:10:37.000000000
> +0200
> +++ mysql-5.1-bk/sql/rpl_mi.h 2007-06-06 03:03:10.000000000 +0200
> @@ -99,6 +99,11 @@
> */
> long clock_diff_with_master;
> + /*
> + Delay replication by this amount (in seconds)
> + */
> + long delay;
Why signed? Could this be time_t? More notes about this later.
We propose to use Doxygen style now, and I hope to merge Doxygen
changes into our trees soon. For Doxygen format, I'd write it like
this:
long delay; ///< Replication delay, in seconds
...only for the comments you add. The rest are already taken care
of, elsewhere.
> +
> };
> void init_master_info_with_options(MASTER_INFO* mi);
> diff -urN mysql-5.1-bk-clean/sql/slave.cc mysql-5.1-bk/sql/slave.cc
> --- mysql-5.1-bk-clean/sql/slave.cc 2007-05-31 16:45:16.000000000
> +0200
> +++ mysql-5.1-bk/sql/slave.cc 2007-06-06 02:56:30.000000000 +0200
> @@ -1212,6 +1212,8 @@
> MYSQL_TYPE_LONG));
> field_list.push_back(new Item_return_int("Connect_Retry", 10,
> MYSQL_TYPE_LONG));
> + field_list.push_back(new Item_return_int("Delay", 10,
> + MYSQL_TYPE_LONG));
As Mark noted, avoid reordering them.
> field_list.push_back(new Item_empty_string("Master_Log_File",
> FN_REFLEN));
> field_list.push_back(new Item_return_int("Read_Master_Log_Pos", 10,
> @@ -1282,6 +1284,7 @@
> protocol->store(mi->user, &my_charset_bin);
> protocol->store((uint32) mi->port);
> protocol->store((uint32) mi->connect_retry);
> + protocol->store((uint32) mi->delay);
Just throwing out ideas: Could you redefine delay to be uint32? We
hate casts, and want as few as possible.
> protocol->store(mi->master_log_name, &my_charset_bin);
> protocol->store((ulonglong) mi->master_log_pos);
> protocol->store(mi->rli.group_relay_log_name +
> @@ -1784,7 +1787,20 @@
> --rli->slave_skip_counter;
> pthread_mutex_unlock(&rli->data_lock);
> if (reason == Log_event::EVENT_SKIP_NOT)
> + {
> + long now= (long)((time_t)time((time_t*) 0));
Ugh. With a different type, could this just be
whatever_type now;
(void) time(now)?
> + DBUG_PRINT("info", ("ev->when= %lu now= %lu delay= %lu",
> + (long)ev->when, now, rli->mi->delay));
Please don't insert tab characters. Your editor probably has this as
an option.
> + if ((rli->mi->delay != 0) &&
> + ((now - ev->when) < rli->mi->delay))
> + {
> + DBUG_PRINT("info", ("delaying replication event %lu secs",
> + rli->mi->delay - (now - ev->when)));
> + safe_sleep(thd,rli->mi->delay - (now - ev->when),
> (CHECK_KILLED_FUNC)sql_slave_killed,
> + (void*)rli);
> + }
> exec_res= ev->apply_event(rli);
> + }
> #ifndef DBUG_OFF
> /*
> This only prints information to the debug trace.
> diff -urN mysql-5.1-bk-clean/sql/slave.h mysql-5.1-bk/sql/slave.h
> --- mysql-5.1-bk-clean/sql/slave.h 2007-05-10 11:59:28.000000000 +0200
> +++ mysql-5.1-bk/sql/slave.h 2007-06-06 01:16:02.000000000 +0200
> @@ -200,7 +200,7 @@
> extern int disconnect_slave_event_count, abort_slave_event_count ;
> /* the master variables are defaults read from my.cnf or command
> line */
> -extern uint master_port, master_connect_retry, report_port;
> +extern uint master_port, master_connect_retry, master_delay,
> report_port;
> extern char * master_user, *master_password, *master_host;
> extern char *master_info_file, *relay_log_info_file, *report_user;
> extern char *report_host, *report_password;
> diff -urN mysql-5.1-bk-clean/sql/sql_lex.h mysql-5.1-bk/sql/sql_lex.h
> --- mysql-5.1-bk-clean/sql/sql_lex.h 2007-06-04 12:03:13.000000000
> +0200
> +++ mysql-5.1-bk/sql/sql_lex.h 2007-06-06 01:29:59.000000000 +0200
> @@ -186,6 +186,7 @@
> {
> char *host, *user, *password, *log_file_name;
> uint port, connect_retry;
> + long delay;
> ulonglong pos;
> ulong server_id;
> /*
> diff -urN mysql-5.1-bk-clean/sql/sql_repl.cc mysql-5.1-bk/sql/
> sql_repl.cc
> --- mysql-5.1-bk-clean/sql/sql_repl.cc 2007-05-24
> 00:39:24.000000000 +0200
> +++ mysql-5.1-bk/sql/sql_repl.cc 2007-06-06 01:56:43.000000000 +0200
> @@ -1130,7 +1130,9 @@
> mi->port = lex_mi->port;
> if (lex_mi->connect_retry)
> mi->connect_retry = lex_mi->connect_retry;
> -
> + if (lex_mi->delay)
> + mi->delay= lex_mi->delay;
> +
The difference in blank lines is that one has a tab character in it.
> if (lex_mi->ssl != LEX_MASTER_INFO::SSL_UNCHANGED)
> mi->ssl= (lex_mi->ssl == LEX_MASTER_INFO::SSL_ENABLE);
> diff -urN mysql-5.1-bk-clean/sql/sql_yacc.yy mysql-5.1-bk/sql/
> sql_yacc.yy
> --- mysql-5.1-bk-clean/sql/sql_yacc.yy 2007-06-04
> 12:14:21.000000000 +0200
> +++ mysql-5.1-bk/sql/sql_yacc.yy 2007-06-06 01:10:35.000000000 +0200
> @@ -769,6 +769,7 @@
> %token LOW_PRIORITY
> %token LT /* OPERATOR */
> %token MASTER_CONNECT_RETRY_SYM
> +%token MASTER_DELAY_SYM
> %token MASTER_HOST_SYM
> %token MASTER_LOG_FILE_SYM
> %token MASTER_LOG_POS_SYM
> @@ -1490,6 +1491,11 @@
> {
> Lex->mi.connect_retry = $3;
> }
> + |
> + MASTER_DELAY_SYM EQ ulong_num
> + {
> + Lex->mi.delay = $3;
> + }
> | MASTER_SSL_SYM EQ ulong_num
> {
> Lex->mi.ssl= $3 ?
> @@ -9953,6 +9959,7 @@
> | MASTER_PASSWORD_SYM {}
> | MASTER_SERVER_ID_SYM {}
> | MASTER_CONNECT_RETRY_SYM {}
> + | MASTER_DELAY_SYM {}
> | MASTER_SSL_SYM {}
> | MASTER_SSL_CA_SYM {}
> | MASTER_SSL_CAPATH_SYM {}
--
Chad Miller, Software Developer chad@stripped
MySQL Inc., www.mysql.com
Orlando, Florida, USA 13-20z, UTC-0400
Office: +1 408 213 6740 sip:6740@stripped
Attachment: [application/pgp-signature] This is a digitally signed message part PGP.sig
