maria-developers team mailing list archive

[Michael Widenius <monty@xxxxxxxxxxxx>]

Hi!

>>>>> "Sergey" == Sergey Petrunya <psergey@xxxxxxxxxxxx> writes:

Sergey> Hi Monty,
Sergey> I've finished addressing the review feedback and I'm submitting the patches for
Sergey> another review. The attached are:
Sergey>  - cumulative all-included diff againist 5.3-main (should just apply)
Sergey>  - diff with post-review changes ( I needed to tweak that file a bit becasue
Sergey>    I've done a merge from -mwl128 tree, so I needed to remove those changes.

Sergey> The tree is at lp:~maria-captains/maria/maria-5.3-mwl128-dsmrr-cpk. 
Sergey> I'm not aware of any buildbot failures except the solaris32 crashes.

After IRC dicusussion Solaris issue located and hopefully fixed.

<cut>

> +++ maria-5.3-mwl128-dsmrr-cpk-noc/sql/multi_range_read.cc	2010-12-02 16:53:34.000000000 +0300

<cut>

> +    if (!skip_index_tuple(*(char**)cur_range_info) &&
>          !skip_record(*(char**)cur_range_info, NULL))
>      {

The above was the solaris sparc problem (unaligned access)

> @@ -385,93 +375,75 @@
>  
> +int Mrr_ordered_index_reader::refill_buffer(bool initial)
>  {
>    KEY_MULTI_RANGE cur_range;
>    uchar **range_info_ptr= (uchar**)&cur_range.ptr;
>    uchar *key_ptr;
>    DBUG_ENTER("Mrr_ordered_index_reader::refill_buffer");
>  
> +  DBUG_ASSERT(key_buffer->is_empty());
> +
> +  if (source_exhausted)
> +    DBUG_RETURN(HA_ERR_END_OF_FILE);
> +
> +  //if (know_key_tuple_params)

Remove the if line

>    {
> +    buf_manager->reset_buffer_sizes(buf_manager->arg);
>      key_buffer->reset();
>      key_buffer->setup_writing(&key_ptr, keypar.key_size_in_keybuf,
>                                is_mrr_assoc? (uchar**)&range_info_ptr : NULL,
>                                sizeof(uchar*));
>    }
>  
> +  while (key_buffer->can_write() && 
> +         !(source_exhausted= (bool)mrr_funcs.next(mrr_iter, &cur_range)))

Don't like that you cast it to a bool.  Something strange may happen if
mrr_funcs.next doesn't return 0 or 1.

Please do one of the following:
- Change next to return bool or at least my_bool
- Add test() around function call
- Change source_exhausted to uint

>    {
>      DBUG_ASSERT(cur_range.range_flag & EQ_RANGE);
>  
> +    key_ptr= (keypar.use_key_pointers)? (uchar*)&cur_range.start_key.key : 
> +                                        (uchar*)cur_range.start_key.key;

Add a comment that key_ptr is used in key_buffer by setup_writing
above.

Preferably, I would suggest one of the following solutions:

Use key_buffer->key_ptr instead of key_ptr
Use key_ptr as argument to write.

<cut>
>  
> -int Mrr_ordered_rndpos_reader::refill_buffer()
> +int Mrr_ordered_rndpos_reader::refill_buffer(bool initial)
>  {
>    int res;
>    DBUG_ENTER("Mrr_ordered_rndpos_reader::refill_buffer");
> @@ -558,14 +528,17 @@
>    if (index_reader_exhausted)
>      DBUG_RETURN(HA_ERR_END_OF_FILE);
>  
> +  while (initial || index_reader_needs_refill || 
> +         (res= refill_from_index_reader()) == HA_ERR_END_OF_FILE)

I assume you don't really need to test 'intial' as when intial is set
index_reader_needs_refill should also be set, shouldn't it?

However it's probably safer this way.

>    {
> +    if ((res= index_reader->refill_buffer(initial)))
>      {
>        if (res == HA_ERR_END_OF_FILE)
>          index_reader_exhausted= TRUE;
>        break;
>      }
> +    initial= FALSE;

Don't set initial (it's a local variable that you don't sue anymore)

> +    index_reader_needs_refill= FALSE;
>    }
>    DBUG_RETURN(res);
>  }

>  
> @@ -640,28 +622,25 @@
>        from a rowid that matched multiple range_ids. Return this record again,
>        with next matching range_id.
>      */
> +    (void)rowid_buffer->read();
>  
>      if (rowid == last_identical_rowid)
>        last_identical_rowid= NULL; /* reached the last of identical rowids */

I assume that rowid_buffer->read() reads the value into rowid.
In that case it should really be marked with volatile.
(Or one should access rowid_buffer->rowid)

> +int Mrr_ordered_index_reader::compare_keys(void* arg, uchar* key1, uchar* key2)
>  {
> +  Mrr_ordered_index_reader *reader= (Mrr_ordered_index_reader*)arg;
> +  TABLE *table= reader->file->get_table();
> +  KEY_PART_INFO *part= table->key_info[reader->file->active_index].key_part;
>    
> -  if (this_->keypar.use_key_pointers)
> +  if (reader->keypar.use_key_pointers)
>    {
>      /* the buffer stores pointers to keys, get to the keys */
>      key1= *((uchar**)key1);
>      key2= *((uchar**)key2);  // todo is this alignment-safe?
>    }

Not alignment safe. (Already fixed)


> +bool DsMrr_impl::setup_buffer_sharing(uint key_size_in_keybuf, 
> +                                      key_part_map key_tuple_map)
>  {
>    uint key_buff_elem_size= key_size_in_keybuf + 
>                             (int)is_mrr_assoc * sizeof(void*);
>    
> +  KEY *key_info= &primary_file->get_table()->key_info[keyno];
>    /* 
>      Ok if we got here we need to allocate one part of the buffer 
>      for keys and another part for rowids.
>    */
> +  ulonglong rowid_buf_elem_size= primary_file->ref_length + 
> +                                 (int)is_mrr_assoc * sizeof(char*);
>    
>    /*
>      Use rec_per_key statistics as a basis to find out how many rowids 
>      we'll get for each key value.
>       TODO: what should be the default value to use when there is no 
>             statistics?
>    */

For engines with no statistics, we need to look at generating these
for every key scan. 

For example, when we do 'read_next_same()' or for every range we read
we will know how many rows there are for that key part. Adjusting the
value with a floating average would probably be helpful.

Regards,
Monty