Hi Sven,
Nice Work.
Please find my review comments below.
STATUS
------
Not approved.
REQUIRED CHANGES
----------------
RC1. There seems to be a dangling function declaration in
sql/repl_failsafe.h:
bool show_new_master(THD* thd);
Please remove this.
RC2. Please, update comments in the source code that refer to show
new master instruction. I can only find one place (sql_lex.h):
271 /**
272 Structure to hold parameters for CHANGE MASTER or START/STOP SLAVE
273 or SHOW NEW MASTER.
274
275 Remark: this should not be confused with Master_info (and perhaps
276 would better be renamed to st_lex_replication_info). Some fields,
277 e.g., delay, are saved in Relay_log_info, not in Master_info.
278 */
279 typedef struct st_lex_master_info
RC3. Please, fix failing test cases:
- main.signal_code
- main.sp-code
Details at:
http://fimafeng11.norway.sun.com:8080/job/mysql-5.5/27/consoleText
REQUESTS
--------
n/a
SUGGESTIONS
-----------
n/a
DETAILS
-------
n/a
On 11/29/2010 11:04 AM, Sven Sandberg wrote:
> #At file:///home/sven/bzr/w5670-show_new_master/5.5-bugteam/ based on
> revid:tor.didriksen@stripped
>
> 3139 Sven Sandberg 2010-11-29
> WL#5670: Proposal: Remove SHOW NEW MASTER statement
> Removes SHOW NEW MASTER statement and all related code.
> Also removes the unused function update_slave_list from repl_failsafe.cc.
> @ sql/mysqld.cc
> Remove SQLCOM_SHOW_NEW_MASTER.
> @ sql/repl_failsafe.cc
> Remove show_new_master, which was only used by the removed SHOW NEW MASTER
> statement.
> Remove translate_master, which was only used by show_new_master.
> Remove find_slave_event, which was only used by translate_master.
> Remove find_target_pos, which was only used by translate_master.
> Remove cmp_master_pos, which was only used by translate_master.
> Remove update_slave_list, which was not used at all.
> @ sql/repl_failsafe.h
> Removed update_slave_list
> @ sql/sp_head.cc
> Removed SQLCOM_SHOW_NEW_MASTER
> @ sql/sql_lex.h
> Removed SQLCOM_SHOW_NEW_MASTER
> @ sql/sql_parse.cc
> Removed SQLCOM_SHOW_NEW_MASTER
> @ sql/sql_repl.cc
> Removed cmp_master_pos(char*,ulonglong,char*,ulonglong), which was
> only used by cmp_master_pos*Slave_log_event* sev, LEX_MASTER_INFO* mi) in
> repl_failsafe.cc,
> which has been removed.
> @ sql/sql_repl.h
> removed cmp_master_pos
> @ sql/sql_yacc.yy
> removed syntax SHOW NEW MASTER.
>
> modified:
> sql/mysqld.cc
> sql/repl_failsafe.cc
> sql/repl_failsafe.h
> sql/sp_head.cc
> sql/sql_lex.h
> sql/sql_parse.cc
> sql/sql_repl.cc
> sql/sql_repl.h
> sql/sql_yacc.yy
> === modified file 'sql/mysqld.cc'
> --- a/sql/mysqld.cc 2010-11-21 13:32:48 +0000
> +++ b/sql/mysqld.cc 2010-11-29 11:04:16 +0000
> @@ -3018,7 +3018,6 @@ SHOW_VAR com_status_vars[]= {
> {"show_grants", (char*) offsetof(STATUS_VAR, com_stat[(uint)
> SQLCOM_SHOW_GRANTS]), SHOW_LONG_STATUS},
> {"show_keys", (char*) offsetof(STATUS_VAR, com_stat[(uint)
> SQLCOM_SHOW_KEYS]), SHOW_LONG_STATUS},
> {"show_master_status", (char*) offsetof(STATUS_VAR, com_stat[(uint)
> SQLCOM_SHOW_MASTER_STAT]), SHOW_LONG_STATUS},
> - {"show_new_master", (char*) offsetof(STATUS_VAR, com_stat[(uint)
> SQLCOM_SHOW_NEW_MASTER]), SHOW_LONG_STATUS},
> {"show_open_tables", (char*) offsetof(STATUS_VAR, com_stat[(uint)
> SQLCOM_SHOW_OPEN_TABLES]), SHOW_LONG_STATUS},
> {"show_plugins", (char*) offsetof(STATUS_VAR, com_stat[(uint)
> SQLCOM_SHOW_PLUGINS]), SHOW_LONG_STATUS},
> {"show_privileges", (char*) offsetof(STATUS_VAR, com_stat[(uint)
> SQLCOM_SHOW_PRIVILEGES]), SHOW_LONG_STATUS},
>
> === modified file 'sql/repl_failsafe.cc'
> --- a/sql/repl_failsafe.cc 2010-10-11 14:50:14 +0000
> +++ b/sql/repl_failsafe.cc 2010-11-29 11:04:16 +0000
> @@ -56,10 +56,6 @@ const char* rpl_status_type[]=
> "RECOVERY_CAPTAIN","NULL",NullS
> };
>
> -static Slave_log_event* find_slave_event(IO_CACHE* log,
> - const char* log_file_name,
> - char* errmsg);
> -
> /*
> All of the functions defined in this file which are not used (the ones to
> handle failsafe) are not used; their code has not been updated for more than
> @@ -91,13 +87,6 @@ void change_rpl_status(ulong from_status
> }\
>
>
> -static inline int cmp_master_pos(Slave_log_event* sev, LEX_MASTER_INFO* mi)
> -{
> - return cmp_master_pos(sev->master_log, sev->master_pos,
> mi->log_file_name,
> - mi->pos);
> -}
> -
> -
> void unregister_slave(THD* thd, bool only_mine, bool need_mutex)
> {
> if (thd->server_id)
> @@ -228,361 +217,6 @@ void end_slave_list()
> }
> }
>
> -static int find_target_pos(LEX_MASTER_INFO *mi, IO_CACHE *log, char *errmsg)
> -{
> - my_off_t log_pos = (my_off_t) mi->pos;
> - uint32 target_server_id = mi->server_id;
> -
> - for (;;)
> - {
> - Log_event* ev;
> - if (!(ev= Log_event::read_log_event(log, (mysql_mutex_t*) 0, 0)))
> - {
> - if (log->error> 0)
> - strmov(errmsg, "Binary log truncated in the middle of event");
> - else if (log->error< 0)
> - strmov(errmsg, "I/O error reading binary log");
> - else
> - strmov(errmsg, "Could not find target event in the binary log");
> - return 1;
> - }
> -
> - if (ev->log_pos>= log_pos&& ev->server_id ==
> target_server_id)
> - {
> - delete ev;
> - mi->pos = my_b_tell(log);
> - return 0;
> - }
> - delete ev;
> - }
> - /* Impossible */
> -}
> -
> -/**
> - @details
> - Before 4.0.15 we had a member of THD called log_pos, it was meant for
> - failsafe replication code in repl_failsafe.cc which is disabled until
> - it is reworked. Event's log_pos used to be preserved through
> - log-slave-updates to make code in repl_failsafe.cc work (this
> - function, SHOW NEW MASTER); but on the other side it caused unexpected
> - values in Exec_Master_Log_Pos in A->B->C replication setup,
> - synchronization problems in master_pos_wait(), ... So we
> - (Dmitri& Guilhem) removed it.
> -
> - So for now this function is broken.
> -*/
> -
> -int translate_master(THD* thd, LEX_MASTER_INFO* mi, char* errmsg)
> -{
> - LOG_INFO linfo;
> - char last_log_name[FN_REFLEN];
> - IO_CACHE log;
> - File file = -1, last_file = -1;
> - mysql_mutex_t *log_lock;
> - const char* errmsg_p;
> - Slave_log_event* sev = 0;
> - my_off_t last_pos = 0;
> - int error = 1;
> - int cmp_res;
> - LINT_INIT(cmp_res);
> - DBUG_ENTER("translate_master");
> -
> - if (!mysql_bin_log.is_open())
> - {
> - strmov(errmsg,"Binary log is not open");
> - DBUG_RETURN(1);
> - }
> -
> - if (!server_id_supplied)
> - {
> - strmov(errmsg, "Misconfigured master - server id was not set");
> - DBUG_RETURN(1);
> - }
> -
> - if (mysql_bin_log.find_log_pos(&linfo, NullS, 1))
> - {
> - strmov(errmsg,"Could not find first log");
> - DBUG_RETURN(1);
> - }
> - thd->current_linfo =&linfo;
> -
> - bzero((char*)&log,sizeof(log));
> - log_lock = mysql_bin_log.get_log_lock();
> - mysql_mutex_lock(log_lock);
> -
> - for (;;)
> - {
> - if ((file=open_binlog(&log, linfo.log_file_name,&errmsg_p))< 0)
> - {
> - strmov(errmsg, errmsg_p);
> - goto err;
> - }
> -
> - if (!(sev = find_slave_event(&log, linfo.log_file_name, errmsg)))
> - goto err;
> -
> - cmp_res = cmp_master_pos(sev, mi);
> - delete sev;
> -
> - if (!cmp_res)
> - {
> - /* Copy basename */
> - fn_format(mi->log_file_name, linfo.log_file_name, "","",1);
> - mi->pos = my_b_tell(&log);
> - goto mi_inited;
> - }
> - else if (cmp_res> 0)
> - {
> - if (!last_pos)
> - {
> - strmov(errmsg,
> - "Slave event in first log points past the target position");
> - goto err;
> - }
> - end_io_cache(&log);
> - mysql_file_close(file, MYF(MY_WME));
> - if (init_io_cache(&log, (file = last_file), IO_SIZE, READ_CACHE, 0, 0,
> - MYF(MY_WME)))
> - {
> - errmsg[0] = 0;
> - goto err;
> - }
> - break;
> - }
> -
> - strmov(last_log_name, linfo.log_file_name);
> - last_pos = my_b_tell(&log);
> -
> - switch (mysql_bin_log.find_next_log(&linfo, 1)) {
> - case LOG_INFO_EOF:
> - if (last_file>= 0)
> - mysql_file_close(last_file, MYF(MY_WME));
> - last_file = -1;
> - goto found_log;
> - case 0:
> - break;
> - default:
> - strmov(errmsg, "Error reading log index");
> - goto err;
> - }
> -
> - end_io_cache(&log);
> - if (last_file>= 0)
> - mysql_file_close(last_file, MYF(MY_WME));
> - last_file = file;
> - }
> -
> -found_log:
> - my_b_seek(&log, last_pos);
> - if (find_target_pos(mi,&log,errmsg))
> - goto err;
> - fn_format(mi->log_file_name, last_log_name, "","",1); /* Copy basename */
> -
> -mi_inited:
> - error = 0;
> -err:
> - mysql_mutex_unlock(log_lock);
> - end_io_cache(&log);
> - mysql_mutex_lock(&LOCK_thread_count);
> - thd->current_linfo = 0;
> - mysql_mutex_unlock(&LOCK_thread_count);
> - if (file>= 0)
> - mysql_file_close(file, MYF(MY_WME));
> - if (last_file>= 0&& last_file != file)
> - mysql_file_close(last_file, MYF(MY_WME));
> -
> - DBUG_RETURN(error);
> -}
> -
> -
> -/**
> - Caller must delete result when done.
> -*/
> -
> -static Slave_log_event* find_slave_event(IO_CACHE* log,
> - const char* log_file_name,
> - char* errmsg)
> -{
> - Log_event* ev;
> - int i;
> - bool slave_event_found = 0;
> - LINT_INIT(ev);
> -
> - for (i = 0; i< 2; i++)
> - {
> - if (!(ev= Log_event::read_log_event(log, (mysql_mutex_t*)0, 0)))
> - {
> - my_snprintf(errmsg, SLAVE_ERRMSG_SIZE,
> - "Error reading event in log '%s'",
> - (char*)log_file_name);
> - return 0;
> - }
> - if (ev->get_type_code() == SLAVE_EVENT)
> - {
> - slave_event_found = 1;
> - break;
> - }
> - delete ev;
> - }
> - if (!slave_event_found)
> - {
> - my_snprintf(errmsg, SLAVE_ERRMSG_SIZE,
> - "Could not find slave event in log '%s'",
> - (char*)log_file_name);
> - return 0;
> - }
> -
> - return (Slave_log_event*)ev;
> -}
> -
> -/**
> - This function is broken now.
> -
> - @seealso translate_master()
> -*/
> -
> -bool show_new_master(THD* thd)
> -{
> - Protocol *protocol= thd->protocol;
> - DBUG_ENTER("show_new_master");
> - List<Item> field_list;
> - char errmsg[SLAVE_ERRMSG_SIZE];
> - LEX_MASTER_INFO* lex_mi=&thd->lex->mi;
> -
> - errmsg[0]=0; // Safety
> - if (translate_master(thd, lex_mi, errmsg))
> - {
> - if (errmsg[0])
> - my_error(ER_ERROR_WHEN_EXECUTING_COMMAND, MYF(0),
> - "SHOW NEW MASTER", errmsg);
> - DBUG_RETURN(TRUE);
> - }
> - else
> - {
> - field_list.push_back(new Item_empty_string("Log_name", 20));
> - field_list.push_back(new Item_return_int("Log_pos", 10,
> - MYSQL_TYPE_LONGLONG));
> - if (protocol->send_result_set_metadata(&field_list,
> - Protocol::SEND_NUM_ROWS | Protocol::SEND_EOF))
> - DBUG_RETURN(TRUE);
> - protocol->prepare_for_resend();
> - protocol->store(lex_mi->log_file_name,&my_charset_bin);
> - protocol->store((ulonglong) lex_mi->pos);
> - if (protocol->write())
> - DBUG_RETURN(TRUE);
> - my_eof(thd);
> - DBUG_RETURN(FALSE);
> - }
> -}
> -
> -/**
> - Asks the master for the list of its other connected slaves.
> -
> - This is for failsafe replication:
> - in order for failsafe replication to work, the servers involved in
> - replication must know of each other. We accomplish this by having each
> - slave report to the master how to reach it, and on connection, each
> - slave receives information about where the other slaves are.
> -
> - @param mysql pre-existing connection to the master
> - @param mi master info
> -
> - @note
> - mi is used only to give detailed error messages which include the
> - hostname/port of the master, the username used by the slave to connect to
> - the master.
> - If the user used by the slave to connect to the master does not have the
> - REPLICATION SLAVE privilege, it will pop in this function because
> - SHOW SLAVE HOSTS will fail on the master.
> -
> - @retval
> - 1 error
> - @retval
> - 0 success
> -*/
> -
> -int update_slave_list(MYSQL* mysql, Master_info* mi)
> -{
> - MYSQL_RES* res=0;
> - MYSQL_ROW row;
> - const char* error=0;
> - bool have_auth_info;
> - int port_ind;
> - DBUG_ENTER("update_slave_list");
> -
> - if (mysql_real_query(mysql, STRING_WITH_LEN("SHOW SLAVE HOSTS")) ||
> - !(res = mysql_store_result(mysql)))
> - {
> - error= mysql_error(mysql);
> - goto err;
> - }
> -
> - switch (mysql_num_fields(res)) {
> - case 5:
> - have_auth_info = 0;
> - port_ind=2;
> - break;
> - case 7:
> - have_auth_info = 1;
> - port_ind=4;
> - break;
> - default:
> - error= "the master returned an invalid number of fields for SHOW SLAVE \
> -HOSTS";
> - goto err;
> - }
> -
> - mysql_mutex_lock(&LOCK_slave_list);
> -
> - while ((row= mysql_fetch_row(res)))
> - {
> - uint32 log_server_id;
> - SLAVE_INFO* si, *old_si;
> - log_server_id = atoi(row[0]);
> - if ((old_si= (SLAVE_INFO*)my_hash_search(&slave_list,
> - (uchar*)&log_server_id,4)))
> - si = old_si;
> - else
> - {
> - if (!(si = (SLAVE_INFO*)my_malloc(sizeof(SLAVE_INFO), MYF(MY_WME))))
> - {
> - error= "the slave is out of memory";
> - mysql_mutex_unlock(&LOCK_slave_list);
> - goto err;
> - }
> - si->server_id = log_server_id;
> - if (my_hash_insert(&slave_list, (uchar*)si))
> - {
> - error= "the slave is out of memory";
> - mysql_mutex_unlock(&LOCK_slave_list);
> - goto err;
> - }
> - }
> - strmake(si->host, row[1], sizeof(si->host)-1);
> - si->port = atoi(row[port_ind]);
> - si->rpl_recovery_rank = atoi(row[port_ind+1]);
> - si->master_id = atoi(row[port_ind+2]);
> - if (have_auth_info)
> - {
> - strmake(si->user, row[2], sizeof(si->user)-1);
> - strmake(si->password, row[3], sizeof(si->password)-1);
> - }
> - }
> - mysql_mutex_unlock(&LOCK_slave_list);
> -
> -err:
> - if (res)
> - mysql_free_result(res);
> - if (error)
> - {
> - sql_print_error("While trying to obtain the list of slaves from the master "
> - "'%s:%d', user '%s' got the following error: '%s'",
> - mi->host, mi->port, mi->user, error);
> - DBUG_RETURN(1);
> - }
> - DBUG_RETURN(0);
> -}
> -
>
> /**
> Execute a SHOW SLAVE HOSTS statement.
>
> === modified file 'sql/repl_failsafe.h'
> --- a/sql/repl_failsafe.h 2010-10-11 14:50:14 +0000
> +++ b/sql/repl_failsafe.h 2010-11-29 11:04:16 +0000
> @@ -36,7 +36,6 @@ extern const char* rpl_role_type[], *rpl
> pthread_handler_t handle_failsafe_rpl(void *arg);
> void change_rpl_status(ulong from_status, ulong to_status);
> int find_recovery_captain(THD* thd, MYSQL* mysql);
> -int update_slave_list(MYSQL* mysql, Master_info* mi);
>
> extern HASH slave_list;
>
>
> === modified file 'sql/sp_head.cc'
> --- a/sql/sp_head.cc 2010-11-18 15:01:58 +0000
> +++ b/sql/sp_head.cc 2010-11-29 11:04:16 +0000
> @@ -237,7 +237,6 @@ sp_get_flags_for_command(LEX *lex)
> case SQLCOM_SHOW_EVENTS:
> case SQLCOM_SHOW_KEYS:
> case SQLCOM_SHOW_MASTER_STAT:
> - case SQLCOM_SHOW_NEW_MASTER:
> case SQLCOM_SHOW_OPEN_TABLES:
> case SQLCOM_SHOW_PRIVILEGES:
> case SQLCOM_SHOW_PROCESSLIST:
>
> === modified file 'sql/sql_lex.h'
> --- a/sql/sql_lex.h 2010-11-11 17:11:05 +0000
> +++ b/sql/sql_lex.h 2010-11-29 11:04:16 +0000
> @@ -162,7 +162,7 @@ enum enum_sql_command {
> SQLCOM_SHOW_OPEN_TABLES,
> SQLCOM_HA_OPEN, SQLCOM_HA_CLOSE, SQLCOM_HA_READ,
> SQLCOM_SHOW_SLAVE_HOSTS, SQLCOM_DELETE_MULTI, SQLCOM_UPDATE_MULTI,
> - SQLCOM_SHOW_BINLOG_EVENTS, SQLCOM_SHOW_NEW_MASTER, SQLCOM_DO,
> + SQLCOM_SHOW_BINLOG_EVENTS, SQLCOM_DO,
> SQLCOM_SHOW_WARNS, SQLCOM_EMPTY_QUERY, SQLCOM_SHOW_ERRORS,
> SQLCOM_SHOW_STORAGE_ENGINES, SQLCOM_SHOW_PRIVILEGES,
> SQLCOM_HELP, SQLCOM_CREATE_USER, SQLCOM_DROP_USER, SQLCOM_RENAME_USER,
>
> === modified file 'sql/sql_parse.cc'
> --- a/sql/sql_parse.cc 2010-11-18 15:01:58 +0000
> +++ b/sql/sql_parse.cc 2010-11-29 11:04:16 +0000
> @@ -316,7 +316,6 @@ void init_update_queries(void)
> sql_command_flags[SQLCOM_SHOW_VARIABLES]= CF_STATUS_COMMAND |
> CF_REEXECUTION_FRAGILE;
> sql_command_flags[SQLCOM_SHOW_CHARSETS]= CF_STATUS_COMMAND |
> CF_REEXECUTION_FRAGILE;
> sql_command_flags[SQLCOM_SHOW_COLLATIONS]= CF_STATUS_COMMAND |
> CF_REEXECUTION_FRAGILE;
> - sql_command_flags[SQLCOM_SHOW_NEW_MASTER]= CF_STATUS_COMMAND;
> sql_command_flags[SQLCOM_SHOW_BINLOGS]= CF_STATUS_COMMAND;
> sql_command_flags[SQLCOM_SHOW_SLAVE_HOSTS]= CF_STATUS_COMMAND;
> sql_command_flags[SQLCOM_SHOW_BINLOG_EVENTS]= CF_STATUS_COMMAND;
> @@ -2138,19 +2137,6 @@ case SQLCOM_PREPARE:
> #endif
> break;
> }
> - case SQLCOM_SHOW_NEW_MASTER:
> - {
> - if (check_global_access(thd, REPL_SLAVE_ACL))
> - goto error;
> - /* This query don't work now. See comment in repl_failsafe.cc */
> -#ifndef WORKING_NEW_MASTER
> - my_error(ER_NOT_SUPPORTED_YET, MYF(0), "SHOW NEW MASTER");
> - goto error;
> -#else
> - res = show_new_master(thd);
> - break;
> -#endif
> - }
>
> #ifdef HAVE_REPLICATION
> case SQLCOM_SHOW_SLAVE_HOSTS:
>
> === modified file 'sql/sql_repl.cc'
> --- a/sql/sql_repl.cc 2010-09-06 17:18:44 +0000
> +++ b/sql/sql_repl.cc 2010-11-29 11:04:16 +0000
> @@ -1664,23 +1664,6 @@ int reset_master(THD* thd)
> return 0;
> }
>
> -int cmp_master_pos(const char* log_file_name1, ulonglong log_pos1,
> - const char* log_file_name2, ulonglong log_pos2)
> -{
> - int res;
> - size_t log_file_name1_len= strlen(log_file_name1);
> - size_t log_file_name2_len= strlen(log_file_name2);
> -
> - // We assume that both log names match up to '.'
> - if (log_file_name1_len == log_file_name2_len)
> - {
> - if ((res= strcmp(log_file_name1, log_file_name2)))
> - return res;
> - return (log_pos1< log_pos2) ? -1 : (log_pos1 == log_pos2) ? 0 : 1;
> - }
> - return ((log_file_name1_len< log_file_name2_len) ? -1 : 1);
> -}
> -
>
> /**
> Execute a SHOW BINLOG EVENTS statement.
>
> === modified file 'sql/sql_repl.h'
> --- a/sql/sql_repl.h 2010-03-31 14:05:33 +0000
> +++ b/sql/sql_repl.h 2010-11-29 11:04:16 +0000
> @@ -43,8 +43,6 @@ int start_slave(THD* thd, Master_info* m
> int stop_slave(THD* thd, Master_info* mi, bool net_report);
> bool change_master(THD* thd, Master_info* mi);
> bool mysql_show_binlog_events(THD* thd);
> -int cmp_master_pos(const char* log_file_name1, ulonglong log_pos1,
> - const char* log_file_name2, ulonglong log_pos2);
> int reset_slave(THD *thd, Master_info* mi);
> int reset_master(THD* thd);
> bool purge_master_logs(THD* thd, const char* to_log);
>
> === modified file 'sql/sql_yacc.yy'
> --- a/sql/sql_yacc.yy 2010-11-11 17:11:05 +0000
> +++ b/sql/sql_yacc.yy 2010-11-29 11:04:16 +0000
> @@ -10921,19 +10921,6 @@ show_param:
> if (prepare_schema_table(YYTHD, lex, $4, SCH_COLUMNS))
> MYSQL_YYABORT;
> }
> - | NEW_SYM MASTER_SYM FOR_SYM SLAVE
> - WITH MASTER_LOG_FILE_SYM EQ
> - TEXT_STRING_sys /* $8 */
> - AND_SYM MASTER_LOG_POS_SYM EQ
> - ulonglong_num /* $12 */
> - AND_SYM MASTER_SERVER_ID_SYM EQ
> - ulong_num /* $16 */
> - {
> - Lex->sql_command = SQLCOM_SHOW_NEW_MASTER;
> - Lex->mi.log_file_name = $8.str;
> - Lex->mi.pos = $12;
> - Lex->mi.server_id = $16;
> - }
> | master_or_binary LOGS_SYM
> {
> Lex->sql_command = SQLCOM_SHOW_BINLOGS;
>
>
>
>
>
