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

[Andrei Elkin]

Davi,

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

I guess this discussion went to the dead end:
I am saying there is no temprory files, you keep to stress on that.

>>>>>>   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 :)

To fix what?

There is a possibility to create one less file if mysqlbinlog program
fails in Load_log_processor::process_first_event(). This part
does not belong to the bug issue bug#35546.

The first complaint 
- mysqlbinlog.cc leaves temporary files on the
  disk after shutdown.  That leads to the pollution of temporary directory.  
of bug#35543 is not precisely written.

It's not mysqlbinlog.cc but mtr that leaves the files.
There is no such thing as mysqlbinlog temporary file :)

And after the patch mtr removes the files.
If there is something else that you need, then you are welcome to file a
bug. It's not a secret there are lots of minor and not so minor things
to improve in mysqlbinlog. But distantly possible one-less file in caller's
temporary directory, that is supposed to be removed anyway, would be
certainly not the first item on my sorted list.

regards,

Andrei


>
>> 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