DS-MRR improvements: more code cleanup

- better comments
- rename variables to better reflect their meaning

sql/multi_range_read.cc

@@ -400,17 +400,6 @@ void SimpleBuffer::reset_for_writing()
     write_pos= read_pos= end;
 }
 
-void SimpleBuffer::reset_for_reading()
-{
-/*
-Do we need this at all?
-  if (direction == 1)
-    pos= start;
-  else
-    pos= end;
-//end?
-*/
-}
 
 uchar *SimpleBuffer::end_of_space()
 {
@@ -478,15 +467,20 @@ int DsMrr_impl::dsmrr_init(handler *h_arg, RANGE_SEQ_IF *seq_funcs,
     use_key_pointers= test(mode & HA_MRR_MATERIALIZED_KEYS);
   }
 
-  do_rowid_fetch= FALSE;
-  doing_cpk_scan= check_cpk_scan(thd, h->inited == handler::INDEX? 
+  do_rndpos_scan= FALSE;
+  bool doing_cpk_scan= check_cpk_scan(thd, h->inited == handler::INDEX? 
                                       h->active_index: h2->active_index, mode);
   if (!doing_cpk_scan /* && !index_only_read */)
   {
     /* Will use rowid buffer to store/sort rowids, etc */
-    do_rowid_fetch= TRUE;
+    do_rndpos_scan= TRUE;
   }
-  DBUG_ASSERT(do_sort_keys || do_rowid_fetch);
+
+  /* 
+    We should either sort keys, or do ordered rnd_pos scan, or both. If we
+    decide to do neither, we should have used default MRR implementation.
+  */
+  DBUG_ASSERT(do_sort_keys || do_rndpos_scan);
 
   
   if (is_mrr_assoc)
@@ -509,11 +503,11 @@ int DsMrr_impl::dsmrr_init(handler *h_arg, RANGE_SEQ_IF *seq_funcs,
     keyno= (h->inited == handler::INDEX)? h->active_index : h2->active_index;
     dsmrr_fill_key_buffer();
     
-    if (dsmrr_eof && !do_rowid_fetch)
+    if (dsmrr_eof && !do_rndpos_scan)
       buf->end_of_used_area= key_buffer.end_of_space();
   }
 
-  if (!do_rowid_fetch)
+  if (!do_rndpos_scan)
   {
     /* 
       We have the keys and won't need to fetch rowids, as key lookup will be
@@ -523,11 +517,6 @@ int DsMrr_impl::dsmrr_init(handler *h_arg, RANGE_SEQ_IF *seq_funcs,
   }
 
   rowid_buff_elem_size= h->ref_length + (is_mrr_assoc? sizeof(char*) : 0);
-  /*
-    psergey2: this is only needed when 
-      - doing a rowid-to-row scan
-      - the buffer wasn't exhausted on the first pass.
-  */
   /*
     There can be two cases:
     - This is the first call since index_init(), h2==NULL
@@ -821,7 +810,7 @@ void DsMrr_impl::setup_buffer_sizes(key_range *sample_key)
   index_ranges_unique= test(key_info->flags & HA_NOSAME && 
                             key_info->key_parts == 
                               my_count_bits(sample_key->keypart_map));
-  if (!do_rowid_fetch)
+  if (!do_rndpos_scan)
   {
     /* Give all space to key buffer. */
     key_buffer.set_buffer_space(full_buf, full_buf_end, SimpleBuffer::FORWARD);
@@ -908,7 +897,7 @@ void DsMrr_impl::dsmrr_fill_key_buffer()
   uchar *key_ptr;
   if (know_key_tuple_params)
   {
-    if (do_rowid_fetch && rowid_buffer.is_empty())
+    if (do_rndpos_scan && rowid_buffer.is_empty())
     {
       /*
         We're using two buffers and both of them are empty now. Restore the
@@ -963,6 +952,18 @@ void DsMrr_impl::dsmrr_fill_key_buffer()
 }
 
 
+/*
+  Take unused space from key buffer and give it to rowid buffer.
+*/
+
+void DsMrr_impl::reallocate_buffer_space()
+{
+  uchar *unused_start, *unused_end;
+  key_buffer.remove_unused_space(&unused_start, &unused_end);
+  rowid_buffer.grow(unused_start, unused_end);
+}
+
+
 /*
   DS-MRR/CPK: multi_range_read_next() function
 
@@ -993,7 +994,7 @@ int DsMrr_impl::dsmrr_next_from_index(char **range_info_arg)
 {
   int res;
   uchar *key_in_buf;
-  handler *file= do_rowid_fetch? h2: h;
+  handler *file= do_rndpos_scan? h2: h;
   bool res2;
 
   while (in_identical_keys_range)
@@ -1068,7 +1069,7 @@ int DsMrr_impl::dsmrr_next_from_index(char **range_info_arg)
         When rowid fetching is used, it controls all buffer refills. When we're
         on our own, try refilling our buffer.
       */
-      if (!do_rowid_fetch)
+      if (!do_rndpos_scan)
         dsmrr_fill_key_buffer();
 
       if (key_buffer.is_empty())
@@ -1078,17 +1079,13 @@ int DsMrr_impl::dsmrr_next_from_index(char **range_info_arg)
       }
     }
     
-    if (do_rowid_fetch)
-    {
     /*
       At this point we're not using anything what we've read from key
       buffer. Cut off unused key buffer space and give it to the rowid
       buffer.
     */
-      uchar *unused_start, *unused_end;
-      key_buffer.remove_unused_space(&unused_start, &unused_end);
-      rowid_buffer.grow(unused_start, unused_end);
-    }
+    if (do_rndpos_scan)
+      reallocate_buffer_space();
 
     /* Get the next range to scan */
     key_buffer.read(); // reads to (cur_index_tuple, cur_range_info)
@@ -1147,7 +1144,7 @@ int DsMrr_impl::dsmrr_next(char **range_info)
   if (use_default_impl)
     return h->handler::multi_range_read_next(range_info);
 
-  if (!do_rowid_fetch)
+  if (!do_rndpos_scan)
     return dsmrr_next_from_index(range_info);
   
   while (last_identical_rowid)
@@ -1421,7 +1418,7 @@ bool DsMrr_impl::choose_mrr_impl(uint keyno, ha_rows rows, uint *flags,
   bool res;
   THD *thd= current_thd;
 
-  doing_cpk_scan= check_cpk_scan(thd, keyno, *flags); 
+  bool doing_cpk_scan= check_cpk_scan(thd, keyno, *flags); 
   bool using_cpk= test(keyno == table->s->primary_key &&
                        h->primary_key_is_clustered());
   if (thd->variables.optimizer_use_mrr == 2 || *flags & HA_MRR_INDEX_ONLY ||

sql/multi_range_read.h

@@ -74,6 +74,8 @@
                 |                 |            
               usused space         user data
   
+  For reverse buffer, start/end have the same meaning, but reading and 
+  writing is done from end to start.
 */
 
 class SimpleBuffer
@@ -134,7 +136,6 @@ class SimpleBuffer
 
   /* Read-mode functions */
   bool is_empty() { return used_size() == 0; }
-  void reset_for_reading();
   void setup_reading(uchar **data1, size_t len1, 
                      uchar **data2, size_t len2);
   bool read();
@@ -209,23 +210,31 @@ class SimpleBuffer
   */
   class PeekIterator
   {
+    SimpleBuffer *buf; /* The buffer we're iterating over*/
     /*
-      if sb->direction==1 : pointer to what to return next
-      if sb->direction==-1: pointer to the end of what is to be returned next
+      if buf->direction==FORWARD  : pointer to what to return next
+      if buf->direction==BACKWARD : pointer to the end of what is to be 
+                                   returned next
     */
     uchar *pos;
-    SimpleBuffer *sb;
-    
   public:
-    void init(SimpleBuffer *sb_arg)
+    /* 
+      Initialize the iterator. After intiialization, the first read_next() call
+      will read what buf_arg->read() would read.
+    */
+    void init(SimpleBuffer *buf_arg)
     {
-      sb= sb_arg;
-      pos= sb->read_pos;
+      buf= buf_arg;
+      pos= buf->read_pos;
     }
     
     /*
-      If the buffer stores tuples, this call will return pointer to the first
-      component.
+      Read the next value. The calling convention is the same as buf->read()
+      has.
+
+      RETURN
+        FALSE - Ok
+        TRUE  - EOF, reached the end of the buffer
     */
     bool read_next()
     {
@@ -234,11 +243,11 @@ class SimpleBuffer
         have written the second component first).
       */
       uchar *res;
-      if ((res= get_next(sb->read_size1)))
+      if ((res= get_next(buf->read_size1)))
       {
-        *(sb->read_ptr1)= res;
-        if (sb->read_ptr2)
-          *sb->read_ptr2= get_next(sb->read_size2);
+        *(buf->read_ptr1)= res;
+        if (buf->read_ptr2)
+          *buf->read_ptr2= get_next(buf->read_size2);
         return FALSE;
       }
       return TRUE; /* EOF */
@@ -247,9 +256,9 @@ class SimpleBuffer
     /* Return pointer to next chunk of nbytes bytes and avance over it */
     uchar *get_next(size_t nbytes)
     {
-      if (sb->direction == 1)
+      if (buf->direction == 1)
       {
-        if (pos + nbytes > sb->write_pos)
+        if (pos + nbytes > buf->write_pos)
           return NULL;
         uchar *res= pos;
         pos += nbytes;
@@ -257,7 +266,7 @@ class SimpleBuffer
       }
       else
       {
-        if (pos - nbytes < sb->write_pos)
+        if (pos - nbytes < buf->write_pos)
           return NULL;
         pos -= nbytes;
         return pos;
@@ -288,6 +297,8 @@ class SimpleBuffer
    S2. Sort Keys
    S3. Sort Rowids
 
+  psergey-TODO.
+
   S1 is used for cases which DS-MRR is unable to handle for some reason.
 
   S2 is the actual DS-MRR. The basic algorithm is as follows:
@@ -339,75 +350,78 @@ class DsMrr_impl
                             uint *flags, COST_VECT *cost);
 private:
   /*
-    The "owner" handler object (the one that calls dsmrr_XXX functions.
-    It is used to retrieve full table rows by calling rnd_pos().
+    The "owner" handler object (the one that is expected to "own" this object
+    and call its functions).
   */
   handler *h;
   TABLE *table; /* Always equal to h->table */
 
   /*
-    Secondary handler object, if needed (we need it when we need to both scan
-    the index and return rows).
+    Secondary handler object. (created when needed, we need it when we need 
+    to run both index scan and rnd_pos() at the same time)
   */
   handler *h2;
   
-  /* Full buffer that we're using (the buffer is obtained from SQL layer) */
+  /** Properties of current MRR scan **/
+
+  uint keyno; /* index we're running the scan on */
+  bool use_default_impl; /* TRUE <=> shortcut all calls to default MRR impl */
+  /* TRUE <=> need range association, buffers hold {rowid, range_id} pairs */
+  bool is_mrr_assoc;
+  /* TRUE <=> sort the keys before making index lookups */
+  bool do_sort_keys;
+  /* TRUE <=> sort rowids and use rnd_pos() to get and return full records */
+  bool do_rndpos_scan;
+
+  /*
+    (if do_sort_keys==TRUE) don't copy key values, use pointers to them 
+    instead.
+  */
+  bool use_key_pointers;
+
+
+  /* The whole buffer space that we're using */
   uchar *full_buf;
   uchar *full_buf_end;
   
-  /* Valid when using both rowid and key buffer: the original bound between them */
+  /* 
+    When using both rowid and key buffers: the bound between key and rowid
+    parts of the buffer. This is the "original" value, actual memory ranges 
+    used by key and rowid parts may be different because of dynamic space 
+    reallocation between them.
+  */
   uchar *rowid_buffer_end;
  
-  /* Buffer to store rowids, or (rowid, range_id) pairs */
-  SimpleBuffer rowid_buffer;
 
-  /*  Reads from rowid buffer go to here: */
-  uchar *rowid;
-  uchar *rowids_range_id;
+  /** Index scaning and key buffer-related members **/
   
-  /*
-    not-NULL: we're traversing a group of (rowid, range_id) pairs with
-              identical rowid values, and this is the pointer to the last one.
-    NULL: we're not in the group of indentical rowids.
-  */
-  uchar *last_identical_rowid;
+  /* TRUE <=> We can get at most one index tuple for a lookup key */
+  bool index_ranges_unique;
 
-  /* Identical keys */
-  bool in_identical_keys_range;
-  uchar *last_identical_key_ptr;
-  SimpleBuffer::PeekIterator identical_key_it;
+  /* TRUE<=> we're in a middle of enumerating records for a key range */
+  bool in_index_range;
   
+  /* Buffer to store (key, range_id) pairs */
   SimpleBuffer key_buffer;
    
-  uint keyno;
-
-  /* Execution control */
-  bool do_sort_keys;
-  bool use_key_pointers;
-  bool do_rowid_fetch;
+  /* key_buffer.read() reads */
+  uchar *cur_index_tuple;
 
-  bool dsmrr_eof; /* TRUE <=> We have reached EOF when reading index tuples */
+  /* if in_index_range==TRUE: range_id of the range we're enumerating */
+  char *cur_range_info;
 
   /* 
-    TRUE <=> key buffer is exhausted (we need this because we may have a situation
-    where we've read everything from the key buffer but haven't finished with
-    scanning the last range)
+    TRUE <=> we've got index tuples/rowids for all keys (need this flag because 
+    we may have a situation where we've read everything from the key buffer but 
+    haven't finished with getting index tuples for the last key)
   */
   bool key_eof;
 
-  /* TRUE <=> need range association, buffer holds {rowid, range_id} pairs */
-  bool is_mrr_assoc;
-
-  bool use_default_impl; /* TRUE <=> shortcut all calls to default MRR impl */
-
-  bool doing_cpk_scan; /* TRUE <=> DS-MRR/CPK variant is used */
-
-  
   /* Initially FALSE, becomes TRUE when we've set key_tuple_xxx members */
   bool know_key_tuple_params;
-  /* Length of lookup tuple being used, in bytes */
-  uint key_tuple_length;
-  key_part_map key_tuple_map; 
+  uint         key_tuple_length; /* Length of index lookup tuple, in bytes */
+  key_part_map key_tuple_map;    /* keyparts used in index lookup tuples */
+
   /*
     This is 
       = key_tuple_length   if we copy keys to buffer
@@ -418,22 +432,51 @@ class DsMrr_impl
   /* = key_size_in_keybuf [ + sizeof(range_assoc_info) ] */
   uint key_buff_elem_size;
   
-  /* = h->ref_length  [ + sizeof(range_assoc_info) ] */
-  uint rowid_buff_elem_size;
+  /* 
+    TRUE <=> we're doing key-ordered index scan and right now several
+    subsequent key values are the same as the one we've already retrieved and
+    returned index tuple for.
+  */
+  bool in_identical_keys_range;
+
+  /* range_id of the first of the identical keys */
+  char *first_identical_range_info;
+
+  /* Pointer to the last of the identical key values */
+  uchar *last_identical_key_ptr;
 
   /* 
-    TRUE <=> We're scanning on a full primary key (and not on prefix), and so 
-    can get max. one match for each key 
+    key_buffer iterator for walking the identical key range (we need to
+    enumerate the set of (identical_key, range_id) pairs multiple times,
+    and do that by walking from current buffer read position until we get
+    last_identical_key_ptr.
   */
-  bool index_ranges_unique;
-  /* TRUE<=> we're in a middle of enumerating records from a range */ 
-  bool in_index_range;
-  uchar *cur_index_tuple;
+  SimpleBuffer::PeekIterator identical_key_it;
 
-  /* if in_index_range==TRUE: range_id of the range we're enumerating */
-  char *cur_range_info;
 
-  char *first_identical_range_info;
+  /** rnd_pos() scan and rowid buffer-related members **/
+
+  /*
+    Buffer to store (rowid, range_id) pairs, or just rowids if 
+    is_mrr_assoc==FALSE
+  */
+  SimpleBuffer rowid_buffer;
+  
+  /* rowid_buffer.read() will set the following:  */
+  uchar *rowid;
+  uchar *rowids_range_id;
+  
+  /*
+    not-NULL: we're traversing a group of (rowid, range_id) pairs with
+              identical rowid values, and this is the pointer to the last one.
+    NULL: we're not in the group of indentical rowids.
+  */
+  uchar *last_identical_rowid;
+
+  bool dsmrr_eof; /* TRUE <=> We have reached EOF when reading index tuples */
+  
+  /* = h->ref_length  [ + sizeof(range_assoc_info) ] */
+  uint rowid_buff_elem_size;
   
   bool choose_mrr_impl(uint keyno, ha_rows rows, uint *flags, uint *bufsz, 
                        COST_VECT *cost);
@@ -446,8 +489,10 @@ class DsMrr_impl
   int dsmrr_next_from_index(char **range_info);
 
   void setup_buffer_sizes(key_range *sample_key);
+  void reallocate_buffer_space();
 
   static range_seq_t key_buf_seq_init(void *init_param, uint n_ranges, uint flags);
   static uint key_buf_seq_next(range_seq_t rseq, KEY_MULTI_RANGE *range);
 };
 
+