Luis, hello.
The patch is correct.
I have few minors comments.
> #At file:///home/lsoares/Workspace/mysql-server/bugfix/39701/5.1-bt/ based on
> revid:alexey.kopytov@stripped
>
> 2801 Luis Soares 2009-02-17
> BUG#39701: Mixed binlog format does not switch to row mode on LOAD_FILE
>
> MIXED mode binlog_format should switch to ROW mode whenever there is a risk of
> breaking
> replication. However, LOAD_FILE, which depends on a file on the same host, does
> not trigger
> a switch to ROW based mode. This leads to scenarios on which the slave
> replicates the
> statement with 'load_file' and it will try to load the file from local file
> system. Given that
> the file is mostly likely inexistent in slave filesystem the operation will not
> succeed
> (probably returning NULL), causing master and slave(s) to diverge.
>
ok
> This patch addresses this bug by marking the load_file function as unsafe. When
> in mixed mode
> logging, this will make the procedure that decides on which logging format to
> use to be aware of
> the load_file function and decide accordingly.
> added:
> mysql-test/suite/rpl/r/rpl_mix_load_file.result
> mysql-test/suite/rpl/t/rpl_mix_load_file.test
> modified:
> sql/item_create.cc
>
> per-file messages:
> mysql-test/suite/rpl/t/rpl_mix_load_file.test
> Simple test to assert that the patch fixes this bug.
> sql/item_create.cc
> Added the set_stmt_unsafe call to lex.
> === added file 'mysql-test/suite/rpl/r/rpl_mix_load_file.result'
> --- a/mysql-test/suite/rpl/r/rpl_mix_load_file.result 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_mix_load_file.result 2009-02-17 00:49:33 +0000
> @@ -0,0 +1,29 @@
> +stop slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +reset master;
> +reset slave;
> +drop table if exists t1,t2,t3,t4,t5,t6,t7,t8,t9;
> +start slave;
You need to --replace_regex out the value of OUTFILE path generated locally on
your computer, in the following and other places.
> +SELECT repeat('x',20) INTO OUTFILE
> '/home/lsoares/Workspace/mysql-server/bugfix/39701/5.1-bt/mysql-test/var/tmp/bug_39701.data';
> +DROP TABLE IF EXISTS t1;
> +Warnings:
> +Note 1051 Unknown table 't1'
> +CREATE TABLE t1 (t text);
> +INSERT INTO t1 VALUES
> (load_file('/home/lsoares/Workspace/mysql-server/bugfix/39701/5.1-bt/mysql-test/var/tmp/bug_39701.data'));;
> +INSERT INTO t1 VALUES ('1');
> +SET BINLOG_FORMAT=STATEMENT;
> +INSERT INTO t1 VALUES (LOAD_FILE('$MYSQL_TMP_DIR/inexistent'));
> +Warnings:
> +Warning 1592 Statement is not safe to log in statement format.
> +show binlog events from <binlog_start>;
> +Log_name Pos Event_type Server_id End_log_pos Info
> +master-bin.000001 # Query # # use `test`; DROP TABLE IF EXISTS t1
> +master-bin.000001 # Query # # use `test`; CREATE TABLE t1 (t text)
> +master-bin.000001 # Query # # use `test`; BEGIN
> +master-bin.000001 # Table_map # # table_id: # (test.t1)
> +master-bin.000001 # Write_rows # # table_id: # flags: STMT_END_F
> +master-bin.000001 # Query # # use `test`; COMMIT
> +master-bin.000001 # Query # # use `test`; INSERT INTO t1 VALUES ('1')
> +master-bin.000001 # Query # # use `test`; INSERT INTO t1 VALUES
> (LOAD_FILE('$MYSQL_TMP_DIR/inexistent'))
> +Comparing tables master:test.t1 and slave:test.t1
> +DROP TABLE t1;
>
How about to incorporate your new test with the existing
rpl_loadfile.test?
The less files the less chances to make mtr2 restarting the servers
etc.
> === added file 'mysql-test/suite/rpl/t/rpl_mix_load_file.test'
> --- a/mysql-test/suite/rpl/t/rpl_mix_load_file.test 1970-01-01 00:00:00 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_mix_load_file.test 2009-02-17 00:49:33 +0000
> @@ -0,0 +1,62 @@
> +# BUG#39701: Mixed binlog format does not switch to row mode on LOAD_FILE
> +#
> +# DESCRIPTION
> +#
> +# Problem: when using load_file string function and mixed binlogging format
> +# there was no switch to row based binlogging format. This leads
> +# to scenarios on which the slave replicates the statement and it
> +# will try to load the file from local file system, which in most
> +# likely it will not exist.
> +#
> +# Solution:
> +# Marking this function as unsafe for statement format, makes the
> +# statement using it to be logged in row based format. As such, data
> +# replicated from the master, becomes the content of the loaded file.
> +# Consequently, the slave receives the necessary data to complete
> +# the load_file instruction correctly.
> +#
> +# IMPLEMENTATION
> +#
> +# The test is implemented as follows:
> +#
> +# On Master,
> +# i) write to file the desired content.
> +# ii) create table, and load into it the file contents
> +# iii) execute load_file and safe statement (wrt to mixed mode binlogging)
> +# iv) show binlogevents to assert that tere are rows for load_file and
> +# statement for safe statement from iii)
> +# v) force format to statement and check that the statement is still
> +# logged, but this time with a warning.
> +# vi) remove the file used in load_file
> +#
> +# On Slave,
> +# vii) check that it has replicated the correct values from master (should
> +# load_file not switch to row mode, then slave differs from master,
> +# because the file was removed in vi) and the load_file returns NULL
> +# when file does not exist)
> +#
> +
> +--source include/master-slave.inc
> +--source include/have_binlog_format_mixed.inc
> +
> +let $file= $MYSQL_TMP_DIR/bug_39701.data;
> +--eval SELECT repeat('x',20) INTO OUTFILE '$file'
> +
> +DROP TABLE IF EXISTS t1;
> +CREATE TABLE t1 (t text);
> +--eval INSERT INTO t1 VALUES (load_file('$file'));
> +INSERT INTO t1 VALUES ('1');
> +
> +SET BINLOG_FORMAT=STATEMENT;
> +INSERT INTO t1 VALUES (LOAD_FILE('$MYSQL_TMP_DIR/inexistent'));
> +--source include/show_binlog_events.inc
> +
> +remove_file $file;
> +sync_slave_with_master;
> +
> +let $diff_table_1=master:test.t1;
> +let $diff_table_2=slave:test.t1;
> +--source include/diff_tables.inc
> +
> +DROP TABLE t1;
> +sync_slave_with_master;
>
For the test itself I suggest to have a query that would call a stored
function calling load_file() itself.
Such nesting ensures the top level query is marked as
binlog-statement-format unsafe regardless of where load_file() is
called at the query processing.
> === modified file 'sql/item_create.cc'
> --- a/sql/item_create.cc 2008-04-11 03:27:24 +0000
> +++ b/sql/item_create.cc 2009-02-17 00:49:33 +0000
> @@ -3791,6 +3791,7 @@ Create_func_load_file Create_func_load_f
> Item*
> Create_func_load_file::create(THD *thd, Item *arg1)
> {
> + thd->lex->set_stmt_unsafe();
> thd->lex->uncacheable(UNCACHEABLE_SIDEEFFECT);
> return new (thd->mem_root) Item_load_file(arg1);
> }
ok
>
>
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe: http://lists.mysql.com/commits?unsub=1
>
cheers,
Andrei
