Code cleanups and add some caching of functions to speed up things

Detailed description:
- Added more function comments and fixed types in some old comments
- Removed an outdated comment
- Cleaned up some functions in records.cc
  - Replaced "while" with "if"
  - Reused error code
  - Made functions similar
- Added caching of pfs_batch_update()
- Simplified some rowid_filter code
  - Only call build_range_rowid_filter() if rowid filter will be used
  - Replaced tab->is_rowid_filter_built with need_to_build_rowid_filter.
    We only have to test need_to_build_rowid_filter to know if we have
    to build the filter. Old code needed two tests
  - Added function 'clear_range_rowid_filter' to disable rowid filter.
    Made things simpler as we can now clear all rowid filter variables
    in one place.
- Removed some 'if' in sub_select()

sql/records.cc

@@ -400,11 +400,8 @@ static int rr_handle_error(READ_RECORD *info, int error)
 static int rr_quick(READ_RECORD *info)
 {
   int tmp;
-  while ((tmp= info->select->quick->get_next()))
-  {
+  if ((tmp= info->select->quick->get_next()))
     tmp= rr_handle_error(info, tmp);
-    break;
-  }
   return tmp;
 }
 
@@ -427,16 +424,14 @@ static int rr_index_first(READ_RECORD *info)
   int tmp;
   // tell handler that we are doing an index scan
   if ((tmp = info->table->file->prepare_index_scan())) 
-  {
-    tmp= rr_handle_error(info, tmp);
-    return tmp;
-  }
+    goto err;
 
-  tmp= info->table->file->ha_index_first(info->record());
   info->read_record_func= rr_index;
-  if (tmp)
-    tmp= rr_handle_error(info, tmp);
+  if (!(tmp= info->table->file->ha_index_first(info->record())))
     return tmp;
+
+err:
+  return rr_handle_error(info, tmp);
 }
 
 
@@ -455,9 +450,9 @@ static int rr_index_first(READ_RECORD *info)
 
 static int rr_index_last(READ_RECORD *info)
 {
-  int tmp= info->table->file->ha_index_last(info->record());
+  int tmp;
   info->read_record_func= rr_index_desc;
-  if (tmp)
+  if ((tmp= info->table->file->ha_index_last(info->record())))
     tmp= rr_handle_error(info, tmp);
   return tmp;
 }

sql/sql_join_cache.cc

@@ -2146,12 +2146,12 @@ enum_nested_loop_state JOIN_CACHE::join_records(bool skip_last)
 
   if (!join_tab->first_unmatched)
   {
-    bool pfs_batch_update= join_tab->pfs_batch_update(join);
-    if (pfs_batch_update)
+    DBUG_ASSERT(join_tab->cached_pfs_batch_update == join_tab->pfs_batch_update());
+    if (join_tab->cached_pfs_batch_update)
       join_tab->table->file->start_psi_batch_mode();
     /* Find all records from join_tab that match records from join buffer */
     rc= join_matching_records(skip_last);   
-    if (pfs_batch_update)
+    if (join_tab->cached_pfs_batch_update)
       join_tab->table->file->end_psi_batch_mode();
     if (rc != NESTED_LOOP_OK && rc != NESTED_LOOP_NO_MORE_ROWS)
       goto finish;
@@ -2321,7 +2321,8 @@ enum_nested_loop_state JOIN_CACHE::join_matching_records(bool skip_last)
   if ((rc= join_tab_execution_startup(join_tab)) < 0)
     goto finish2;
 
-  join_tab->build_range_rowid_filter_if_needed();
+  if (join_tab->need_to_build_rowid_filter)
+    join_tab->build_range_rowid_filter();
 
   /* Prepare to retrieve all records of the joined table */
   if (unlikely((error= join_tab_scan->open())))

sql/sql_select.cc

@@ -1993,8 +1993,11 @@ bool JOIN::make_range_rowid_filters()
                                                tab->range_rowid_filter_info,
                                                filter_container, sel);
       if (tab->rowid_filter)
+      {
+        tab->need_to_build_rowid_filter= true;
         continue;
       }
+    }
   no_filter:
     if (sel->quick)
       delete sel->quick;
