Fixed bug lp:809245

In addition to the bug fix explained below, the patch performs
few renames, and adds some comments to avoid similar problems.

Analysis:
The failed assert was due to a bug in MWL#68, where it was
incorrectly assumed that the size of the bitmap
subselect_rowid_merge_engine::null_only_columns should be
the same as the size of the array of Ordered_keys.

The bitmap null_only_columns contains bits to mark columns
that contain only NULLs. Therefore the indexes of the bits
to be set in null_only_columns are different from the indexes
of the Ordered_keys. If there is a NULL-only column that appears
in a table after the last partial match column with Ordered_key,
this NULL-only column would require setting a bit with index
bigger than the size of the bitmap null_only_columns.

Accessing such a bit caused the failed assert.

Solution:
Upon analysis, it turns out that null_only_columns is not needed
at all, because we are looking for partial matches, and having
such columns guarantees that there is a partial match for any
corresponding outer value.

Therefore the patch removes
  subselect_rowid_merge_engine::null_only_columns.

1 files changed, 36 insertions, 43 deletions

diff --git a/sql/item_subselect.cc b/sql/item_subselect.cc
index f7c778dbdbf..3ce82bf6dac 100644
--- a/sql/item_subselect.cc
+++ b/sql/item_subselect.cc
@@ -3864,8 +3864,7 @@ subselect_hash_sj_engine::get_strategy_using_data()
       bitmap_set_bit(&non_null_key_parts, i);
       --count_partial_match_columns;
     }
-    if (result_sink->get_null_count_of_col(i) ==
-               tmp_table->file->stats.records)
+    if (result_sink->get_null_count_of_col(i) == tmp_table->file->stats.records)
       ++count_null_only_columns;
   }
 
@@ -5172,7 +5171,6 @@ void subselect_partial_match_engine::print(String *str,
 /*
   @param non_null_key_parts  
   @param partial_match_key_parts  A union of all single-column NULL key parts.
-  @param count_partial_match_columns Number of NULL keyparts (set bits above).
 
   @retval FALSE  the engine was initialized successfully
   @retval TRUE   there was some (memory allocation) error during initialization,
@@ -5189,24 +5187,24 @@ subselect_rowid_merge_engine::init(MY_BITMAP *non_null_key_parts,
   rownum_t cur_rownum= 0;
   select_materialize_with_stats *result_sink=
     (select_materialize_with_stats *) result;
-  uint cur_keyid= 0;
+  uint cur_keyid;
   Item_in_subselect *item_in= (Item_in_subselect*) item;
   int error;
 
-  if (keys_count == 0)
+  if (merge_keys_count == 0)
   {
     /* There is nothing to initialize, we will only do regular lookups. */
     return FALSE;
   }
 
   DBUG_ASSERT(!covering_null_row_width || (covering_null_row_width &&
-                                           keys_count == 1 &&
+                                           merge_keys_count == 1 &&
                                            non_null_key_parts));
   /*
     Allocate buffers to hold the merged keys and the mapping between rowids and
     row numbers.
   */
