Re: bk commit into 5.1 tree (istruewing:1.2555) BUG#29838

[Antony T Curtis]

Hi Ingo,

I personally would prefer the swap the order of the code a little and to
change the 'if' expression to be something like:


  if (search_flag == HA_READ_KEY_EXACT &&
      last_used_keyseg == keyinfo->seg + keyinfo->keysegs)
  {
    /*
      (Bug #29838)
      We have found a row which matches the key exactly but it
      was inserted by another thread since we obtained a lock
      which permitted concurrent inserts. The record may not
      even be present on disk yet because keys are inserted 
      before the record appended to the data file..
    */
    my_errno= HA_ERR_KEY_NOT_FOUND;
    info->lastpos= HA_OFFSET_ERROR;
  }
  else
  {
    do
    {
      ... 
    } while (...)
  }

This allows someone viewing the source to easily see at least the start
of both branches of the 'if' statement, even on an 80x25 console... Also
the 'if' expression more clearly shows when that part will be executed.

Aside from that, I have no other complaints.

Regards,

Antony,


On Sat, 2007-07-28 at 19:01 +0200, Ingo Struewing wrote:
> Below is the list of changes that have just been committed into a local
> 5.1 repository of istruewing. When istruewing does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2007-07-28 19:01:19+02:00, istruewing@stripped +1 -0
>   Bug#29838 - myisam corruption using concurrent select ... and update
>   
>   When using concurrent insert with parallel index reads, it could
>   happen that reading sessions found keys that pointed to records
>   yet to be written to the data file. The result was a report of
>   a corrupted table. But it was false alert.
>   
>   When inserting a record in a table with indexes, the keys are
>   inserted into the indexes before the record is written to the data
>   file. When the insert happens concurrently to selects, an
>   index read can find a key that references the record that is not
>   yet written to the data file. To avoid any access to such record,
>   the select saves the current end of file position when it starts.
>   Since concurrent inserts are always appended at end of the data
>   file, the select can easily ignore any concurrently inserted record.
>   
>   The problem was that the ignore was only done for non-exact key
>   searches (partial key or using >, >=, < or <=).
>   
>   The fix is to ignore concurrently inserted records also for
>   exact key searches.
>   
>   No test case. Concurrent inserts cannot be tested with the test
>   suite. Test cases are attached to the bug report.
>   
>   The fix shall go to 4.1 and up when approved.
> 
>   storage/myisam/mi_rkey.c@stripped, 2007-07-28 19:01:15+02:00, istruewing@stripped
> +48 -33
>     Bug#29838 - myisam corruption using concurrent select ... and update
>     Fixed mi_rkey() to always ignore records beyond saved eof.
> 
> diff -Nrup a/storage/myisam/mi_rkey.c b/storage/myisam/mi_rkey.c
> --- a/storage/myisam/mi_rkey.c	2007-05-10 11:59:32 +02:00
> +++ b/storage/myisam/mi_rkey.c	2007-07-28 19:01:15 +02:00
> @@ -93,43 +93,58 @@ int mi_rkey(MI_INFO *info, uchar *buf, i
>      if (!_mi_search(info, keyinfo, key_buff, use_key_length,
>                      myisam_read_vec[search_flag],
> info->s->state.key_root[inx]))
>      {
> -      /*
> -        If we searching for a partial key (or using >, >=, < or <=) and
> -        the data is outside of the data file, we need to continue searching
> -        for the first key inside the data file
> -      */
> -      if (info->lastpos >= info->state->data_file_length &&
> -          (search_flag != HA_READ_KEY_EXACT ||
> -           last_used_keyseg != keyinfo->seg + keyinfo->keysegs))
> +      if (info->lastpos >= info->state->data_file_length)
>        {
> -        do
> +        /*
> +          If we searching for a partial key (or using >, >=, < or <=)
> and
> +          the data is outside of the data file, we need to continue searching
> +          for the first key inside the data file
> +        */
> +        if (search_flag != HA_READ_KEY_EXACT ||
> +            last_used_keyseg != keyinfo->seg + keyinfo->keysegs)
> +        {
> +          do
> +          {
> +            uint not_used[2];
> +            /*
> +              Skip rows that are inserted by other threads since we got a lock
> +              Note that this can only happen if we are not searching after an
> +              full length exact key, because the keys are sorted
> +              according to position
> +            */
> +            if  (_mi_search_next(info, keyinfo, info->lastkey,
> +                                 info->lastkey_length,
> +                                 myisam_readnext_vec[search_flag],
> +                                 info->s->state.key_root[inx]))
> +              break; /* purecov: inspected */
> +            /*
> +              Check that the found key does still match the search.
> +              _mi_search_next() delivers the next key regardless of its
> +              value.
> +            */
> +            if (search_flag == HA_READ_KEY_EXACT &&
> +                ha_key_cmp(keyinfo->seg, key_buff, info->lastkey,
> +                           use_key_length, SEARCH_FIND, not_used))
> +            {
> +              /* purecov: begin inspected */
> +              my_errno= HA_ERR_KEY_NOT_FOUND;
> +              info->lastpos= HA_OFFSET_ERROR;
> +              break;
> +              /* purecov: end */
> +            }
> +          } while (info->lastpos >= info->state->data_file_length);
> +        }
> +        else
>          {
> -          uint not_used[2];
> -          /*
> -            Skip rows that are inserted by other threads since we got a lock
> -            Note that this can only happen if we are not searching after an
> -            full length exact key, because the keys are sorted
> -            according to position
> -          */
> -          if  (_mi_search_next(info, keyinfo, info->lastkey,
> -                               info->lastkey_length,
> -                               myisam_readnext_vec[search_flag],
> -                               info->s->state.key_root[inx]))
> -            break;
>            /*
> -            Check that the found key does still match the search.
> -            _mi_search_next() delivers the next key regardless of its
> -            value.
> +            We cannot use rows that are inserted by other threads since
> +            we got a lock ("concurrent inserts"). The record may not
> +            even be present yet. Keys are inserted before the record is.
> +            (Bug #29838)
>            */
> -          if (search_flag == HA_READ_KEY_EXACT &&
> -              ha_key_cmp(keyinfo->seg, key_buff, info->lastkey,
> use_key_length,
> -                         SEARCH_FIND, not_used))
> -          {
> -            my_errno= HA_ERR_KEY_NOT_FOUND;
> -            info->lastpos= HA_OFFSET_ERROR;
> -            break;
> -          }
> -        } while (info->lastpos >= info->state->data_file_length);
> +          my_errno= HA_ERR_KEY_NOT_FOUND;
> +          info->lastpos= HA_OFFSET_ERROR;
> +        }
>        }
>      }
>    }
> 
-- 
Antony T Curtis, Senior Software Developer
MySQL Inc, www.mysql.com
SIP: 4468@stripped