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

[Guilhem Bichot]

Hello,

Ingo Struewing a écrit :
> #At file:///home2/mydev/bzrroot/mysql-6.0-bug38133/
> 
>  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?

>       - File locking on Windows failed for the sequence "exclusive lock, shared
> lock,
>       unlock, exclusive lock". It blocked on the last try to acquire an exclusive
> lock.
>       
>       Fixed by
>       - requesting a reasonable number of file descriptors,
>       - before re-open, close a file only if all file descriptors are in use,
>       - do not re-open a file, if the command is 'close',
>       - unlock a file before every lock.
> modified:
>   mysql-test/r/myisamlog.result
>   mysql-test/t/myisamlog.test
>   mysys/my_file.c
>   mysys/my_lock.c
>   storage/myisam/mi_close.c
>   storage/myisam/mi_examine_log.c
>   storage/myisam/mi_locking.c
>   storage/myisam/myisamlog.c
> 
> per-file messages:
>   mysys/my_lock.c
>     Bug#38133 - Myisamlog test fails on Windows
>     Changed control flow so that LockFileEx() is not called after a failed
>     UnlockFileEx().
>     Added unlocking before locking to prevent multiple locks by the same process.
>     Moved errno retrieval down so that it is done for both unlock and lock.
>   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?

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

Why? Efficiency?

>     Reduced closing of files to cases where file descriptors are used up.

What problem is this fixing?

>     Added code to re-lock a file after re-open.

What problem is this fixing?

>     Pulled close_some_files() out of reopen_closed_file() to have access
>     to max_files.
>   storage/myisam/mi_locking.c
>     Bug#38133 - Myisamlog test fails on Windows
>     Added DBUG.

Actually the change to mi_locking.c does more than DBUG.

>   storage/myisam/myisamlog.c
>     Bug#38133 - Myisamlog test fails on Windows
>     Allowed for more of one (1!) file descriptor on platforms that
>     do not raise the maximum above the request.

> === 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
>  
>      if (locktype == F_UNLCK)
>      {
> +      DBUG_PRINT("my", ("UnlockFileEx handle: %d", hFile));
>        /* The lock flags are currently ignored by Windows */
>        if(UnlockFileEx(hFile, 0, liLength.LowPart, liLength.HighPart, &ov))
>          DBUG_RETURN(0);
> -      else
> -        lastError= GetLastError();
>      }
> -    else if (locktype == F_RDLCK)
> +    else
> +    {
> +      if (locktype == F_RDLCK)
> +      {
>          /* read lock is mapped to a shared lock. */
>          dwFlags= 0;
> -    else
> +      }
> +      else
> +      {
>          /* write lock is mapped to an exclusive lock. */
>          dwFlags= LOCKFILE_EXCLUSIVE_LOCK;
> +      }
>  
> -    if (MyFlags & MY_NO_WAIT)
> -       dwFlags|= LOCKFILE_FAIL_IMMEDIATELY;
> +      if (MyFlags & MY_NO_WAIT)
> +        dwFlags|= LOCKFILE_FAIL_IMMEDIATELY;
>  
> -    if (LockFileEx(hFile, dwFlags, 0, liLength.LowPart, liLength.HighPart,
> &ov))
> +      /*
> +        Drop old lock first to avoid double locking.
> +        During analyze of Bug#38133 (Myisamlog test fails on Windows)
> +        I met the situation that the program myisamlog locked the file
> +        exclusively, then additionally shared, then did one unlock, and
> +        then blocked on an attempt to lock it exclusively again.
> +        Unlocking before every lock fixed the problem.
> +      */
> +      (void) UnlockFileEx(hFile, 0, liLength.LowPart, liLength.HighPart, &ov);
> +
> +      DBUG_PRINT("my", ("LockFileEx handle: %d dwFlags: %d", hFile, dwFlags));
> +      if (LockFileEx(hFile, dwFlags, 0, liLength.LowPart,
> +                     liLength.HighPart, &ov))
>          DBUG_RETURN(0);
> +    }
> +    lastError= GetLastError();
>    }
>  #else
>  #if defined(HAVE_FCNTL)

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?
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")?