@@ -2028,16 +2031,16 @@ JOIN::init_range_rowid_filters()
        tab;
        tab= next_linear_tab(this, tab, WITH_BUSH_ROOTS))
   {
+    tab->need_to_build_rowid_filter= false;     // Safety
     if (!tab->rowid_filter)
       continue;
     if (tab->rowid_filter->get_container()->alloc())
     {
-      delete tab->rowid_filter;
-      tab->rowid_filter= 0;
+      tab->clear_range_rowid_filter();
       continue;
     }
     tab->table->file->rowid_filter_push(tab->rowid_filter);
-    tab->is_rowid_filter_built= false;
+    tab->need_to_build_rowid_filter= true;
   }
   DBUG_RETURN(0);
 }
@@ -13331,10 +13334,7 @@ make_join_select(JOIN *join,SQL_SELECT *select,COND *cond)
 	    sel->quick_keys.clear_all();
 	    sel->needed_reg.clear_all();
             if (is_hj && tab->rowid_filter)
-	    {
-              delete tab->rowid_filter;
-              tab->rowid_filter= 0;
-	    }
+              tab->clear_range_rowid_filter();
 	  }
 	  else
 	  {
@@ -14021,6 +14021,7 @@ void set_join_cache_denial(JOIN_TAB *join_tab)
       don't do join buffering for the first table in sjm nest. 
     */
     join_tab[-1].next_select= sub_select;
+    join_tab[-1].cached_pfs_batch_update= join_tab[-1].pfs_batch_update();
     if (join_tab->type == JT_REF && join_tab->is_ref_for_hash_join())
     {
       join_tab->type= JT_ALL;
@@ -14850,9 +14851,7 @@ make_join_readinfo(JOIN *join, ulonglong options, uint no_jbuf_after)
     */
     if (!(tab->bush_root_tab && 
           tab->bush_root_tab->bush_children->end == tab + 1))
-    {
-      tab->next_select=sub_select;		/* normal select */
-    }
+      tab->next_select= sub_select;		/* normal select */
 
     if (tab->loosescan_match_tab)
     {
@@ -15011,6 +15010,7 @@ make_join_readinfo(JOIN *join, ulonglong options, uint no_jbuf_after)
       abort();
       /* purecov: end */
     }
+    tab->cached_pfs_batch_update= tab->pfs_batch_update();
 
     DBUG_EXECUTE("where",
                  char buff[256];
@@ -15113,10 +15113,18 @@ bool error_if_full_join(JOIN *join)
 }
 
 
-void JOIN_TAB::build_range_rowid_filter_if_needed()
+/**
+   build_range_rowid_filter()
+
+   Build range rowid filter.  This function should only be called if
+   need_to_build_rowid_filter
+   is true
+*/
+
+void JOIN_TAB::build_range_rowid_filter()
 {
-  if (rowid_filter && !is_rowid_filter_built)
-  {
+  DBUG_ASSERT(need_to_build_rowid_filter && rowid_filter);
+
   /**
      The same handler object (table->file) is used to build a filter
      and to perfom a primary table access (by the main query).
@@ -15125,25 +15133,38 @@ void JOIN_TAB::build_range_rowid_filter_if_needed()
      and after building of the filter has been finished it should be
      switched back to the previos tracker.
   */
+
   Exec_time_tracker *table_tracker= table->file->get_time_tracker();
   Rowid_filter_tracker *rowid_tracker= rowid_filter->get_tracker();
   table->file->set_time_tracker(rowid_tracker->get_time_tracker());
   rowid_tracker->start_tracking(join->thd);
-    if (!rowid_filter->build())
-    {
-      is_rowid_filter_built= true;
-    }
-    else
+
+  if (rowid_filter->build())
   {
-      delete rowid_filter;
-      rowid_filter= 0;
+    /* Failed building rowid filter */
+    clear_range_rowid_filter();
   }
+  need_to_build_rowid_filter= false;
   rowid_tracker->stop_tracking(join->thd);
   table->file->set_time_tracker(table_tracker);
-  }
 }
 
 
+/*
+  Clear used rowid filter
+
+  Note that rowid_filter is allocated on mem_root and not really freed!
+  Only the rowid data is freed.
+*/
+
+void JOIN_TAB::clear_range_rowid_filter()
+{
+  delete rowid_filter;
+  rowid_filter= 0;
+  need_to_build_rowid_filter= false;
+  range_rowid_filter_info= 0;
+}
+
 /**
   cleanup JOIN_TAB.
 
@@ -15164,10 +15185,7 @@ void JOIN_TAB::cleanup()
   delete quick;
   quick= 0;
   if (rowid_filter)
-  {
-    delete rowid_filter;
-    rowid_filter= 0;
-  }
+    clear_range_rowid_filter();
   if (cache)
   {
     cache->free();
@@ -15365,6 +15383,7 @@ double JOIN_TAB::get_examined_rows()
 
   TODO: consider moving this together with join_tab_execution_startup
 */
+
 bool JOIN_TAB::preread_init()
 {
   TABLE_LIST *derived= table->pos_in_table_list;
@@ -15406,7 +15425,31 @@ bool JOIN_TAB::preread_init()
 }
 
 
-bool JOIN_TAB::pfs_batch_update(JOIN *join)
+/**
+  pfs_batch_update()
+
+  Check if the used table will do a lot of read calls in a row without
+  any intervening read calls to any other tables.
+
+  @return 0  No
+  @return 1  Yes
+
+  If yes, then the handler will be informed about this with the
+  start_psi_batch_mode() / end_psi_batch_mode() calls
+
+  This is currently used only to speed up performance schema code for
+  multiple reads.
+
+  In the future we may also inform the engine about this.  The engine
+  could use this information to cache the used pages, keep blocks
+  locked in the page cache and similar things to speed up repeated
+  reads.
+
+  The return value of this function is cached in
+  JOIN_TAB::cached_pfs_batch_update
+*/
+
+bool JOIN_TAB::pfs_batch_update()
 {
   /*
     Use PFS batch mode if
@@ -22120,6 +22163,7 @@ do_select(JOIN *join, Procedure *procedure)
 {
   int rc= 0;
   enum_nested_loop_state error= NESTED_LOOP_OK;
+  uint top_level_tables= join->exec_join_tab_cnt();
   DBUG_ENTER("do_select");
 
   if (join->pushdown_query)
@@ -22136,8 +22180,9 @@ do_select(JOIN *join, Procedure *procedure)
 
     if (join->pushdown_query->store_data_in_temp_table)
     {
-      JOIN_TAB *last_tab= join->join_tab + join->exec_join_tab_cnt();
+      JOIN_TAB *last_tab= join->join_tab + top_level_tables;
       last_tab->next_select= end_send;
+      last_tab->cached_pfs_batch_update= last_tab->pfs_batch_update();
 
       enum_nested_loop_state state= last_tab->aggr->end_send();
       if (state >= NESTED_LOOP_OK)
@@ -22154,6 +22199,7 @@ do_select(JOIN *join, Procedure *procedure)
   
   join->procedure= procedure;
   join->duplicate_rows= join->send_records=0;
+
   if (join->only_const_tables() && !join->need_tmp)
   {
     Next_select_func end_select= setup_end_select_func(join, NULL);
@@ -22226,6 +22272,17 @@ do_select(JOIN *join, Procedure *procedure)
                           dbug_serve_apcs(join->thd, 1);
                    );
 
+  /*
+    We have to update the cached_pfs_batch_update as
+    join_tab->select_cond may have changed.
+
+    This can happen in case of group by where some sub queries are not
+    needed anymore.  This is checked by main.ps
+  */
+    if (top_level_tables)
+      join->join_tab[top_level_tables-1].cached_pfs_batch_update=
+        join->join_tab[top_level_tables-1].pfs_batch_update();
+
     JOIN_TAB *join_tab= join->join_tab +
                         (join->tables_list ? join->const_tables : 0);
     if (join->outer_ref_cond && !join->outer_ref_cond->val_int())
@@ -22617,6 +22674,8 @@ sub_select_cache(JOIN *join, JOIN_TAB *join_tab, bool end_of_records)
 enum_nested_loop_state
 sub_select(JOIN *join,JOIN_TAB *join_tab,bool end_of_records)
 {
+  int error;
+  enum_nested_loop_state rc;
   DBUG_ENTER("sub_select");
 
   if (join_tab->last_inner)
@@ -22636,10 +22695,7 @@ sub_select(JOIN *join,JOIN_TAB *join_tab,bool end_of_records)
   }
   join_tab->tracker->r_scans++;
 
-  int error;
-  enum_nested_loop_state rc= NESTED_LOOP_OK;
-  READ_RECORD *info= &join_tab->read_record;
-
+  rc= NESTED_LOOP_OK;
 
   for (SJ_TMP_TABLE *flush_dups_table= join_tab->flush_weedout_table;
        flush_dups_table;
@@ -22651,9 +22707,21 @@ sub_select(JOIN *join,JOIN_TAB *join_tab,bool end_of_records)
   if (!join_tab->preread_init_done && join_tab->preread_init())
     DBUG_RETURN(NESTED_LOOP_ERROR);
 
-  join_tab->build_range_rowid_filter_if_needed();
+  if (unlikely(join_tab->rowid_filter))
+  {
+    if (unlikely(join_tab->need_to_build_rowid_filter))
+    {
+      join_tab->build_range_rowid_filter();
+      /*
+        We have to check join_tab->rowid_filter again as the above
+        function may have cleared it in case of errors.
+      */
       if (join_tab->rowid_filter && join_tab->rowid_filter->is_empty())
         rc= NESTED_LOOP_NO_MORE_ROWS;
+    }
+    else if (join_tab->rowid_filter->is_empty())
+      rc= NESTED_LOOP_NO_MORE_ROWS;
+  }
 
   join->return_tab= join_tab;
 
@@ -22679,8 +22747,8 @@ sub_select(JOIN *join,JOIN_TAB *join_tab,bool end_of_records)
   if (join_tab->loosescan_match_tab)
     join_tab->loosescan_match_tab->found_match= FALSE;
 
-  const bool pfs_batch_update= join_tab->pfs_batch_update(join);
-  if (pfs_batch_update)
+  DBUG_ASSERT(join_tab->cached_pfs_batch_update == join_tab->pfs_batch_update());
+  if (join_tab->cached_pfs_batch_update)
     join_tab->table->file->start_psi_batch_mode();
 
   if (rc != NESTED_LOOP_NO_MORE_ROWS)
@@ -22691,11 +22759,9 @@ sub_select(JOIN *join,JOIN_TAB *join_tab,bool end_of_records)
     rc= evaluate_join_record(join, join_tab, error);
   }
 
-  /* 
-    Note: psergey has added the 2nd part of the following condition; the 
-    change should probably be made in 5.1, too.
-  */
   bool skip_over= FALSE;
+  READ_RECORD *info= &join_tab->read_record;
+
   while (rc == NESTED_LOOP_OK && join->return_tab >= join_tab)
   {
     if (join_tab->loosescan_match_tab && 
@@ -22730,15 +22796,21 @@ sub_select(JOIN *join,JOIN_TAB *join_tab,bool end_of_records)
     rc= evaluate_join_record(join, join_tab, error);
   }
 
-  if (rc == NESTED_LOOP_NO_MORE_ROWS &&
-      join_tab->last_inner && !join_tab->found)
+  if (rc == NESTED_LOOP_NO_MORE_ROWS)
+  {
+    if (join_tab->last_inner && !join_tab->found)
+    {
       rc= evaluate_null_complemented_join_record(join, join_tab);
+      if (rc == NESTED_LOOP_NO_MORE_ROWS)
+        rc= NESTED_LOOP_OK;
+    }
+    else
+      rc= NESTED_LOOP_OK;
+  }
 
-  if (pfs_batch_update)
+  if (join_tab->cached_pfs_batch_update)
     join_tab->table->file->end_psi_batch_mode();
 
-  if (rc == NESTED_LOOP_NO_MORE_ROWS)
-    rc= NESTED_LOOP_OK;
   DBUG_RETURN(rc);
 }
 
@@ -22765,7 +22837,6 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab,
   ha_rows found_records=join->found_records;
   COND *select_cond= join_tab->select_cond;
   bool select_cond_result= TRUE;
-
   DBUG_ENTER("evaluate_join_record");
   DBUG_PRINT("enter",
              ("evaluate_join_record join: %p  join_tab: %p  "
@@ -22793,7 +22864,7 @@ evaluate_join_record(JOIN *join, JOIN_TAB *join_tab,
       DBUG_RETURN(NESTED_LOOP_ERROR);
   }
 
-  if (!select_cond || select_cond_result)
+  if (select_cond_result)
   {
     /*
       There is no select condition or the attached pushed down
@@ -23643,7 +23714,8 @@ int join_init_read_record(JOIN_TAB *tab)
     need_unpacking= tbl ? tbl->is_sjm_scan_table() : FALSE;
   }
 
-  tab->build_range_rowid_filter_if_needed();
+  if (tab->need_to_build_rowid_filter)
+    tab->build_range_rowid_filter();
 
   if (tab->filesort && tab->sort_table())     // Sort table.
     return 1;
@@ -25770,8 +25842,9 @@ test_if_skip_sort_order(JOIN_TAB *tab,ORDER *order,ha_rows select_limit,
           select->quick= 0;          // Cleanup either reset to save_quick,
                                      // or 'delete save_quick'
         tab->index= best_key;
-        tab->read_first_record= order_direction > 0 ?
-                                join_read_first:join_read_last;
+        tab->read_first_record= (order_direction > 0 ?
+                                 join_read_first:
+                                 join_read_last);
         tab->type=JT_NEXT;           // Read with index_first(), index_next()
 
         /*
@@ -25780,11 +25853,7 @@ test_if_skip_sort_order(JOIN_TAB *tab,ORDER *order,ha_rows select_limit,
         */
         if (tab->rowid_filter &&
             table->file->is_clustering_key(tab->index))
-	{
-          tab->range_rowid_filter_info= 0;
-          delete tab->rowid_filter;
-          tab->rowid_filter= 0;
-        }
+          tab->clear_range_rowid_filter();
 
         if (tab->pre_idx_push_select_cond)
         {
@@ -25818,12 +25887,8 @@ test_if_skip_sort_order(JOIN_TAB *tab,ORDER *order,ha_rows select_limit,
         tab->use_quick=1;
         tab->ref.key= -1;
         tab->ref.key_parts=0;		// Don't use ref key.
-        tab->range_rowid_filter_info= 0;
         if (tab->rowid_filter)
-	{
-          delete tab->rowid_filter;
-          tab->rowid_filter= 0;
-        }
+          tab->clear_range_rowid_filter();
         tab->read_first_record= join_init_read_record;
         if (tab->is_using_loose_index_scan())
           tab->join->tmp_table_param.precomputed_group_by= TRUE;

sql/sql_select.h

@@ -423,6 +423,8 @@ typedef struct st_join_table {
   bool          cached_eq_ref_table,eq_ref_table;
   bool          shortcut_for_distinct;
   bool          sorted;
+  bool          cached_pfs_batch_update;
+
   /* 
     If it's not 0 the number stored this field indicates that the index
     scan has been chosen to access the table data and we expect to scan 
@@ -571,10 +573,11 @@ typedef struct st_join_table {
   Range_rowid_filter_cost_info *range_rowid_filter_info;
   /* Rowid filter to be used when joining this join table */
   Rowid_filter *rowid_filter;
-  /* Becomes true just after the used range filter has been built / filled */
-  bool is_rowid_filter_built;
+  /* True if the plan requires a rowid filter and it's not built yet */
+  bool need_to_build_rowid_filter;
 
-  void build_range_rowid_filter_if_needed();
+  void build_range_rowid_filter();
+  void clear_range_rowid_filter();
 
   void cleanup();
   inline bool is_using_loose_index_scan()
@@ -685,7 +688,7 @@ typedef struct st_join_table {
   double get_examined_rows();
   bool preread_init();
 
-  bool pfs_batch_update(JOIN *join);
+  bool pfs_batch_update();
 
   bool is_sjm_nest() { return MY_TEST(bush_children); }