MWL#121-125 DS-MRR improvements

- Address Monty's review feedback, portion 2

sql/multi_range_read.cc

@@ -351,7 +351,7 @@ int Mrr_ordered_index_reader::get_next(char **range_info)
       scanning_key_val_iter= TRUE;
     }
 
-    if ((res= kv_it.get_next()))
+    if ((res= kv_it.get_next(range_info)))
     {
       scanning_key_val_iter= FALSE;
       if ((res != HA_ERR_KEY_NOT_FOUND && res != HA_ERR_END_OF_FILE))
@@ -359,17 +359,14 @@ int Mrr_ordered_index_reader::get_next(char **range_info)
       kv_it.move_to_next_key_value();
       continue;
     }
-    char *range_info;
-    memcpy(&range_info, cur_range_info, sizeof(char*));
-    if (!skip_index_tuple(range_info) &&
-        !skip_record(range_info, NULL))
+    if (!skip_index_tuple(*range_info) &&
+        !skip_record(*range_info, NULL))
     {
       break;
     }
     /* Go get another (record, range_id) combination */
   } /* while */
 
-  memcpy(range_info, cur_range_info, sizeof(void*));
   DBUG_RETURN(0);
 }
 
@@ -430,7 +427,7 @@ int Mrr_ordered_index_reader::refill_buffer(bool initial)
   key_buffer->reset();
   key_buffer->setup_writing(&key_ptr, keypar.key_size_in_keybuf,
                             is_mrr_assoc? (uchar**)&range_info_ptr : NULL,
-                            sizeof(uchar*));
+                            is_mrr_assoc? sizeof(uchar*):0);
 
   while (key_buffer->can_write() && 
          !(source_exhausted= mrr_funcs.next(mrr_iter, &cur_range)))
@@ -603,7 +600,7 @@ int Mrr_ordered_rndpos_reader::refill_from_index_reader()
   rowid_buffer->reset();
   rowid_buffer->setup_writing(&index_rowid, file->ref_length,
                               is_mrr_assoc? (uchar**)&range_info_ptr: NULL,
-                              sizeof(void*));
+                              is_mrr_assoc? sizeof(char*):0);
 
   last_identical_rowid= NULL;
 
@@ -630,9 +627,8 @@ int Mrr_ordered_rndpos_reader::refill_from_index_reader()
   /* Sort the buffer contents by rowid */
   rowid_buffer->sort((qsort2_cmp)rowid_cmp_reverse, (void*)file);
 
-  rowid_buffer->setup_reading(&rowid, file->ref_length,
-                              is_mrr_assoc? (uchar**)&rowids_range_id: NULL,
-                              sizeof(void*));
+  rowid_buffer->setup_reading(file->ref_length,
+                              is_mrr_assoc ? sizeof(char*) : 0);
   DBUG_RETURN(rowid_buffer->is_empty()? HA_ERR_END_OF_FILE : 0);
 }
 
@@ -662,14 +658,14 @@ int Mrr_ordered_rndpos_reader::get_next(char **range_info)
     */
     (void)rowid_buffer->read();
 
-    if (rowid == last_identical_rowid)
+    if (rowid_buffer->read_ptr1 == last_identical_rowid)
       last_identical_rowid= NULL; /* reached the last of identical rowids */
 
     if (!is_mrr_assoc)
       return 0;
 
-    memcpy(range_info, rowids_range_id, sizeof(uchar*));
-    if (!index_reader->skip_record((char*)*range_info, rowid))
+    memcpy(range_info, rowid_buffer->read_ptr2, sizeof(uchar*));
+    if (!index_reader->skip_record((char*)*range_info, rowid_buffer->read_ptr1))
       return 0;
   }
   
@@ -685,13 +681,13 @@ int Mrr_ordered_rndpos_reader::get_next(char **range_info)
 
     if (is_mrr_assoc)
     {
-      memcpy(range_info, rowids_range_id, sizeof(uchar*));
-
-      if (index_reader->skip_record(*range_info, rowid))
+      memcpy(range_info, rowid_buffer->read_ptr2, sizeof(uchar*));
+      if (index_reader->skip_record(*range_info, rowid_buffer->read_ptr1))
         continue;
     }
 
-    res= file->ha_rnd_pos(file->get_table()->record[0], rowid);
+    res= file->ha_rnd_pos(file->get_table()->record[0], 
+                          rowid_buffer->read_ptr1);
 
     if (res == HA_ERR_RECORD_DELETED)
     {
@@ -709,19 +705,17 @@ int Mrr_ordered_rndpos_reader::get_next(char **range_info)
     Check if subsequent buffer elements have the same rowid value as this
     one. If yes, remember this fact so that we don't make any more rnd_pos()
     calls with this value.
-  */
-  uchar *cur_rowid= rowid;
-  /* 
+
     Note: this implies that SQL layer doesn't touch table->record[0]
     between calls.
   */
   Lifo_buffer_iterator it;
   it.init(rowid_buffer);
-  while (!it.read()) // reads to (rowid, ...)
+  while (!it.read())
   {
-    if (file->cmp_ref(rowid, cur_rowid))
+    if (file->cmp_ref(it.read_ptr1, rowid_buffer->read_ptr1))
       break;
-    last_identical_rowid= rowid;
+    last_identical_rowid= it.read_ptr1;
   }
   return 0;
 }