>    pthread_mutex_lock(&THR_LOCK_myisam);
>    if (info->lock_type == F_EXTRA_LCK)
> 
> === modified file 'storage/myisam/mi_examine_log.c'
> --- a/storage/myisam/mi_examine_log.c	2008-07-08 20:18:10 +0000
> +++ b/storage/myisam/mi_examine_log.c	2008-07-23 09:57:40 +0000
> @@ -69,6 +69,13 @@ struct file_info {
>    my_bool closed;
>    /** If this table matches the inclusion rules (or has to be ignored) */
>    my_bool used;
> +  /**
> +    Lock type, set by last MI_LOG_LOCK command. Initialized to F_UNLCK
> +    at MI_LOG_OPEN. This is not changed if the file is temporarily
> +    closed. So it can be re-locked on re-open.
> +  */
> +  int lock_type;
> +  /* File was last accessed at this log entry number. */
>    ulong accessed;
>  };
>  
> @@ -96,7 +103,7 @@ static int test_when_accessed(struct fil
>  			      struct st_access_param *access_param);
>  static void file_info_free(struct file_info *info);
>  static int close_some_file(TREE *tree);
> -static int reopen_closed_file(TREE *tree,struct file_info *file_info);
> +static int reopen_closed_file(struct file_info *file_info);
>  static int find_record_with_key(struct file_info *file_info,uchar *record);
>  static int mi_close_care_state(MI_INFO *info);
>  static void printf_log(uint verbose, ulong isamlog_process,
> @@ -142,7 +149,7 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>    int lock_command,mi_result;
>    char isam_file_name[FN_REFLEN], llbuff[21], llbuff2[21];
>    uchar head[20], *head_ptr;
> -  uchar	*buff;
> +  uchar	*buff= NULL;
>    struct test_if_open_param open_param;
>    IO_CACHE cache;
>    File log_file;
> @@ -155,6 +162,7 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>        { 11, 14 }, { 11, 14 }, {  9, 16 }, {  9, 16 }, {  7, 12 }  };
>    uint has_pid_and_result[]= {1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0};
>    DBUG_ENTER("mi_examine_log");
> +  DBUG_PRINT("myisamlog", ("max_files: %u", mi_exl->max_files));
>  
>    compile_time_assert((sizeof(mi_log_command_name) /
>                         sizeof(mi_log_command_name[0]) ==
> @@ -186,6 +194,14 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>    (void) init_key_cache(dflt_key_cache,KEY_CACHE_BLOCK_SIZE,KEY_CACHE_SIZE,
>                          0, 0);
>  
> +  /*
> +    Initialize members of file_info that are used for pointing to
> +    allocated memory. At the error labels we want to be able to free it.
> +  */
> +  file_info.name= NULL;
> +  file_info.show_name= NULL;
> +  file_info.record= NULL;
> +
>    files_open=0; access_time=0;
>    while (access_time++ != mi_exl->number_of_commands &&
>  	 !my_b_read(&cache, head, 1))
> @@ -194,6 +210,18 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>      head_ptr= head;
>      command=(uint) head_ptr[0];
>      command-= (big_numbers= (command & MI_LOG_BIG_NUMBERS));
> +    /*
> +      'command' is a number that is used to index arrays. Better check
> +      it for range.
> +    */
> +    if (command >= MI_LOG_END_SENTINEL)
> +    {
> +      my_errno= HA_ERR_WRONG_COMMAND;
> +      fprintf(stderr,"Unknown command %u in logfile at position %s\n",
> +              command, llstr(isamlog_filepos, llbuff));
> +      fflush(stderr);
> +      goto end;
> +    }
>      if (big_numbers != 0)
>        big_numbers= 1;
>      if (my_b_read(&cache, head, head_len[command][big_numbers] - 1))
> @@ -219,24 +247,61 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>      }
>      else
>        isamlog_process= file_info.process= result= 0;
> +
> +    DBUG_PRINT("myisamlog", ("command: %u '%s'  process: %ld  filenr: %d",
> +                             command, mi_log_command_name[command],
> +                             file_info.process, file_info.filenr));
> +    /*
> +      Try to find the file with file_info.process and file_info.filenr
> +      in the file tree. The search function, as registered with
> +      init_tree() is file_info_compare(). If the file does not exist in
> +      the tree, most commands will be ignored for this file.
> +    */
>      if ((curr_file_info=(struct file_info*) tree_search(&tree, &file_info,
>  							tree.custom_arg)))
>      {
> +      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.

> +                               curr_file_info->used,
> curr_file_info->closed));
>        curr_file_info->accessed=access_time;
> -      if (mi_exl->update && curr_file_info->used &&
> curr_file_info->closed)
> +      /*
> +        If the file has been closed due to lack of file descriptors,
> +        re-open it to execute the command.
> +        No need to re-open for the MI_LOG_CLOSE command.
> +      */
> +      if (mi_exl->update && curr_file_info->used &&
> curr_file_info->closed &&
> +          (command != MI_LOG_CLOSE))
>        {
> -	if (reopen_closed_file(&tree,curr_file_info))
> -	{
> -	  command=sizeof(mi_exl->com_count)/sizeof(mi_exl->com_count[0][0])/3;
> -	  result=0;
> -	  goto com_err;
> -	}
> +        /*
> +          We found a closed file. It can only be closed due to a lack
> +          of file descriptors. When a file is explicitly closed, its
> +          information is removed from the tree and freed.
> +          But this does not mean that there are still open files in
> +          the tree. All other files could have been explicitly closed
> +          meanwhile. So close a file only if there is still a lack of
> +          file descriptors.
> +        */
> +        if (files_open >= mi_exl->max_files)
> +        {
> +          if (close_some_file(&tree))
> +          {
> +            DBUG_PRINT("myisamlog", ("failed to close some file"));
> +            goto com_err; /* No file to close */
> +          }
> +          files_open--;
> +        }
> +        if (reopen_closed_file(curr_file_info))
> +        {
> +          DBUG_PRINT("myisamlog", ("failed to reopen closed file"));
> +         
> command=sizeof(mi_exl->com_count)/sizeof(mi_exl->com_count[0][0])/3;
> +          result=0;
> +          goto com_err;
> +        }
> +        files_open++;
>          mi_exl->re_open_count++;
>        }
>      }
> -    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?

