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. 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.
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.
> > >
> >
> >> >> /*
> >> >> 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.