-  if (!(merge_keys= (Ordered_key**) thd->alloc(keys_count *
+  if (!(merge_keys= (Ordered_key**) thd->alloc(merge_keys_count *
                                                sizeof(Ordered_key*))) ||
       !(row_num_to_rowid= (uchar*) my_malloc((size_t)(row_count * rowid_length),
         MYF(MY_WME))))
@@ -5215,14 +5213,16 @@ subselect_rowid_merge_engine::init(MY_BITMAP *non_null_key_parts,
   /* Create the only non-NULL key if there is any. */
   if (non_null_key_parts)
   {
-    non_null_key= new Ordered_key(cur_keyid, tmp_table, item_in->left_expr,
+    non_null_key= new Ordered_key(0, tmp_table, item_in->left_expr,
                                   0, 0, 0, row_num_to_rowid);
     if (non_null_key->init(non_null_key_parts))
       return TRUE;
-    merge_keys[cur_keyid]= non_null_key;
-    merge_keys[cur_keyid]->first();
-    ++cur_keyid;
+    merge_keys[0]= non_null_key;
+    merge_keys[0]->first();
+    cur_keyid= 1;
   }
+  else
+    cur_keyid= 0;
 
   /*
     If there is a covering NULL row, the only key that is needed is the
@@ -5231,9 +5231,8 @@ subselect_rowid_merge_engine::init(MY_BITMAP *non_null_key_parts,
   */
   if (!covering_null_row_width)
   {
-    if (bitmap_init_memroot(&matching_keys, keys_count, thd->mem_root) ||
-        bitmap_init_memroot(&matching_outer_cols, keys_count, thd->mem_root) ||
-        bitmap_init_memroot(&null_only_columns, keys_count, thd->mem_root))
+    if (bitmap_init_memroot(&matching_keys, merge_keys_count, thd->mem_root) ||
+        bitmap_init_memroot(&matching_outer_cols, merge_keys_count, thd->mem_root))
       return TRUE;
 
     /*
@@ -5242,31 +5241,25 @@ subselect_rowid_merge_engine::init(MY_BITMAP *non_null_key_parts,
     */
     for (uint i= 0; i < partial_match_key_parts->n_bits; i++)
     {
-      if (!bitmap_is_set(partial_match_key_parts, i))
+      /* Skip columns that have no NULLs, or contain only NULLs. */
+      if (!bitmap_is_set(partial_match_key_parts, i) ||
+          result_sink->get_null_count_of_col(i) == row_count)
         continue;
 
-      if (result_sink->get_null_count_of_col(i) == row_count)
-      {
-        bitmap_set_bit(&null_only_columns, cur_keyid);
-        continue;
-      }
-      else
-      {
-        merge_keys[cur_keyid]= new Ordered_key(
+      merge_keys[cur_keyid]= new Ordered_key(
                                      cur_keyid, tmp_table,
                                      item_in->left_expr->element_index(i),
                                      result_sink->get_null_count_of_col(i),
                                      result_sink->get_min_null_of_col(i),
                                      result_sink->get_max_null_of_col(i),
                                      row_num_to_rowid);
-        if (merge_keys[cur_keyid]->init(i))
-          return TRUE;
-        merge_keys[cur_keyid]->first();
-      }
+      if (merge_keys[cur_keyid]->init(i))
+        return TRUE;
+      merge_keys[cur_keyid]->first();
       ++cur_keyid;
     }
   }
-  DBUG_ASSERT(cur_keyid == keys_count);
+  DBUG_ASSERT(cur_keyid == merge_keys_count);
 
   /* Populate the indexes with data from the temporary table. */
   if (tmp_table->file->ha_rnd_init_with_error(1))
@@ -5307,7 +5300,7 @@ subselect_rowid_merge_engine::init(MY_BITMAP *non_null_key_parts,
       non_null_key->add_key(cur_rownum);
     }
 
-    for (uint i= (non_null_key ? 1 : 0); i < keys_count; i++)
+    for (uint i= (non_null_key ? 1 : 0); i < merge_keys_count; i++)
     {
       /*
         Check if the first and only indexed column contains NULL in the curent
@@ -5324,14 +5317,14 @@ subselect_rowid_merge_engine::init(MY_BITMAP *non_null_key_parts,
   tmp_table->file->ha_rnd_end();
 
   /* Sort all the keys by their NULL selectivity. */
-  my_qsort(merge_keys, keys_count, sizeof(Ordered_key*),
+  my_qsort(merge_keys, merge_keys_count, sizeof(Ordered_key*),
            (qsort_cmp) cmp_keys_by_null_selectivity);
 
   /* Sort the keys in each of the indexes. */
-  for (uint i= 0; i < keys_count; i++)
+  for (uint i= 0; i < merge_keys_count; i++)
     merge_keys[i]->sort_keys();
 
-  if (init_queue(&pq, keys_count, 0, FALSE,
+  if (init_queue(&pq, merge_keys_count, 0, FALSE,
                  subselect_rowid_merge_engine::cmp_keys_by_cur_rownum, NULL,
                  0, 0))
     return TRUE;
@@ -5343,10 +5336,10 @@ subselect_rowid_merge_engine::init(MY_BITMAP *non_null_key_parts,
 subselect_rowid_merge_engine::~subselect_rowid_merge_engine()
 {
   /* None of the resources below is allocated if there are no ordered keys. */
-  if (keys_count)
+  if (merge_keys_count)
   {
     my_free((char*) row_num_to_rowid, MYF(0));
-    for (uint i= 0; i < keys_count; i++)
+    for (uint i= 0; i < merge_keys_count; i++)
       delete merge_keys[i];
     delete_queue(&pq);
     if (tmp_table->file->inited == handler::RND)
@@ -5404,6 +5397,10 @@ subselect_rowid_merge_engine::cmp_keys_by_cur_rownum(void *arg,
   Check if certain table row contains a NULL in all columns for which there is
   no match in the corresponding value index.
 
+  @note
+  There is no need to check the columns that contain only NULLs, because
+  those are guaranteed to match.
+
   @retval TRUE if a NULL row exists
   @retval FALSE otherwise
 */
@@ -5411,16 +5408,14 @@ subselect_rowid_merge_engine::cmp_keys_by_cur_rownum(void *arg,
 bool subselect_rowid_merge_engine::test_null_row(rownum_t row_num)
 {
   Ordered_key *cur_key;
-  uint cur_id;
-  for (uint i = 0; i < keys_count; i++)
+  for (uint i = 0; i < merge_keys_count; i++)
   {
     cur_key= merge_keys[i];
-    cur_id= cur_key->get_keyid();
-    if (bitmap_is_set(&matching_keys, cur_id))
+    if (bitmap_is_set(&matching_keys, cur_key->get_keyid()))
     {
       /*
-        The key 'i' (with id 'cur_keyid') already matches a value in row 'row_num',
-        thus we skip it as it can't possibly match a NULL.
+        The key 'i' (with id 'cur_keyid') already matches a value in row
+        'row_num', thus we skip it as it can't possibly match a NULL.
       */
       continue;
     }
@@ -5481,7 +5476,7 @@ bool subselect_rowid_merge_engine::partial_match()
     Do not add the non_null_key, since it was already processed above.
   */
   bitmap_clear_all(&matching_outer_cols);
-  for (uint i= test(non_null_key); i < keys_count; i++)
+  for (uint i= test(non_null_key); i < merge_keys_count; i++)
   {
     DBUG_ASSERT(merge_keys[i]->get_column_count() == 1);
     if (merge_keys[i]->get_search_key(0)->null_value)
@@ -5519,7 +5514,6 @@ bool subselect_rowid_merge_engine::partial_match()
 
   min_key= (Ordered_key*) queue_remove_top(&pq);
   min_row_num= min_key->current();
-  bitmap_copy(&matching_keys, &null_only_columns);
   bitmap_set_bit(&matching_keys, min_key->get_keyid());
   bitmap_union(&matching_keys, &matching_outer_cols);
   if (min_key->next_same())
@@ -5555,7 +5549,6 @@ bool subselect_rowid_merge_engine::partial_match()
       {
         min_key= cur_key;
         min_row_num= cur_row_num;
-        bitmap_copy(&matching_keys, &null_only_columns);
         bitmap_set_bit(&matching_keys, min_key->get_keyid());
         bitmap_union(&matching_keys, &matching_outer_cols);
       }