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

[Vladislav Vaintroub]

Guys,
FWIW, I had a problem is a similar problem to yours, I worked around in
http://lists.mysql.com/commits/51808  The problem was lock file once /
unlock-twice.  Not a problem to unlock unlocked region on *nix, but on
Windows it is unless on we suppress ERROR_NOT_LOCKED (exactly what I did in
this patch)



> -----Original Message-----
> From: Guilhem.Bichot@stripped [mailto:Guilhem.Bichot@stripped] On Behalf
> Of Guilhem Bichot
> Sent: Thursday, November 06, 2008 2:21 PM
> To: Ingo Struewing
> Cc: commits@stripped
> Subject: Re: bzr commit into mysql-6.0-backup branch
> (ingo.struewing:2672) Bug#38133
> 
> 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
> 
> 
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1