@@ -1202,36 +1196,30 @@ int Key_value_records_iterator::init(Mrr_ordered_index_reader *owner_arg)
   owner= owner_arg;
 
   identical_key_it.init(owner->key_buffer);
-  /* Get the first pair into (cur_index_tuple, cur_range_info) */ 
-  owner->key_buffer->setup_reading(&cur_index_tuple, 
-                                   owner->keypar.key_size_in_keybuf,
-                                   owner->is_mrr_assoc? 
-                                     (uchar**)&owner->cur_range_info: NULL,
-                                   sizeof(void*));
+  owner->key_buffer->setup_reading(owner->keypar.key_size_in_keybuf,
+                                   owner->is_mrr_assoc ? sizeof(void*) : 0);
 
   if (identical_key_it.read())
     return HA_ERR_END_OF_FILE;
 
-  uchar *key_in_buf= cur_index_tuple;
+  uchar *key_in_buf= last_identical_key_ptr= identical_key_it.read_ptr1;
 
-  last_identical_key_ptr= cur_index_tuple;
+  uchar *index_tuple= key_in_buf;
   if (owner->keypar.use_key_pointers)
-    memcpy(&cur_index_tuple, key_in_buf, sizeof(char*));
+    memcpy(&index_tuple, key_in_buf, sizeof(char*));
   
   /* Check out how many more identical keys are following */
-  uchar *save_cur_index_tuple= cur_index_tuple;
   while (!identical_key_it.read())
   {
     if (owner->disallow_identical_key_handling ||
         Mrr_ordered_index_reader::compare_keys(owner, key_in_buf, 
-                                               cur_index_tuple))
+                                               identical_key_it.read_ptr1))
       break;
-    last_identical_key_ptr= cur_index_tuple;
+    last_identical_key_ptr= identical_key_it.read_ptr1;
   }
   identical_key_it.init(owner->key_buffer);
-  cur_index_tuple= save_cur_index_tuple;
   res= owner->file->ha_index_read_map(owner->file->get_table()->record[0], 
-                                   cur_index_tuple, 
+                                      index_tuple, 
                                       owner->keypar.key_tuple_map, 
                                       HA_READ_KEY_EXACT);
 
@@ -1247,7 +1235,7 @@ int Key_value_records_iterator::init(Mrr_ordered_index_reader *owner_arg)
 }
 
 
-int Key_value_records_iterator::get_next()
+int Key_value_records_iterator::get_next(char **range_info)
 {
   int res;
 
@@ -1261,7 +1249,7 @@ int Key_value_records_iterator::get_next()
     
     handler *h= owner->file;
     if ((res= h->ha_index_next_same(h->get_table()->record[0], 
-                                    cur_index_tuple, 
+                                    identical_key_it.read_ptr1, 
                                     owner->keypar.key_tuple_length)))
     {
       /* It's either HA_ERR_END_OF_FILE or some other error */
@@ -1273,7 +1261,10 @@ int Key_value_records_iterator::get_next()
   }
 
   identical_key_it.read(); /* This gets us next range_id */
-  if (!last_identical_key_ptr || (cur_index_tuple == last_identical_key_ptr))
+  memcpy(range_info, identical_key_it.read_ptr2, sizeof(char*));
+
+  if (!last_identical_key_ptr || 
+      (identical_key_it.read_ptr1 == last_identical_key_ptr))
   {
     /* 
       We've reached the last of the identical keys that current record is a
@@ -1289,7 +1280,7 @@ int Key_value_records_iterator::get_next()
 void Key_value_records_iterator::move_to_next_key_value()
 {
   while (!owner->key_buffer->read() && 
-         (cur_index_tuple != last_identical_key_ptr)) {}
+         (owner->key_buffer->read_ptr1 != last_identical_key_ptr)) {}
 }
 
 

sql/multi_range_read.h

@@ -133,10 +133,10 @@ class Key_value_records_iterator
   */
   bool get_next_row;
   
-  uchar *cur_index_tuple; /* key_buffer.read() reads to here */
+  //uchar *cur_index_tuple; /* key_buffer.read() reads to here */
 public:
   int init(Mrr_ordered_index_reader *owner_arg);
-  int get_next();
+  int get_next(char **range_info);
   void move_to_next_key_value();
 };
 
@@ -281,9 +281,6 @@ private:
 
   bool scanning_key_val_iter;
   
-  /* Key_value_records_iterator::read() will place range_info here */
-  char *cur_range_info;
-
   /* Buffer to store (key, range_id) pairs */
   Lifo_buffer *key_buffer;
   
@@ -367,8 +364,8 @@ private:
   Lifo_buffer *rowid_buffer;
   
   /* rowid_buffer.read() will set the following:  */
-  uchar *rowid;
-  uchar *rowids_range_id;
+  //uchar *rowid;
+  //uchar *rowids_range_id;
 
   int refill_from_index_reader();
 };

