Hello,
Ingo Strüwing a écrit, Le 11/06/2008 03:58 PM:
> 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.
Thank you, and sorry for the additional work caused.
> 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.
>>> - 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.
Ah, it's automatic closing (due to lack of descriptors) which collided
with explicit closing (MI_LOG_CLOSE execution).
>>> 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.
Don't bother now. When I apply the updated patch, it will probably
become clearer for me.
>>> === 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.
We don't.
> 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.
Fine with me, you can keep your change.
>>> + 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...
I don't think the prohibition has been taken back, but in practice, a
grep of "%p" in sql/ in 6.0 shows tens of occurences.
I believe it's because compilers evolved.
The advantage of %p is for 64-bit systems where "long uint" is 32-bit
(Windows): %lx cuts half of the pointer value.
> ...
>>> - 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.
Not that I liked it, but it gave a smaller debug trace.
To get the name with a single DBUG_PRINT you could use
mi_log_command_name[command].
>>> === 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.
I should look more into this with the updated patch. From what I had
understood, temporary tables should always use F_EXTRA_LCK.
>>> + 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.
There is no real problem. I was only wondering :)
> ...
>>> + 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.
No, I understand your logic.
