List:Commits« Previous MessageNext Message »
From:Guilhem Bichot Date:November 6 2008 3:38pm
Subject:Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2672)
Bug#38133
View as plain text  
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.
Thread
bzr commit into mysql-6.0-backup branch (ingo.struewing:2672) Bug#38133Ingo Struewing23 Jul
  • RE: bzr commit into mysql-6.0-backup branch (ingo.struewing:2672) Bug#38133Chuck Bell29 Jul
  • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2672)Bug#38133Guilhem Bichot6 Nov
    • RE: bzr commit into mysql-6.0-backup branch (ingo.struewing:2672)Bug#38133Vladislav Vaintroub6 Nov
    • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2672)Bug#38133Ingo Strüwing6 Nov
      • RE: bzr commit into mysql-6.0-backup branch (ingo.struewing:2672)Bug#38133Vladislav Vaintroub6 Nov
        • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2672)Bug#38133Ingo Strüwing6 Nov
      • Re: bzr commit into mysql-6.0-backup branch (ingo.struewing:2672)Bug#38133Guilhem Bichot6 Nov
        • New patch available [Re: bzr commit into mysql-6.0-backup branch(ingo.struewing:2672) Bug#38133]Ingo Strüwing13 Nov