sql/sql_lifo_buffer.h

@@ -39,14 +39,16 @@ protected:
   uchar **write_ptr2;
   size_t size2;
 
+public:
   /**
-    read() will do reading by storing pointer to read data into *read_ptr1 (if
-    the buffer stores atomic elements), or into {*read_ptr1, *read_ptr2} (if
-    the buffer stores pairs).
+    read() will do reading by storing pointers to read data into read_ptr1 or
+    into (read_ptr1, read_ptr2), depending on whether the buffer was set to
+    store single objects or pairs.
   */
-  uchar **read_ptr1;
-  uchar **read_ptr2;
+  uchar *read_ptr1;
+  uchar *read_ptr2;
 
+protected:
   uchar *start; /**< points to start of buffer space */
   uchar *end;   /**< points to just beyond the end of buffer space */
 public:
@@ -85,11 +87,9 @@ public:
     Specify where read() should store pointers to read data, as well as read
     data size. The sizes must match those passed to setup_writing().
   */
-  void setup_reading(uchar **data1, size_t len1, uchar **data2, size_t len2)
+  void setup_reading(size_t len1, size_t len2)
   {
-    read_ptr1= data1;
     DBUG_ASSERT(len1 == size1);
-    read_ptr2= data2;
     DBUG_ASSERT(len2 == size2);
   }
   
@@ -116,7 +116,7 @@ protected:
   
   /* To be used only by iterator class: */
   virtual uchar *get_pos()= 0;
-  virtual bool read(uchar **position)= 0;
+  virtual bool read(uchar **position, uchar **ptr1, uchar **ptr2)= 0;
   friend class Lifo_buffer_iterator;
 public:
   virtual bool have_space_for(size_t bytes) = 0;
@@ -184,14 +184,14 @@ public:
     *position= (*position) - bytes;
     return *position;
   }
-  bool read() { return read(&pos); }
-  bool read(uchar **position)
+  bool read() { return read(&pos, &read_ptr1, &read_ptr2); }
+  bool read(uchar **position, uchar **ptr1, uchar **ptr2)
   {
-    if (!have_data(*position, size1 + (read_ptr2 ? size2 : 0)))
+    if (!have_data(*position, size1 + size2))
       return TRUE;
-    if (read_ptr2)
-      *read_ptr2= read_bytes(position, size2);
-    *read_ptr1= read_bytes(position, size1);
+    if (size2)
+      *ptr2= read_bytes(position, size2);
+    *ptr1= read_bytes(position, size1);
     return FALSE;
   }
   void remove_unused_space(uchar **unused_start, uchar **unused_end)
@@ -268,15 +268,15 @@ public:
   }
   bool read()
   {
-    return read(&pos);
+    return read(&pos, &read_ptr1, &read_ptr2);
   }
-  bool read(uchar **position)
+  bool read(uchar **position, uchar **ptr1, uchar **ptr2)
   {
-    if (!have_data(*position, size1 + (read_ptr2 ? size2 : 0)))
+    if (!have_data(*position, size1 + size2))
       return TRUE;
-    *read_ptr1= read_bytes(position, size1);
-    if (read_ptr2)
-      *read_ptr2= read_bytes(position, size2);
+    *ptr1= read_bytes(position, size1);
+    if (size2)
+      *ptr2= read_bytes(position, size2);
     return FALSE;
   }
   bool have_data(uchar *position, size_t bytes)
@@ -312,13 +312,17 @@ public:
 };
 
 
-
-/** Iterator to walk over contents of the buffer without reading it. */
+/** Iterator to walk over contents of the buffer without reading from it */
 class Lifo_buffer_iterator
 {
   uchar *pos;
   Lifo_buffer *buf;
+  
 public:
+  /* The data is read to here */
+  uchar *read_ptr1;
+  uchar *read_ptr2;
+
   void init(Lifo_buffer *buf_arg)
   {
     buf= buf_arg;
@@ -333,7 +337,7 @@ public:
   */
   bool read() 
   {
-    return buf->read(&pos);
+    return buf->read(&pos, &read_ptr1, &read_ptr2);
   }
 };