Hi Andrei!
Two things to fix, see below!
I'm not 100% sure about the mtr things, but if Magnus accepts it, I
trust it.
/Sven
Andrei Elkin wrote:
> Below is the list of changes that have just been committed into a local
> 6.0 repository of aelkin. When aelkin does a push these changes
> will be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2008-05-05 17:17:19+03:00, aelkin@stripped +3 -0
> Bug #35546 mysqlbinlog.cc: Load_log_processor::process_first_event() fails creating
> unique
> Bug #35543 mysqlbinlog.cc does not properly work with tmp files
>
> There were failures on pb executions in that mysqlbinlog could not compose a new
> unique file name of its purpose.
> The reason of the bug has been that mysqlbinlog used the system wide temprorary
> directory instead
> of the specified by mtr so that mysqlbinlog's temporary files happened to be out of
> reach of mtr.
>
> Logics of the mysqlbinlog program correctly allows leaving new produced files such
> as ones that
> hold LOAD DATA data.
> This is fine as long as a caller of mysqlbinlog takes care of the new files later.
>
> The problem fixed with specifying explicitly the temp directory for mysqlbinlog
> when it's called
> from mtr. New files mysqlbinlog can produce now reside in the mtr temporary
> directory.
>
> Also there were memory leaks reported (bug#35543) which are fixed with finding out
> the sources and deploying
> freeing.
>
> client/mysqlbinlog.cc@stripped, 2008-05-05 17:17:16+03:00,
> aelkin@stripped +10 -1
> Bug #35543 memory leak issue.
> Fixed with freeing at error branches of
> Load_log_processor::process_first_event()
>
> mysql-test/mysql-test-run.pl@stripped, 2008-05-05 17:17:16+03:00,
> aelkin@stripped +13 -1
> passing --local-load= $opt_tmpdir to mysqlbinlog.
>
> mysql-test/suite/binlog/t/binlog_killed_simulate.test@stripped, 2008-05-05
> 17:17:16+03:00, aelkin@stripped +1 -1
> changing to portable macro.
>
> diff -Nrup a/client/mysqlbinlog.cc b/client/mysqlbinlog.cc
> --- a/client/mysqlbinlog.cc 2008-03-05 12:19:29 +02:00
> +++ b/client/mysqlbinlog.cc 2008-05-05 17:17:16 +03:00
> @@ -438,6 +438,7 @@ Exit_status Load_log_processor::process_
> {
> error("Could not construct local filename %s%s.",
> target_dir_name,bname);
> + my_free(fname, MYF(0));
> delete ce;
> DBUG_RETURN(ERROR_STOP);
> }
> @@ -445,9 +446,15 @@ Exit_status Load_log_processor::process_
> rec.fname= fname;
> rec.event= ce;
>
> + /*
> + rec contains dyn-allocated fname. It's stored in file_names so that
> + fname won't be lost but be free-d at case EXECUTE_LOAD_QUERY_EVENT
> + of `process_event' the grand-caller.
> + */
I would suggest a small reformulation of this comment. Basically, the
comment says that 'case EXECUTE_LOAD_QUERY_EVENT' is *not* responsible
for freeing the memory, but it is not completely clear who *is*
responsible for it. It is also a bit confusing to refer to the
grand-caller (to understand it, you must trace the calls two steps up).
Maybe you could write something like:
/*
Store the filename so that the next Execute_load_query_log_event
can use it. The responsibility for freeing fname is hereby
transferred to load_processor.
*/
> if (set_dynamic(&file_names, (uchar*)&rec, file_id))
> {
> error("Out of memory.");
> + my_free(fname, MYF(0));
> delete ce;
> DBUG_RETURN(ERROR_STOP);
> }
> @@ -465,6 +472,8 @@ Exit_status Load_log_processor::process_
> error("Failed closing file.");
> retval= ERROR_STOP;
> }
> + if (retval != OK_CONTINUE)
> + my_free(fname, MYF(0));
fname should not be freed here. The responsibility for freeing has been
transferred to the load log processor. The load log processor will be
destroyed at program termination, whether the program fails or not. So
if you free here, the pointer will be freed twice.
> DBUG_RETURN(retval);
> }
>
> @@ -823,7 +832,7 @@ Exit_status process_event(PRINT_EVENT_IN
> print_event_info->common_header_len=
> glob_description_event->common_header_len;
> ev->print(result_file, print_event_info);
> - ev->temp_buf= 0; // as the event ref is zeroed
> + ev->free_temp_buf(); // as the event ref is zeroed
> /*
> We don't want this event to be deleted now, so let's hide it (I
> (Guilhem) should later see if this triggers a non-serious Valgrind
> diff -Nrup a/mysql-test/mysql-test-run.pl b/mysql-test/mysql-test-run.pl
> --- a/mysql-test/mysql-test-run.pl 2008-04-28 11:19:40 +03:00
> +++ b/mysql-test/mysql-test-run.pl 2008-05-05 17:17:16 +03:00
> @@ -987,6 +987,18 @@ sub client_arguments ($) {
> }
>
>
> +sub mysqlbinlog_arguments () {
> + my $exe= mtr_exe_exists("$path_client_bindir/mysqlbinlog");
> +
> + my $args;
> + mtr_init_args(\$args);
> + mtr_add_arg($args, "--defaults-file=%s", $path_config_file);
> + mtr_add_arg($args, "--local-load=%s", $opt_tmpdir);
> + client_debug_arg($args, "mysqlbinlog");
> + return mtr_args2str($exe, @$args);
> +}
> +
> +
> sub mysqlslap_arguments () {
> my $exe= mtr_exe_maybe_exists("$path_client_bindir/mysqlslap");
> if ( $exe eq "" ) {
> @@ -1218,7 +1230,7 @@ sub environment_setup {
> $ENV{'MYSQL_SLAP'}= mysqlslap_arguments();
> $ENV{'MYSQL_IMPORT'}= client_arguments("mysqlimport");
> $ENV{'MYSQL_SHOW'}= client_arguments("mysqlshow");
> - $ENV{'MYSQL_BINLOG'}= client_arguments("mysqlbinlog");
> + $ENV{'MYSQL_BINLOG'}= mysqlbinlog_arguments();
> $ENV{'MYSQL'}= client_arguments("mysql");
> $ENV{'MYSQL_UPGRADE'}= client_arguments("mysql_upgrade");
> $ENV{'MYSQLADMIN'}= native_path($exe_mysqladmin);
> diff -Nrup a/mysql-test/suite/binlog/t/binlog_killed_simulate.test
> b/mysql-test/suite/binlog/t/binlog_killed_simulate.test
> --- a/mysql-test/suite/binlog/t/binlog_killed_simulate.test 2008-04-08 09:34:29
> +03:00
> +++ b/mysql-test/suite/binlog/t/binlog_killed_simulate.test 2008-05-05 17:17:16
> +03:00
> @@ -63,7 +63,7 @@ let $error_code= `select @a like "%#%err
> eval select $error_code /* must return 0 to mean the killed query is in */;
>
> # cleanup for the sub-case
> -system rm $MYSQLTEST_VARDIR/tmp/binlog_killed_bug27571.binlog;
> +remove_file $MYSQLTEST_VARDIR/tmp/binlog_killed_bug27571.binlog;
>
>
> drop table t1,t2;
>
--
Sven Sandberg, Software Engineer
MySQL AB, www.mysql.com