> @@ -255,13 +320,19 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>      }
>      switch ((enum myisam_log_commands) command) {
>      case MI_LOG_OPEN:
> +      DBUG_PRINT("myisamlog", ("command MI_LOG_OPEN"));
>        if (curr_file_info)
>  	printf("\nWarning: %s is opened with same process and filenumber\n"
>                 "Maybe you should use the -P option ?\n",
>  	       curr_file_info->show_name);
> -      file_info.name=0;
> -      file_info.show_name=0;
> -      file_info.record=0;
> +      /*
> +        These file_info memebers should be non-null only during an open
> +        operation. Initially and after open they should be nulled.
> +        That way we can free them in case of a jump to an error label.
> +      */
> +      DBUG_ASSERT(!file_info.name);
> +      DBUG_ASSERT(!file_info.show_name);
> +      DBUG_ASSERT(!file_info.record);
>        length= big_numbers ? mi_uint4korr(head_ptr) : mi_uint2korr(head_ptr);
>        if (read_string(&cache, (uchar **)&file_info.name, length))
>  	goto err;
> @@ -330,8 +401,19 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>  	  goto end;
>  	files_open++;
>  	file_info.closed=0;
> +        /* After explicit open, file is not locked. */
> +        file_info.lock_type= F_UNLCK;
>        }
>        (void) tree_insert(&tree, (uchar*) &file_info, 0, tree.custom_arg);
> +
> +      /*
> +        tree_insert() copied file_info. Avoid that the allocated members
> +        are freed while in use.
> +      */
> +      file_info.name= NULL;
> +      file_info.show_name= NULL;
> +      file_info.record= NULL;
> +
>        if (file_info.used)
>        {
>  	if (mi_exl->verbose && !mi_exl->record_pos_file)
> @@ -438,7 +524,6 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>          head_ptr+= 4;
>          length= mi_uint2korr(head_ptr);
>        }
> -      buff=0;
>        if (read_string(&cache,&buff,length))
>  	goto err;
>        if ((!mi_exl->record_pos_file ||
> @@ -519,9 +604,11 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>  	}
>        }
>        my_free(buff,MYF(0));
> +      buff= NULL;
>        break;
>      case MI_LOG_WRITE_BYTES_MYI:
>      case MI_LOG_WRITE_BYTES_MYD:
> +      DBUG_PRINT("myisamlog", ("command MI_LOG_WRITE_BYTES_MYI or
> MI_LOG_WRITE_BYTES_MYD"));
>        if (big_numbers)
>        {
>          filepos= mi_sizekorr(head_ptr);
> @@ -534,7 +621,6 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>          head_ptr+= 4;
>          length= mi_uint2korr(head_ptr);
>        }
> -      buff=0;
>        if (read_string(&cache, &buff, length))
>          goto err;
>        if ((!mi_exl->record_pos_file ||
> @@ -565,8 +651,10 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>            goto com_err;
>        }
>        my_free(buff,MYF(0));
> +      buff= NULL;
>        break;
>      case MI_LOG_CHSIZE_MYI:
> +      DBUG_PRINT("myisamlog", ("command MI_LOG_CHSIZE_MYI"));
>        /* here 'filepos' means new length of file */
>        if (big_numbers)
>          filepos= mi_sizekorr(head_ptr);
> @@ -607,14 +696,24 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>  	printf_log(mi_exl->verbose, isamlog_process, isamlog_filepos,
>                     "%s: %s(%d) -> %d",FILENAME(curr_file_info),
>  		   mi_log_command_name[command],lock_command,result);
> -      if (mi_exl->update && curr_file_info &&
> !curr_file_info->closed)
> +      if (mi_exl->update && curr_file_info)
>        {
> -	if (mi_lock_database(curr_file_info->isam,lock_command) !=
> -	    (int) result)
> -	  goto com_err;
> +        /* Remember lock type for re-open. */
> +        curr_file_info->lock_type= lock_command;
> +        if (!curr_file_info->closed)
> +        {
> +          DBUG_PRINT("myisamlog",
> +                     ("lock info: 0x%lx  process: %ld  filenr: %d  file: '%s'",
> +                      (long) curr_file_info, curr_file_info->process,
> +                      curr_file_info->filenr, curr_file_info->name));
> +          if (mi_lock_database(curr_file_info->isam,lock_command) !=
> +              (int) result)
> +            goto com_err;
> +        }
>        }
>        break;
>      case MI_LOG_DELETE_ALL:
> +      DBUG_PRINT("myisamlog", ("command MI_LOG_DELETE_ALL"));
>        if (mi_exl->verbose && !mi_exl->record_pos_file &&
>  	  (!mi_exl->table_selection_hook ||
>             (curr_file_info && curr_file_info->used)))
> @@ -645,22 +747,29 @@ int mi_examine_log(MI_EXAMINE_LOG_PARAM 
>  
>   err:
>    fflush(stdout);
> +  DBUG_PRINT("myisamlog", ("err label"));
>    fprintf(stderr,"Got error %d when reading from logfile\n",my_errno);
>    fflush(stderr);
>    goto end;
>   com_err:
>    fflush(stdout);
> +  DBUG_PRINT("myisamlog", ("com_err label"));
>    fprintf(stderr,"Got error %d, expected %d on command %s at %s\n",
>            my_errno,result,mi_log_command_name[command],
>            llstr(isamlog_filepos,llbuff));
>    fflush(stderr);
>   end:
> +  DBUG_PRINT("myisamlog", ("end label"));
>    end_key_cache(dflt_key_cache, 1);
>    delete_tree(&tree);
>    (void) end_io_cache(&cache);
>    (void) my_close(log_file,MYF(0));
>    if (write_file)
>      (void) my_fclose(write_file,MYF(MY_WME));
> +  my_free(file_info.name, MYF(MY_ALLOW_ZERO_PTR));
> +  my_free(file_info.show_name, MYF(MY_ALLOW_ZERO_PTR));
> +  my_free(file_info.record, MYF(MY_ALLOW_ZERO_PTR));
> +  my_free(buff, MYF(MY_ALLOW_ZERO_PTR));
>    DBUG_RETURN(1);
>  }
>  
> @@ -742,6 +851,8 @@ static int test_when_accessed (struct fi
>  static void file_info_free(struct file_info *fileinfo)
>  {
>    DBUG_ENTER("file_info_free");
> +  DBUG_PRINT("myisamlog", ("freeing info: 0x%lx  file: '%s'",
> +                           (long) fileinfo, fileinfo->name));
>    /* The 2 conditions below can be true only if 'update' */
>    if (!fileinfo->closed)
>      (void) mi_close_care_state(fileinfo->isam);
> @@ -765,6 +876,9 @@ static int close_some_file(TREE *tree)
>                     (void*) &access_param,left_root_right);
>    if (!access_param.found)
>      return 1;			/* No open file that is possibly to close */
> +  DBUG_PRINT("myisamlog", ("closing info: 0x%lx  file: '%s'",
> +                           (long) access_param.found,
> +                           access_param.found->name));
>    if (mi_close_care_state(access_param.found->isam))
>      return 1;
>    access_param.found->closed=1;
> @@ -772,20 +886,35 @@ static int close_some_file(TREE *tree)
>  }
>  
>  
> -static int reopen_closed_file(TREE *tree, struct file_info *fileinfo)
> +static int reopen_closed_file(struct file_info *fileinfo)
>  {
>    char name[FN_REFLEN];
> -  if (close_some_file(tree))
> -    return 1;				/* No file to close */
> +  DBUG_ENTER("reopen_closed_file");
> +
>    strmov(name,fileinfo->show_name);
>    if (fileinfo->id > 1)
>      *strrchr(name,'<')='\0';		/* Remove "<id>" */
>  
>    if (!(fileinfo->isam= mi_open(name, O_RDWR,
>                                  HA_OPEN_FOR_REPAIR | HA_OPEN_WAIT_IF_LOCKED)))
> -    return 1;
> +    DBUG_RETURN(1);
>    fileinfo->closed=0;
> -  return 0;
> +  /*
> +    If the file was explicitly locked when we needed to close it due to
> +    lack of file descriptors, we re-lock it after re-open.
> +  */
> +  if (fileinfo->lock_type != F_UNLCK)
> +  {
> +    DBUG_PRINT("myisamlog",
> +               ("lock info: 0x%lx  process: %ld  filenr: %d  file: '%s'",
> +                (long) fileinfo, fileinfo->process,
> +                fileinfo->filenr, fileinfo->name));
> +    (void) mi_lock_database(fileinfo->isam, fileinfo->lock_type);
> +  }
> +  DBUG_PRINT("myisamlog", ("re-opened info: 0x%lx  lock_type: %d  file: '%s'",
> +                           (long) fileinfo, fileinfo->lock_type,
> +                           fileinfo->name));
> +  DBUG_RETURN(0);
>  }
>  
>  	/* Try to find record with uniq key */
> 
> === 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?

>  	{
>  	  if (share->r_locks)
>  	  {					/* Only read locks left */
> @@ -568,9 +572,11 @@ int _mi_decrement_open_count(MI_INFO *in
>    uchar buff[2];
>    register MYISAM_SHARE *share=info->s;
>    int lock_error=0,write_error=0;
> +  DBUG_ENTER("_mi_decrement_open_count");
>    if (share->global_changed)
>    {
>      uint old_lock=info->lock_type;
> +    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.

>      share->global_changed=0;
>      lock_error=mi_lock_database(info,F_WRLCK);
>      /* Its not fatal even if we couldn't get the lock ! */
> @@ -589,7 +595,7 @@ int _mi_decrement_open_count(MI_INFO *in
>      if (!lock_error)
>        lock_error=mi_lock_database(info,old_lock);
>    }
> -  return test(lock_error || write_error);
> +  DBUG_RETURN(test(lock_error || write_error));

You just wanted to change "return" to DBUG_RETURN, so my comment is
pointless, but I'll write it anyway: this test() is unneeded; the || is
guaranteed to return 0 or 1 by the C standard, so test() does not change
it, it just adds a useless if().

> === modified file 'storage/myisam/myisamlog.c'
> --- a/storage/myisam/myisamlog.c	2008-07-09 07:12:43 +0000
> +++ b/storage/myisam/myisamlog.c	2008-07-23 09:57:40 +0000
> @@ -59,8 +59,13 @@ int main(int argc, char **argv)
>      mi_exl.table_selection_hook= matches_list_of_tables;
>    }
>  
> -  /* Number of MyISAM files we can have open at one time */
> -  mi_exl.max_files= (my_set_max_open_files(max(mi_exl.max_files,8))-6)/2;
> +  /*
> +    Number of MyISAM files we can have open at one time.
> +    Some operating systems do not increase the limit above the
> +    input argument of my_set_max_open_files(). So don't start too low.
> +  */
> +  mi_exl.max_files=
> +    (my_set_max_open_files(max(mi_exl.max_files, MY_NFILE)) - 6) / 2;


In fact, after executing this "=", mi_exl.max_files serves as the limit
on open tables, not files (there is a factor of two between those); I
mean, in mi_examine_log.c, we increment mi_exl.max_files by one when we
open a table. So I suggest changing the comment's start to:
"Number of MyISAM tables we can have open at one time".

> @@ -75,6 +80,7 @@ int main(int argc, char **argv)
>  	   (mi_exl.recover ? "recover" : "update"),mi_exl.log_filename);
>  
>    error= mi_examine_log(&mi_exl);
> +  DBUG_PRINT("myisamlog", ("error from mi_examine_log: %d", error));

Do we want this new type of tag?


-- 
Mr. Guilhem Bichot <guilhem@stripped>
Sun Microsystems / MySQL, Lead Software Engineer
Bordeaux, France
www.sun.com / www.mysql.com