Hi,
Comments in-line.
He Zhenxing wrote:
> Alfranio Correia wrote:
>
>> 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.
>>
>>
>
> I think by 'pass' you mean that the test will not cause error, in
> contrast with the other two test cases. But since you agreed to rename
> the other two test cases, there is no reason to rename this one.
ok.
> And
> someone might add codes to test some load data statements that would
> fail with expected error, that would conflict with the name or have to
> use a new test case.
>
> Although bzr can handle renames, but I'm not sure if this is 100%
> guaranteed, and so I'm afraid this might cause merge problems in the
> future.
>
>
>>>>
>>>>
>>>
>>>
>>>>>> --- 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.
>>
>>
>
> No, I don't think it need to be together with the pattern, even if you
> want to put it in init_relay_log_info, it should be put in the end of
> the function, so that when this check fails, other part of the rli
> structure will be initialized properly, so that user will not run into
> trouble when the run SHOW SLAVE STATUS.
>
I don't really buy this.
> And I think it's better to only do the check when slave SQL thread
> starts. I think it would confuse the use if CHANGE MASTER fails because
> of this.
>
>
I completely agree with this. It would be strange to receive an error
about a temporary directory while running change master.
I will change it.
>>>>
>>>>
>>>
>>>
>>>>>> /*
>>>>>> 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.
>>
>
>
Cheers.
