Hi!

...

...

...

...

"Sergey" == Sergey Petrunya <psergey@askmonty.org> 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