Hi Alfranio,
Patch is good, but I have some comments!
Alfranio Correia wrote:
> #At
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-42861/mysql-5.1-bugteam/
> based on revid:joro@stripped
>
> 2832 Alfranio Correia 2009-03-16
> BUG#42861 Assigning invalid directories to --slave-load-tmpdir crashes the
> slave
>
> Compiling with debug and assigning an invalid directory to --slave-load-tmpdir
> was crashing the slave due to the following assertion DBUG_ASSERT(! is_set()
> ||
> can_overwrite_status).
>
> This assertion assumes that a thread can changing its state once (i.e. ok,
> error, etc) before aborting, cleaning/resuming or completing its execution
> unless the overwrite flag (i.e. can_overwrite_status) is true.
>
> The cleanup function however was being failing and an error thrown by
> my_dir and the replication SQL thread was not aborting its execution.
> Furthermore, the Append_block_log_event::do_apply_event was not reseting
> internal states before starting processing.
>
> To fix the problem we check the directory before starting the replication
> and reset internal states before starting processing.
> added:
> mysql-test/suite/rpl/r/rpl_slave_fail_load_in_start.result
> mysql-test/suite/rpl/t/rpl_slave_fail_load_in_rm-slave.opt
> mysql-test/suite/rpl/t/rpl_slave_fail_load_in_rm.test
> mysql-test/suite/rpl/t/rpl_slave_fail_load_in_start-slave.opt
> mysql-test/suite/rpl/t/rpl_slave_fail_load_in_start.test
> renamed:
> mysql-test/suite/rpl/r/rpl_slave_load_in.result =>
> mysql-test/suite/rpl/r/rpl_slave_pass_load_in.result
> mysql-test/suite/rpl/t/rpl_slave_load_in.test =>
> mysql-test/suite/rpl/t/rpl_slave_pass_load_in.test
> modified:
> sql/log_event.cc
> sql/rpl_rli.cc
> mysql-test/suite/rpl/r/rpl_slave_pass_load_in.result
> mysql-test/suite/rpl/t/rpl_slave_pass_load_in.test
>
> per-file messages:
> sql/log_event.cc
> Three changes:
>
> 1 - Any error in the cleanup procedure is ignore. Eventually,
> any problem will be caught later.
>
> 2 - Resets internal states before creating a temporary file.
>
> 3 - Introduces fault injection to simulate that a problem happend with the
> temporary file.
> sql/rpl_rli.cc
> Checks if temporary directory used by commands like
> LOAD DATA INFILE is valid.
> === added file 'mysql-test/suite/rpl/r/rpl_slave_fail_load_in_start.result'
I'd suggest to name it: rpl_slave_load_tmpdir_not_exist
> --- a/mysql-test/suite/rpl/r/rpl_slave_fail_load_in_start.result 1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/rpl/r/rpl_slave_fail_load_in_start.result 2009-03-16 16:55:56
> +0000
> @@ -0,0 +1,2 @@
> +START SLAVE;
> +ERROR HY000: Can't read dir of '../../../error' (Errcode: 2)
>
> === renamed file 'mysql-test/suite/rpl/r/rpl_slave_load_in.result' =>
> 'mysql-test/suite/rpl/r/rpl_slave_pass_load_in.result'
Please do not rename this test case
> --- a/mysql-test/suite/rpl/r/rpl_slave_load_in.result 2009-02-21 09:36:07 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_slave_pass_load_in.result 2009-03-16 16:55:56 +0000
> @@ -5,6 +5,19 @@ reset slave;
> drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> start slave;
> create table t1(a int not null auto_increment, b int, primary key(a));
> +create table t2(a int not null auto_increment, b int, primary key(a))
> engine=innodb;
> +create table t3(a int not null auto_increment, b int, primary key(a))
> engine=innodb;
> load data infile '../../std_data/rpl_loaddata.dat' into table t1;
> +start transaction;
> +insert into t2(b) values (1);
> +insert into t2(b) values (2);
> +insert into t3(b) values (1);
> +insert into t3(b) values (2);
> +load data infile '../../std_data/rpl_loaddata.dat' into table t2;
> +load data infile '../../std_data/rpl_loaddata.dat' into table t2;
> +commit;
> Comparing tables master:test.t1 and slave:test.t1
> +Comparing tables master:test.t2 and slave:test.t2
> drop table t1;
> +drop table t2;
> +drop table t3;
>
> === added file 'mysql-test/suite/rpl/t/rpl_slave_fail_load_in_rm-slave.opt'
> --- a/mysql-test/suite/rpl/t/rpl_slave_fail_load_in_rm-slave.opt 1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/rpl/t/rpl_slave_fail_load_in_rm-slave.opt 2009-03-16 16:55:56
> +0000
> @@ -0,0 +1 @@
> +--loose-debug=d,LOAD_DATA_INFILE_has_no_file
>
> === added file 'mysql-test/suite/rpl/t/rpl_slave_fail_load_in_rm.test'
How about rpl_remove_slave_load_tmpdir.test
> --- a/mysql-test/suite/rpl/t/rpl_slave_fail_load_in_rm.test 1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/rpl/t/rpl_slave_fail_load_in_rm.test 2009-03-16 16:55:56
> +0000
> @@ -0,0 +1,46 @@
> +##########################################################################
> +# This test verifies if the slave fails gracefully when the temporary
> +# file used to load data is removed while it is about to be used it.
> +# Similar errors are caught if the temporary directory is removed.
> +#
> +# Steps:
> +# 1 - Creates a table and populates it through "LOAD DATA INFILE".
> +# 2 - Catches error.
> +##########################################################################
> +--source include/have_binlog_format_mixed_or_statement.inc
> +--source include/have_innodb.inc
> +--source include/have_debug.inc
> +--source include/master-slave.inc
> +
> +##########################################################################
> +# Loading data
> +##########################################################################
> +connection master;
> +
> +create table t1(a int not null auto_increment, b int, primary key(a))
> engine=innodb;
> +
> +start transaction;
> + insert into t1(b) values (1);
> + insert into t1(b) values (2);
> + load data infile '../../std_data/rpl_loaddata.dat' into table t1;
> +commit;
> +
> +##########################################################################
> +# Catch Error
> +##########################################################################
> +connection slave;
> +source include/wait_for_slave_sql_to_stop.inc;
> +
> +let $error=query_get_value("show slave status", Last_SQL_Error, 1);
> +echo $error;
> +
> +##########################################################################
> +# Clean up
> +##########################################################################
> +connection master;
> +
> +drop table t1;
> +
> +connection slave;
> +
> +drop table t1;
>
> === added file 'mysql-test/suite/rpl/t/rpl_slave_fail_load_in_start-slave.opt'
> --- a/mysql-test/suite/rpl/t/rpl_slave_fail_load_in_start-slave.opt 1970-01-01
> 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_slave_fail_load_in_start-slave.opt 2009-03-16
> 16:55:56 +0000
> @@ -0,0 +1 @@
> +--slave-load-tmpdir=../../../error
>
> === added file 'mysql-test/suite/rpl/t/rpl_slave_fail_load_in_start.test'
> --- a/mysql-test/suite/rpl/t/rpl_slave_fail_load_in_start.test 1970-01-01 00:00:00
> +0000
> +++ b/mysql-test/suite/rpl/t/rpl_slave_fail_load_in_start.test 2009-03-16 16:55:56
> +0000
> @@ -0,0 +1,13 @@
> +##########################################################################
> +# This test verifies if the start slave fails gracefuly when an
> +# invalid directory is used to set --slave-load-tmpdir.
> +##########################################################################
> +connect (master,127.0.0.1,root,,test,$MASTER_MYPORT,);
> +connect (master1,127.0.0.1,root,,test,$MASTER_MYPORT,);
> +connect (slave,127.0.0.1,root,,test,$SLAVE_MYPORT,);
> +connect (slave1,127.0.0.1,root,,test,$SLAVE_MYPORT,);
> +
> +connection slave;
> +
> +--error 12
> +START SLAVE;
>
> === renamed file 'mysql-test/suite/rpl/t/rpl_slave_load_in.test' =>
> 'mysql-test/suite/rpl/t/rpl_slave_pass_load_in.test'
> --- a/mysql-test/suite/rpl/t/rpl_slave_load_in.test 2009-02-21 09:36:07 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_slave_pass_load_in.test 2009-03-16 16:55:56 +0000
> @@ -3,10 +3,11 @@
> # event while the "--secure-file-priv" option is set.
> #
> # The test is divided in two steps:
> -# 1 - Creates a table and populates it through "LOAD DATA INFILE".
> +# 1 - Creates tables and populates them through "LOAD DATA INFILE".
> # 2 - Compares the master and slave.
> ##########################################################################
> -source include/master-slave.inc;
> +--source include/have_innodb.inc
> +--source include/master-slave.inc
>
> ##########################################################################
> # Loading data
> @@ -14,8 +15,20 @@ source include/master-slave.inc;
> connection master;
>
> create table t1(a int not null auto_increment, b int, primary key(a));
> +create table t2(a int not null auto_increment, b int, primary key(a))
> engine=innodb;
> +create table t3(a int not null auto_increment, b int, primary key(a))
> engine=innodb;
> +
> load data infile '../../std_data/rpl_loaddata.dat' into table t1;
>
> +start transaction;
> + insert into t2(b) values (1);
> + insert into t2(b) values (2);
> + insert into t3(b) values (1);
> + insert into t3(b) values (2);
> + load data infile '../../std_data/rpl_loaddata.dat' into table t2;
> + load data infile '../../std_data/rpl_loaddata.dat' into table t2;
> +commit;
> +
Could you explain the purpose of the using multiple tables and
transaction?
> ##########################################################################
> # Checking Consistency
> ##########################################################################
> @@ -25,11 +38,17 @@ let $diff_table_1=master:test.t1;
> let $diff_table_2=slave:test.t1;
> source include/diff_tables.inc;
>
> +let $diff_table_1=master:test.t2;
> +let $diff_table_2=slave:test.t2;
> +source include/diff_tables.inc;
> +
> ##########################################################################
> # Clean up
> ##########################################################################
> connection master;
>
> drop table t1;
> +drop table t2;
> +drop table t3;
>
> sync_slave_with_master;
>
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc 2009-03-05 10:23:46 +0000
> +++ b/sql/log_event.cc 2009-03-16 16:55:56 +0000
> @@ -382,7 +382,7 @@ static void cleanup_load_tmpdir()
> uint i;
> char fname[FN_REFLEN], prefbuf[31], *p;
>
> - if (!(dirp=my_dir(slave_load_tmpdir,MYF(MY_WME))))
> + if (!(dirp=my_dir(slave_load_tmpdir,MYF(0))))
OK
> return;
>
> /*
> @@ -6173,6 +6173,16 @@ int Append_block_log_event::do_apply_eve
> int error = 1;
> DBUG_ENTER("Append_block_log_event::do_apply_event");
>
> + /*
> + Usually lex_start() is called by mysql_parse(), but we need it here
> + as the present method does not call mysql_parse().
> + */
> + if (!thd->lock)
> + {
> + lex_start(thd);
> + mysql_reset_thd_for_next_command(thd);
> + }
> +
There can be more than one Append_block_log_event for one LOAD DATA
query, and only need to do this for the first such event, which means
when creating the load file. So I think you can put these to:
if (get_create_or_append())
{
...
{
And I think there is no need to check if (!thd->lock) here.
> fname= strmov(proc_info, "Making temp file ");
> slave_load_file_stem(fname, file_id, server_id, ".data");
> thd_proc_info(thd, proc_info);
> @@ -6197,6 +6207,10 @@ int Append_block_log_event::do_apply_eve
> get_type_str(), fname);
> goto err;
> }
> +
> + DBUG_EXECUTE_IF("LOAD_DATA_INFILE_has_no_file", my_close(fd,MYF(0)); fd= -1;
> + my_delete(fname, MYF(0)););
> +
Please use all lower letters for the keyword, and I'd suggest something
like: remove_slave_load_file_before_write
> if (my_write(fd, (uchar*) block, block_len, MYF(MY_WME+MY_NABP)))
> {
> rli->report(ERROR_LEVEL, my_errno,
>
> === modified file 'sql/rpl_rli.cc'
> --- a/sql/rpl_rli.cc 2009-02-22 13:40:52 +0000
> +++ b/sql/rpl_rli.cc 2009-03-16 16:55:56 +0000
> @@ -80,6 +80,40 @@ Relay_log_info::~Relay_log_info()
> DBUG_VOID_RETURN;
> }
>
> +/*
> + Check the temporary directory used by commands like
> + LOAD DATA INFILE.
> + */
> +int check_temp_dir(char* tmp_dir, char *tmp_file)
> +{
> + int fd;
> + MY_DIR *dirp;
> +
> + DBUG_ENTER("check_temp_dir");
> +
> + /*
> + Check if the directory exists.
> + */
> + if (!(dirp=my_dir(tmp_dir,MYF(MY_WME))))
> + DBUG_RETURN(1);
> + my_dirend(dirp);
> +
> + /*
> + Check permissions to create a file.
> + */
> + if ((fd= my_create(tmp_file, CREATE_MODE,
> + O_WRONLY | O_BINARY | O_EXCL | O_NOFOLLOW,
> + MYF(MY_WME))) < 0)
> + DBUG_RETURN(1);
> +
> + /*
> + Clean up.
> + */
> + my_close(fd, MYF(0));
> + my_delete(tmp_file, MYF(0));
> +
> + DBUG_RETURN(0);
> +}
>
> int init_relay_log_info(Relay_log_info* rli,
> const char* info_fname)
> @@ -109,6 +143,12 @@ int init_relay_log_info(Relay_log_info*
> MY_RETURN_REAL_PATH);
> rli->slave_patternload_file_size= strlen(rli->slave_patternload_file);
>
> + if (check_temp_dir(slave_load_tmpdir, rli->slave_patternload_file))
> + {
> + pthread_mutex_unlock(&rli->data_lock);
> + DBUG_RETURN(1);
> + }
> +
init_relay_log_info is called in many places, including CHANGE MASER, I
think it's more appropriate to move check_temp_dir to handle_slave_sql.
> /*
> The relay log will now be opened, as a SEQ_READ_APPEND IO_CACHE.
> Note that the I/O thread flushes it to disk after writing every
>
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe: http://lists.mysql.com/commits?unsub=1
