Hi Jasonh,
Thank you.
See some comments in-line.
He Zhenxing wrote:
> > 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
> >
>
ok.
> >
>
>> >> ---
> 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
> >
>
Why not? If you don't have a strong opinion on that I would like to
rename it.
> >
>
>> >> --- 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
> >
>
ok.
> >
>
>> >> --- 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?
> >
>
Sorry that sneaked into the test case.
An the transactions were used to make sure that I could safely reset the
state by calling:
lex_start(thd);
mysql_reset_thd_for_next_command(thd);
> >
>
>> >>
> ##########################################################################
>> >> # 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.
> >
>
I agree. I will remove it.
> >
> >
>
>> >> 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
> >
>
ok... I was using the same pattern already there but I agree with you.
> >
>
>> >> 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.
> >
>
It makes sense to be together with the pattern rli->slave_patternload_file*
Don't you think? Besides the init_relay_log_info is only called to
startup/configure the slave.
> >
>
>> >> /*
>> >> 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
>> >>
>>
> >
> >
>
Cheers.
