Sven, hello!
> 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.
>
Thanks for your good notes!
> /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:
Changed in a way we have discussed on #rep today.
>
> /*
> 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.
You are right. Calling load_log_processor::destroy somehow slipped
from my attention (perhaps appeared to so transparent that I managed
not to see at all:).
To double check us i run my tests with valgrind emulating errors in
different branches of Load_log_processor::process_first_event().
>
>> 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
The newer patch has been committed.
cheers,
Andrei
