Fix for bug#5138 continued: added comments, removed extra debug printf calls, changed ha_heap::records_in_range to use table->rec_per_key.

heap/hp_block.c:
  Fix for bug#5138 continued: Added comments.
heap/hp_delete.c:
  Fix for bug#5138 continued: Added comments.
heap/hp_write.c:
  Fix for bug#5138 continued: Added comments, removed unneeded printf
include/heap.h:
  Fix for bug#5138 continued: Added comments.
mysql-test/r/heap_hash.result:
  Fix for bug#5138 continued: added FLUSH TABLES and rec_per_key estimates tests, updated test results
mysql-test/t/heap_hash.test:
  Fix for bug#5138 continued: added FLUSH TABLES and rec_per_key estimates tests, updated test results
sql/ha_heap.cc:
  Fix for bug#5138 continued: 
    fixed comments to be correct
    removed unneded printf
    use TABLE::rec_per_key for records_in_range statistics

3 files changed, 124 insertions, 28 deletions

diff --git a/heap/hp_block.c b/heap/hp_block.c
index 6a022fb3084..f26b208b521 100644
--- a/heap/hp_block.c
+++ b/heap/hp_block.c
@@ -18,12 +18,19 @@
 
 #include "heapdef.h"
 
-	/* Find record according to record-position */
+/*
+  Find record according to record-position.
+      
+  The record is located by factoring position number pos into (p_0, p_1, ...) 
+  such that
+     pos = SUM_i(block->level_info[i].records_under_level * p_i)
+  {p_0, p_1, ...} serve as indexes to descend the blocks tree.
+*/
 
 byte *hp_find_block(HP_BLOCK *block, ulong pos)
 {
   reg1 int i;
-  reg3 HP_PTRS *ptr;
+  reg3 HP_PTRS *ptr; /* block base ptr */
 
   for (i=block->levels-1, ptr=block->root ; i > 0 ; i--)
   {
@@ -34,8 +41,18 @@ byte *hp_find_block(HP_BLOCK *block, ulong pos)
 }
 
 
-	/* get one new block-of-records. Alloc ptr to block if neaded */
-	/* Interrupts are stopped to allow ha_panic in interrupts */
+/*
+  Get one new block-of-records. Alloc ptr to block if needed
+  SYNOPSIS
+    hp_get_new_block()
+      block             HP_BLOCK tree-like block
+      alloc_length OUT  Amount of memory allocated from the heap
+      
+  Interrupts are stopped to allow ha_panic in interrupts 
+  RETURN
+    0  OK
+    1  Out of memory
+*/
 
 int hp_get_new_block(HP_BLOCK *block, ulong *alloc_length)
 {
@@ -46,6 +63,18 @@ int hp_get_new_block(HP_BLOCK *block, ulong *alloc_length)
     if (block->level_info[i].free_ptrs_in_block)
       break;
 
+  /*
+    Allocate space for leaf block plus space for upper level blocks up to
+    first level that has a free slot to put the pointer. 
+    In some cases we actually allocate more then we need:
+    Consider e.g. a situation where we have one level 1 block and one level 0
+    block, the level 0 block is full and this function is called. We only 
+    need a leaf block in this case. Nevertheless, we will get here with i=1 
+    and will also allocate sizeof(HP_PTRS) for non-leaf block and will never 
+    use this space.
+    This doesn't add much overhead - with current values of sizeof(HP_PTRS) 
+    and my_default_record_cache_size we get about 1/128 unused memory.
+   */
   *alloc_length=sizeof(HP_PTRS)*i+block->records_in_block* block->recbuffer;
   if (!(root=(HP_PTRS*) my_malloc(*alloc_length,MYF(0))))
     return 1;
@@ -60,21 +89,33 @@ int hp_get_new_block(HP_BLOCK *block, ulong *alloc_length)
     dont_break();		/* Dont allow SIGHUP or SIGINT */
     if ((uint) i == block->levels)
     {
+      /* Adding a new level on top of the existing ones. */
       block->levels=i+1;
+      /*
+        Use first allocated HP_PTRS as a top-level block. Put the current
+        block tree into the first slot of a new top-level block.
+      */
       block->level_info[i].free_ptrs_in_block=HP_PTRS_IN_NOD-1;
       ((HP_PTRS**) root)[0]= block->root;
       block->root=block->level_info[i].last_blocks= root++;
     }
+    /* Occupy the free slot we've found at level i */
     block->level_info[i].last_blocks->
       blocks[HP_PTRS_IN_NOD - block->level_info[i].free_ptrs_in_block--]=
 	(byte*) root;
-
+    
+    /* Add a block subtree with each node having one left-most child */
     for (j=i-1 ; j >0 ; j--)
     {
       block->level_info[j].last_blocks= root++;
       block->level_info[j].last_blocks->blocks[0]=(byte*) root;
       block->level_info[j].free_ptrs_in_block=HP_PTRS_IN_NOD-1;
     }
+    
+    /* 
+      root now points to last (block->records_in_block* block->recbuffer)
+      allocated bytes. Use it as a leaf block.
+    */
     block->level_info[0].last_blocks= root;
     allow_break();		/* Allow SIGHUP & SIGINT */
   }
diff --git a/heap/hp_delete.c b/heap/hp_delete.c
index a89fe49f495..9cf8b8936b6 100644
--- a/heap/hp_delete.c
+++ b/heap/hp_delete.c
@@ -97,8 +97,8 @@ int hp_rb_delete_key(HP_INFO *info, register HP_KEYDEF *keyinfo,
     flag		Is set if we want's to correct info->current_ptr
 
   RETURN
-    0	ok
-    #	error number
+    0      Ok
+    other  Error code
 */
 
 int hp_delete_key(HP_INFO *info, register HP_KEYDEF *keyinfo,
diff --git a/heap/hp_write.c b/heap/hp_write.c
index 853360976bf..43cee67b39c 100644
--- a/heap/hp_write.c
+++ b/heap/hp_write.c
@@ -36,7 +36,6 @@ int heap_write(HP_INFO *info, const byte *record)
   byte *pos;
   HP_SHARE *share=info->s;
   DBUG_ENTER("heap_write");
-  printf("heap_write\n");
 #ifndef DBUG_OFF
   if (info->mode & O_RDONLY)
   {
@@ -160,7 +159,31 @@ static byte *next_free_record_pos(HP_SHARE *info)
 	      block_pos*info->block.recbuffer);
 }
 
-	/* Write a hash-key to the hash-index */
+
+/*
+  Write a hash-key to the hash-index
+  SYNOPSIS
+    info     Heap table info
+    keyinfo  Key info
+    record   Table record to added
+    recpos   Memory buffer where the table record will be stored if added 
+             successfully
+  NOTE
+    Hash index uses HP_BLOCK structure as a 'growable array' of HASH_INFO 
+    structs. Array size == number of entries in hash index.
+    hp_mask(hp_rec_hashnr()) maps hash entries values to hash array positions.
+    If there are several hash entries with the same hash array position P,
+    they are connected in a linked list via HASH_INFO::next_key. The first 
+    list element is located at position P, next elements are located at 
+    positions for which there is no record that should be located at that
+    position. The order of elements in the list is arbitrary.
+
+  RETURN
+    0  - OK
+    -1 - Out of memory
+    HA_ERR_FOUND_DUPP_KEY - Duplicate record on unique key. The record was 
+    still added and the caller must call hp_delete_key for it.
+*/
 
 int hp_write_key(HP_INFO *info, HP_KEYDEF *keyinfo,
 		 const byte *record, byte *recpos)
@@ -182,33 +205,52 @@ int hp_write_key(HP_INFO *info, HP_KEYDEF *keyinfo,
   pos= hp_find_hash(&keyinfo->block,(first_index=share->records-halfbuff));
   
   /*
-    We're about to add one more hash position, with hash_mask=#records. 
-    Entries that should be relocated to that position are currently members 
-    of the list that starts at #first_index position.
-    At #first_index position there may be either:
-    a) A list of items with hash_mask=first_index. The list contains 
+    We're about to add one more hash array position, with hash_mask=#records.
+    The number of hash positions will change and some entries might need to 
+    be relocated to the newly added position. Those entries are currently 
+    members of the list that starts at #first_index position (this is 
+    guaranteed by properties of hp_mask(hp_rec_hashnr(X)) mapping function)
+    At #first_index position currently there may be either:
+    a) An entry with hashnr != first_index. We don't need to move it.
+    or
+    b) A list of items with hash_mask=first_index. The list contains entries
+       of 2 types:
        1) entries that should be relocated to the list that starts at new 
-          position we're adding
+          position we're adding ('uppper' list)
        2) entries that should be left in the list starting at #first_index 
-          position 
-    or
-    b) An entry with hashnr != first_index. We don't need to move it.
+          position ('lower' list)
   */
   if (pos != empty)				/* If some records */
   {
     do
     {
       hashnr = hp_rec_hashnr(keyinfo, pos->ptr_to_rec);
-      if (flag == 0)				/* First loop; Check if ok */
+      if (flag == 0)
       {
-        /* Bail out if we're dealing with case b) from above comment */
+        /* 
+          First loop, bail out if we're dealing with case a) from above 
+          comment
+        */
 	if (hp_mask(hashnr, share->blength, share->records) != first_index)
 	  break;
       }
+      /*
+        flag & LOWFIND - found a record that should be put into lower position
+        flag & LOWUSED - lower position occupied by the record
+        Same for HIGHFIND and HIGHUSED and 'upper' position
+
+        gpos  - ptr to last element in lower position's list
+        gpos2 - ptr to last element in upper position's list
+
+        ptr_to_rec - ptr to last entry that should go into lower list.
+        ptr_to_rec2 - same for upper list.
+      */
       if (!(hashnr & halfbuff))
-      {						/* Key will not move */
+      {						
+        /* Key should be put into 'lower' list */
 	if (!(flag & LOWFIND))
 	{
+          /* key is the first element to go into lower position */
 	  if (flag & HIGHFIND)
 	  {
 	    flag=LOWFIND | HIGHFIND;
@@ -219,16 +261,21 @@ int hp_write_key(HP_INFO *info, HP_KEYDEF *keyinfo,
 	  }
 	  else
 	  {
-	    flag=LOWFIND | LOWUSED;		/* key isn't changed */
+            /*
+              We can only get here at first iteration: key is at 'lower' 
+              position pos and should be left here.
+            */
+	    flag=LOWFIND | LOWUSED;
 	    gpos=pos;
 	    ptr_to_rec=pos->ptr_to_rec;
 	  }
 	}
 	else
-	{
+        {
+          /* Already have another key for lower position */
 	  if (!(flag & LOWUSED))
 	  {
-	    /* Change link of previous LOW-key */
+	    /* Change link of previous lower-list key */
 	    gpos->ptr_to_rec=ptr_to_rec;
 	    gpos->next_key=pos;
 	    flag= (flag & HIGHFIND) | (LOWFIND | LOWUSED);
@@ -238,19 +285,21 @@ int hp_write_key(HP_INFO *info, HP_KEYDEF *keyinfo,
 	}
       }
       else
-      {						/* key will be moved */
+      {
+        /* key will be put into 'higher' list */
 	if (!(flag & HIGHFIND))
 	{
 	  flag= (flag & LOWFIND) | HIGHFIND;
 	  /* key shall be moved to the last (empty) position */
-	  gpos2 = empty; empty=pos;
+	  gpos2= empty;
+          empty= pos;
 	  ptr_to_rec2=pos->ptr_to_rec;
 	}
 	else
 	{
 	  if (!(flag & HIGHUSED))
 	  {
-	    /* Change link of previous hash-key and save */
+	    /* Change link of previous upper-list key and save */
 	    gpos2->ptr_to_rec=ptr_to_rec2;
 	    gpos2->next_key=pos;
 	    flag= (flag & LOWFIND) | (HIGHFIND | HIGHUSED);
@@ -262,8 +311,14 @@ int hp_write_key(HP_INFO *info, HP_KEYDEF *keyinfo,
     }
     while ((pos=pos->next_key));
     
-    if ((flag & (LOWFIND | HIGHFIND)) == (LOWFIND | HIGHFIND)) 
+    if ((flag & (LOWFIND | HIGHFIND)) == (LOWFIND | HIGHFIND))
+    {
+      /*
+        If both 'higher' and 'lower' list have at least one element, now
+        there are two hash buckets instead of one.
+      */
       keyinfo->hash_buckets++;
+    }
 
     if ((flag & (LOWFIND | LOWUSED)) == LOWFIND)
     {