Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2672) Bug#38133

[Ingo Strüwing]

Hi Guilhem.

thank you very much for the first look into the patch. On your request,
I will port it onto a current tree for further review. Please find some
answers below.

Guilhem Bichot, 06.11.2008 14:20:
...
> Ingo Struewing a écrit :
...
>>  2672 Ingo Struewing    2008-07-23
>>       Bug#38133 - Myisamlog test fails on Windows
>>             Updating of a table from the myisam.log file was not
>> possible in most cases.
>>       - The initialization of myisamlog set the number of usable file
>> descriptors
>>       to one (1!) on Windows.
> 
> Ok
> 
>>       - Every log command to a different file required a close of the
>> previous file.
>>       After a 'close' command the closing of the previous file failed
>> due to a
>>       lack of closable files and myisamlog stopped.
> 
> Was this Windows-specific? If yes, why?

Yes, because of the above. Only one file descriptor was available. When
the 'close' command came to the low-level close(), the descriptor had
been closed already.

In theory this might happen on other systems too, when more files are
handled than file descriptors are available.

...
>>   storage/myisam/mi_examine_log.c
>>     Bug#38133 - Myisamlog test fails on Windows
>>     Added member lock_type to file_info.
> 
>>     Changed tracking of allocated memory in 'buff' and 'file_info' so
>> that
>>     it can be freed in case of errors.
> 
> because it leaked in case of errors?

Too long ago. Either it leaked during the tests, or the possibility was
obvious from code. If the answer is important, I can start to
re-investigate.

> 
>>     Added a check for valid data in the log file.
>>     Disabled re-opening for a close command.
> 
> Why? Efficiency?

For efficiency and for correctness in case of a lack of file descriptors.

> 
>>     Reduced closing of files to cases where file descriptors are used up.
> 
> What problem is this fixing?

Inefficiency.

> 
>>     Added code to re-lock a file after re-open.
> 
> What problem is this fixing?

After auto-re-open, files did not become locked again. So other
applications could tamper with the files while myisamlog was also
working with them.

...
>> === modified file 'mysys/my_lock.c'
>> --- a/mysys/my_lock.c    2008-06-26 17:48:42 +0000
>> +++ b/mysys/my_lock.c    2008-07-23 09:57:40 +0000
>> @@ -131,24 +131,43 @@ int my_lock(File fd, int locktype, my_of
...
> I'm looking at mysys/my_lock.c as it is in the current 6.0-main, and the
> portion above is quite different, so it's hard to see what the patch
> changes?
> Could you please make an updated patch?

Ok, please stay tuned.

> I wouldn't need to make such request if I had reviewed earlier, I know,
> I know...
> I have more comments below, and will have more when looking at the
> updated patch.
> 
>> === modified file 'storage/myisam/mi_close.c'
>> --- a/storage/myisam/mi_close.c    2008-07-09 07:12:43 +0000
>> +++ b/storage/myisam/mi_close.c    2008-07-23 09:57:40 +0000
>> @@ -30,6 +30,7 @@ int mi_close(register MI_INFO *info)
>>    DBUG_PRINT("enter",("base: %p  reopen: %u  locks: %u",
>>                info, (uint) share->reopen,
>>                        (uint) share->tot_locks));
>> +  DBUG_PRINT("myisam", ("close '%s'", share->unresolv_file_name));
> 
> why not just add share->unresolv_file_name into the already existing
> DBUG_PRINT("enter")?

I didn't know that we have a rule to reduce the number of DBUG_PRINTs.
In this case, I can remove it before pushing. Otherwise I'm always happy
when the lines in the trace file don't become too long.

...
>> +      DBUG_PRINT("myisamlog", ("found info: 0x%lx  file: '%s'  "
>> +                               "used: %d  closed: %d",
>> +                               (long) curr_file_info,
>> curr_file_info->name,
> 
> About 0x%lx, I see %p is spreading in the current MySQL code these last
> months; I believe it's because most compilers which we use support %p.

I can remember absolute prohibition by Monty. This has never been taken
back to my knowledge. There are a lot more conventions that degenerate
because new colleagues are not taught to adhere to them...

...
>> -    DBUG_PRINT("info",("command: %u curr_file_info: 0x%lx used: %u",
>> -                       command, (ulong)curr_file_info,
>> -                       curr_file_info ? curr_file_info->used : 0));
> 
> Why do we zap this?
> It was one single DBUG_PRINT which told about the command, instead of
> one DBUG_PRINT per command like added in this patch?

The command name was printed numerically, which required manual lookup.
But if you like it, I'll happily put it back.

...
>> === modified file 'storage/myisam/mi_locking.c'
>> --- a/storage/myisam/mi_locking.c    2008-07-09 07:12:43 +0000
>> +++ b/storage/myisam/mi_locking.c    2008-07-23 09:57:40 +0000
>> @@ -104,7 +104,11 @@ int mi_lock_database(MI_INFO *info, int
>>            mi_print_error(share, HA_ERR_CRASHED);
>>            mi_mark_crashed(info);
>>          }
>> -    if (info->lock_type != F_EXTRA_LCK)
>> +        /*
>> +          If we have a pseudo lock (F_EXTRA_LCK) or a temporary table,
>> +          skip file locking.
>> +        */
>> +        if ((info->lock_type != F_EXTRA_LCK) &&
> !share->temporary)
> 
> indentation looks strange but I know this sometimes happens in diffs.
> Why is this " && !share->temporary" needed?

I believe, I have seen cases where a temporary table did not take a
F_EXTRA_LCK lock. I want to prevent locking in this case too.

...
>> +    DBUG_PRINT("myisam", ("updating open_count: %u",
>> share->state.open_count));
> 
> I wonder - do we really want to introduce a new type of DBUG tag -
> "myisam" - when most of the rest of MyISAM uses "info", "enter", "exit".
> I realize I myself introduced a "myisam_backup" tag in two lines of
> myisam_backup_engine.cc, but now I have remorse and trouble sleeping at
> night.

What is the problem with more tags? I like to grep for them in a trace
file. Also it enables to make use of the DBUG feature to pre-select
certain output with --debug=d,myisam,table_cache.

...
>> +  DBUG_PRINT("myisamlog", ("error from mi_examine_log: %d", error));
> 
> Do we want this new type of tag?

Again, it allows me to select certain lines out of the mass. But when
you have explained why we should not have many tags, I will change it.

Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder, Wolfgang Engels, Dr. Roland Bömer
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028