Re: bk commit into 6.0 tree (aelkin:1.2646) BUG#35546

[Davi Arnaut]

Andrei Elkin wrote:

[..]

>>>>>   
>>>>>   Logics of the mysqlbinlog program correctly allows leaving new
> produced files such as ones that 
>>>>>   hold LOAD DATA data.
>>>> This is just working around the issue. What happens in a non-mtr
>>>> environment? That tmp file creation function is unacceptable.
>>>>
>>> Let's state the issue precisely. Imo the issue is about removing
>>> temprory files of mtr, not mysqlbinlog - bug#35546. mysqlbinlog is a
>>> producer of some file(s) that are the main sql program and possibly
>>> data files that the program can refer to e.g because of LOAD DATA. So
>>> there are no temporary files as such that mysqlbinlog forgets to
>>> remove. Removal is a duty of a caller of mysqlbinlog.
>>>
>>> Let's agree on that first.
>> I partly agree. I agree that the caller should clean it if the operation
>> finished successfully, otherwise it's mysqlbinlog duty to clean up.
> 
> If mysqlbinlog finished with a failure, there would be a subset of
> files it was going to produce. 
> Whether the subset is whole or not is
> out of mysqlbinlog concern.  The user might be okay with partial
> results and possibly would like to process them further.
> The user/a binlog caller are gaining all the info with the current
> patch in order to determine where are the files mysqlbinlog has
> produced.
> However it has been their concern so far to process them further and
> possibly to remove.

IMHO, this is wrong. If every process were to leave it's temporary file
cleanup to the caller /tmp would be messier than it already is.
Temporary files should be cleaned up unless the program crashed.

This action of not cleaning temporary files is better suited for Windows
where there is a tool to remove temporary files.

I guess that ultimately the user will end up creating a "unique" temp
directory for each invocation of mysqlbinlog.

>>>>>   This is fine as long as a caller of mysqlbinlog takes care of the
> new files later.
>>>> OK, callers are responsible for cleaning up files left by mysqlbinlog.
>>>>
>>> And you seem to have agreed.
>>>
>>>> How does a caller know which files mysqlbinlog created?
>>> <paster from the patch>
>>>
>>>   mysql-test/mysql-test-run.pl@stripped, 2008-05-01
> 14:04:31+03:00,aelkin@stripped +33 -1
>>>     adding --local-load= $opt_tmpdir argument to mysqlbinlog.
>>> </paste>
>>>
>>> It can find out what has left in the mysqlbinlog's --local-load directory.
>>>
>>>> How the caller know which files are temporary?
>>> I understand you must have been confused with the synopsis
>>>  
>>>   Bug #34283 mysqlbinlog leaves tmpfile after termination
> 
>> I haven't looked at Bug#34283 but I might be misunderstanding something.
>> They way I see it is that mysqlbinlog creates a few temporary files (by
>> calling create_unique_file) but does not remove the temp file if the
>> operation fails later on. For example, the function
>> Load_log_processor::process_first_event:
>>
>>
>>   if ((file= create_unique_file(fname,ptr)) < 0)
>>   {
>>     error("Could not construct local filename %s%s.",
>>           target_dir_name,bname);
>>     delete ce;
>>     DBUG_RETURN(ERROR_STOP);
>>   }
>>
>>   rec.fname= fname;
>>   rec.event= ce;
>>
>>   if (set_dynamic(&file_names, (uchar*)&rec, file_id))
>>   {
>>     error("Out of memory.");
>>     delete ce;
>>     DBUG_RETURN(ERROR_STOP);
>>   }
>>
>> The problem of this and other error paths is that the temporary file is
>> left behind without proper cleanup.
>>
> 
> I think the current logic is pretty robust although not ideal.

Not robust, on the contrary.

> In cases you are pointing out the file perhaps could be safely
> removed. You are welcome to make it better!

Also, you are welcome to properly fix it :)

> But this is not something that affected cases of the bugs I am fixing.
> To remind - the matter is that not-failed mysqlbinlog saturated a
> directory that was not under the mtr's supervision.
> 
>>> There is no such thing as mysqlbinlog temprory file.
>> If there is no temporary file the function name should be changed to
>> something more meaningful.
> 
> Which function do you mean?

Everything with "temporary file" in that file. You say that there is no
such thing as "mysqlbinlog temprory file", yet this expression is used a
lot in the very same file.

> create_unique_file?
> 
> Does not the name says it precisely, that it's not about to create a
> temporary file?

/*
  ..

  Creates a temporary file to be used in LOAD DATA and writes first
  block of data to it. Registers its file name (and optional
  Create_file event) in the array of active temporary files.

  ..

*/

--

I think I've stated my point, now it's up to you and the reviewers. eof.

Regards,

-- 
Davi Arnaut, Software Engineer
MySQL Server Runtime Team
Database Group, Sun Microsystems