MDEV-26247 Clean up spider_fields

Spider gbh query rewrite should get table for fields in a simple way.
Add a method spider_fields::find_table that searches its table holders
to find table for a given field. This way we will be able to get rid
of the first pass during the gbh creation where field_chains and
field_holders are created.

We also check that the field belongs to a spider table while walking
through the query, so we could remove
all_query_fields_are_query_table_members(). However, this requires an
earlier creation of the table_holder so that tables are added before
checking. We do that, and in doing so, also decouple table_holder and
spider_fields

Remove unused methods and fields. Add comments.

storage/spider/spd_db_conn.cc

@@ -9167,19 +9167,20 @@ int spider_db_open_item_ident(
       } else {
         if (str)
         {
-          SPIDER_FIELD_CHAIN *field_chain = fields->get_next_field_chain();
-          SPIDER_FIELD_HOLDER *field_holder = field_chain->field_holder;
-          spider = field_holder->spider;
+          SPIDER_TABLE_HOLDER *table= fields->find_table(field);
+          /* If table or table->spider is NULL the GBH creation
+          would have been skipped the first pass (see below). */
+          spider = table->spider;
           share = spider->share;
           if ((error_num = share->dbton_share[dbton_id]->
             append_column_name_with_alias(str, field->field_index,
-            field_holder->alias->ptr(), field_holder->alias->length())))
+            table->alias->ptr(), table->alias->length())))
             DBUG_RETURN(error_num);
-        } else {
-          if ((error_num = fields->add_field(field)))
+        } else
         {
-            DBUG_RETURN(error_num);
-          }
+          SPIDER_TABLE_HOLDER *table= fields->find_table(field);
+          if (!table || !table->spider)
+            DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM);
         }
       }
     }
@@ -9298,21 +9299,20 @@ int spider_db_open_item_field(
         } else {
           if (str)
           {
-            SPIDER_FIELD_CHAIN *field_chain = fields->get_next_field_chain();
-            SPIDER_FIELD_HOLDER *field_holder = field_chain->field_holder;
-            spider = field_holder->spider;
+            SPIDER_TABLE_HOLDER *table= fields->find_table(field);
+            /* If table or table->spider is NULL the GBH creation
+            would have been skipped the first pass (see below). */
+            spider = table->spider;
             share = spider->share;
-            field = spider->field_exchange(field);
-            DBUG_ASSERT(field);
             if ((error_num = share->dbton_share[dbton_id]->
               append_column_name_with_alias(str, field->field_index,
-              field_holder->alias->ptr(), field_holder->alias->length())))
+              table->alias->ptr(), table->alias->length())))
               DBUG_RETURN(error_num);
-          } else {
-            if ((error_num = fields->add_field(field)))
+          } else
           {
-              DBUG_RETURN(error_num);
-            }
+            SPIDER_TABLE_HOLDER *table= fields->find_table(field);
+            if (!table || !table->spider)
+              DBUG_RETURN(ER_SPIDER_COND_SKIP_NUM);
           }
           DBUG_RETURN(0);
         }

storage/spider/spd_db_include.h

@@ -604,34 +604,26 @@ typedef struct spider_conn_holder
   spider_conn_holder *next;
 } SPIDER_CONN_HOLDER;
 
+/* Record information of a local (spider) table, for use of the spider
+group by handler. */
 typedef struct spider_table_holder
 {
   TABLE *table;
   ha_spider *spider;
+  /* alias of the table, in the form of tk, where k is the index of
+  the table from `query->from' indexed by next_local. */
   spider_string *alias;
 } SPIDER_TABLE_HOLDER;
 
-typedef struct spider_field_holder
-{
-  Field *field;
-  ha_spider *spider;
-  spider_string *alias;
-  spider_field_holder *next;
-} SPIDER_FIELD_HOLDER;
-
-typedef struct spider_field_chain
-{
-  spider_field_holder *field_holder;
-  spider_field_chain *next;
-} SPIDER_FIELD_CHAIN;
-
+/* For use of the spider group by handler. */
 class spider_fields
 {
   uint dbton_count;
   uint current_dbton_num;
   uint dbton_ids[SPIDER_DBTON_SIZE];
+  /* Number of tables in `query->from'. */
   uint table_count;
-  uint current_table_num;
+  /* All tables in `query->from', in the same order by next_local. */
   SPIDER_TABLE_HOLDER *table_holder;
   SPIDER_LINK_IDX_CHAIN *first_link_idx_chain;
   SPIDER_LINK_IDX_CHAIN *last_link_idx_chain;
@@ -640,13 +632,6 @@ class spider_fields
   SPIDER_CONN_HOLDER *first_conn_holder;
   SPIDER_CONN_HOLDER *last_conn_holder;
   SPIDER_CONN_HOLDER *current_conn_holder;
-  SPIDER_FIELD_HOLDER *first_field_holder;
-  SPIDER_FIELD_HOLDER *last_field_holder;
-  SPIDER_FIELD_HOLDER *current_field_holder;
-  SPIDER_FIELD_CHAIN *first_field_chain;
-  SPIDER_FIELD_CHAIN *last_field_chain;
-  SPIDER_FIELD_CHAIN *current_field_chain;
-  Field **first_field_ptr;
   Field **current_field_ptr;
 public:
   spider_fields();
@@ -702,24 +687,14 @@ class spider_fields
   void free_conn_holder(
     SPIDER_CONN_HOLDER *conn_holder_arg
   );
-  SPIDER_TABLE_HOLDER *add_table(
-    ha_spider *spider_arg
-  );
-  bool all_query_fields_are_query_table_members();
-  int create_table_holder(
+  SPIDER_TABLE_HOLDER *find_table(Field *field);
+  void set_table_holder(
+    SPIDER_TABLE_HOLDER *table_holder_arg,
     uint table_count_arg
   );
-  void set_pos_to_first_table_holder();
-  SPIDER_TABLE_HOLDER *get_next_table_holder();
+  SPIDER_TABLE_HOLDER *get_first_table_holder();
   SPIDER_TABLE_HOLDER *get_table_holder(TABLE *table);
   uint get_table_count();
-  int add_field(Field *field_arg);
-  SPIDER_FIELD_HOLDER *create_field_holder();
-  void set_pos_to_first_field_holder();
-  SPIDER_FIELD_HOLDER *get_next_field_holder();
-  SPIDER_FIELD_CHAIN *create_field_chain();
-  void set_pos_to_first_field_chain();
-  SPIDER_FIELD_CHAIN *get_next_field_chain();
   void set_field_ptr(Field **field_arg);
   Field **get_next_field_ptr();
   int ping_table_mon_from_table(

storage/spider/spd_db_mysql.cc

@@ -15639,8 +15639,7 @@ int spider_mbase_handler::append_from_and_tables_part(
     default:
       DBUG_RETURN(0);
   }
-  fields->set_pos_to_first_table_holder();
-  table_holder = fields->get_next_table_holder();
+  table_holder = fields->get_first_table_holder();
   table_list = table_holder->table->pos_in_table_list;
   error_num = spider_db_mbase_utility->append_from_and_tables(
     table_holder->spider, fields, str,

storage/spider/spd_db_mysql.h

@@ -676,8 +676,11 @@ class spider_mbase_share: public spider_db_share
   spider_string      *show_table_status;
   spider_string      *show_records;
   spider_string      *show_index;
+  /* The remote table names */
   spider_string      *table_names_str;
+  /* The remote db names */
   spider_string      *db_names_str;
+  /* fixme: this field looks useless */
   spider_string      *db_table_str;
 #ifdef SPIDER_HAS_HASH_VALUE_TYPE
   my_hash_value_type *db_table_str_hash_value;

storage/spider/spd_group_by_handler.cc

@@ -46,11 +46,9 @@ extern SPIDER_DBTON spider_dbton[SPIDER_DBTON_SIZE];
 
 spider_fields::spider_fields() :
   dbton_count(0), current_dbton_num(0),
-  table_count(0), current_table_num(0), table_holder(NULL),
+  table_count(0), table_holder(NULL),
   first_link_idx_chain(NULL), last_link_idx_chain(NULL), current_link_idx_chain(NULL),
-  first_conn_holder(NULL), last_conn_holder(NULL), current_conn_holder(NULL),
-  first_field_holder(NULL), last_field_holder(NULL), current_field_holder(NULL),
-  first_field_chain(NULL), last_field_chain(NULL), current_field_chain(NULL)
+  first_conn_holder(NULL), last_conn_holder(NULL), current_conn_holder(NULL)
 {
   DBUG_ENTER("spider_fields::spider_fields");
   DBUG_PRINT("info",("spider this=%p", this));
@@ -69,24 +67,6 @@ spider_fields::~spider_fields()
       spider_free(spider_current_trx, current_link_idx_chain, MYF(0));
     }
   }
-  if (first_field_chain)
-  {
-    while ((current_field_chain = first_field_chain))
-    {
-      first_field_chain = current_field_chain->next;
-      spider_free(spider_current_trx, current_field_chain, MYF(0));
-    }
-  }
-  if (first_field_holder)
-  {
-    while ((current_field_holder = first_field_holder))
-    {
-      first_field_holder = current_field_holder->next;
-      spider_free(spider_current_trx, current_field_holder, MYF(0));
-    }
-  }
-  if (table_holder)
-    spider_free(spider_current_trx, table_holder, MYF(0));
   if (first_conn_holder)
   {
     while ((current_conn_holder = first_conn_holder))
@@ -475,7 +455,6 @@ int spider_fields::get_ok_link_idx(
 
 void spider_fields::set_first_link_idx(
 ) {
-  SPIDER_TABLE_HOLDER *table_holder;
   SPIDER_LINK_IDX_HOLDER *link_idx_holder;
   SPIDER_LINK_IDX_CHAIN *link_idx_chain;
   uint dbton_id;
@@ -497,11 +476,10 @@ void spider_fields::set_first_link_idx(
     DBUG_ASSERT(link_idx_chain);
     set_pos_to_first_table_on_link_idx_chain(link_idx_chain);
 
-    set_pos_to_first_table_holder();
-    while ((table_holder = get_next_table_holder()))
+    for (uint i= 0; i < table_count; i++)
     {
       link_idx_holder = get_next_table_on_link_idx_chain(link_idx_chain);
-      spider = table_holder->spider;
+      spider = table_holder[i].spider;
       dbton_hdl = spider->dbton_handler[dbton_id];
       dbton_hdl->first_link_idx = link_idx_holder->link_idx;
     }
@@ -848,19 +826,17 @@ void spider_fields::free_conn_holder(
   DBUG_VOID_RETURN;
 }
 
-SPIDER_TABLE_HOLDER *spider_fields::add_table(
-  ha_spider *spider_arg
+/* Add the table associated with an ha_spider to a table_holder.
+Return the table_holder. */
+static SPIDER_TABLE_HOLDER *spider_add_table_holder(
+  ha_spider *spider_arg,
+  SPIDER_TABLE_HOLDER *table_holder
 ) {
   spider_string *str;
   uint length;
   char tmp_buf[SPIDER_SQL_INT_LEN + 2];
   SPIDER_TABLE_HOLDER *return_table_holder;
-  SPIDER_FIELD_HOLDER *field_holder;
-  TABLE *table = spider_arg->get_table();
-  Field *field;
   DBUG_ENTER("spider_fields::add_table");
-  DBUG_PRINT("info",("spider this=%p", this));
-  DBUG_PRINT("info",("spider table_count=%u", table_count));
   DBUG_PRINT("info",("spider idx_for_direct_join=%u",
     spider_arg->idx_for_direct_join));
   length = my_sprintf(tmp_buf, (tmp_buf, "t%u",
@@ -879,90 +855,49 @@ SPIDER_TABLE_HOLDER *spider_fields::add_table(
   return_table_holder->spider = spider_arg;
   return_table_holder->alias = str;
 
-  set_pos_to_first_field_holder();
-  while ((field_holder = get_next_field_holder()))
-  {
-    if (!field_holder->spider)
-    {
-      field = field_holder->field;
-      if (
-        field->field_index < table->s->fields &&
-        field == table->field[field->field_index]
-      ) {
-        field_holder->spider = spider_arg;
-        field_holder->alias = str;
-      }
-    }
-  }
   DBUG_RETURN(return_table_holder);
 }
 
-/**
-  Verify that all fields in the query are members of tables that are in the
-  query.
-
-  @return TRUE              All fields in the query are members of tables
-                            that are in the query.
-          FALSE             At least one field in the query is not a
-                            member of a table that is in the query.
-*/
-
-bool spider_fields::all_query_fields_are_query_table_members()
+/* Return the table that field belongs to, or NULL if none exists. */
+SPIDER_TABLE_HOLDER *spider_fields::find_table(Field *field)
 {
-  SPIDER_FIELD_HOLDER *field_holder;
-  DBUG_ENTER("spider_fields::all_query_fields_are_query_table_members");
-  DBUG_PRINT("info",("spider this=%p", this));
-
-  set_pos_to_first_field_holder();
-  while ((field_holder = get_next_field_holder()))
-  {
-    if (!field_holder->spider)
-    {
-      DBUG_PRINT("info", ("spider field is not a member of a query table"));
-      DBUG_RETURN(FALSE);
-    }
-  }
+  for (uint i = 0; i < table_count; i++)
+    if (field->table == table_holder[i].table)
+      return &table_holder[i];
+  return NULL;
+}
 
-  DBUG_RETURN(TRUE);
+void spider_fields::set_table_holder(SPIDER_TABLE_HOLDER *table_holder_arg,
+                                     uint table_count_arg)
+{
+  table_holder= table_holder_arg;
+  table_count= table_count_arg;
 }
 
-int spider_fields::create_table_holder(
+/* Allocate space for table_count_arg table holders. */
+static SPIDER_TABLE_HOLDER *spider_create_table_holder(
   uint table_count_arg
 ) {
-  DBUG_ENTER("spider_fields::create_table_holder");
-  DBUG_PRINT("info",("spider this=%p", this));
-  DBUG_ASSERT(!table_holder);
+  SPIDER_TABLE_HOLDER* table_holder;
+  DBUG_ENTER("spider_create_table_holder");
+  if (table_count_arg == 0)
+    DBUG_RETURN(0);
   table_holder = (SPIDER_TABLE_HOLDER *)
     spider_malloc(spider_current_trx, 249,
     table_count_arg * sizeof(SPIDER_TABLE_HOLDER),
     MYF(MY_WME | MY_ZEROFILL));
-  if (!table_holder)
-    DBUG_RETURN(HA_ERR_OUT_OF_MEM);
-  table_count = table_count_arg;
-  current_table_num = 0;
-  DBUG_RETURN(0);
-}
-
-void spider_fields::set_pos_to_first_table_holder(
-) {
-  DBUG_ENTER("spider_fields::set_pos_to_first_table_holder");
-  DBUG_PRINT("info",("spider this=%p", this));
-  current_table_num = 0;
-  DBUG_VOID_RETURN;
+  DBUG_RETURN(table_holder);
 }
 
-SPIDER_TABLE_HOLDER *spider_fields::get_next_table_holder(
-) {
-  SPIDER_TABLE_HOLDER *return_table_holder;
-  DBUG_ENTER("spider_fields::get_next_table_holder");
-  DBUG_PRINT("info",("spider this=%p", this));
-  if (current_table_num >= table_count)
-    DBUG_RETURN(NULL);
-  return_table_holder = &table_holder[current_table_num];
-  ++current_table_num;
-  DBUG_RETURN(return_table_holder);
+/* Return pointer to the first table holder. */
+SPIDER_TABLE_HOLDER *spider_fields::get_first_table_holder()
+{
+  DBUG_ENTER("spider_fields::get_first_spider");
+  DBUG_RETURN(table_holder);
 }
 
+/* Return the first table holder associated with a given table, or
+NULL if not found. */
 SPIDER_TABLE_HOLDER *spider_fields::get_table_holder(TABLE *table)
 {
   uint table_num;
@@ -982,117 +917,12 @@ uint spider_fields::get_table_count()
   DBUG_RETURN(table_count);
 }
 
-int spider_fields::add_field(
-  Field *field_arg
-) {
-  SPIDER_FIELD_HOLDER *field_holder;
-  SPIDER_FIELD_CHAIN *field_chain;
-  DBUG_ENTER("spider_fields::add_field");
-  DBUG_PRINT("info",("spider this=%p", this));
-  DBUG_PRINT("info",("spider field=%p", field_arg));
-  if (!first_field_holder)
-  {
-    field_holder = create_field_holder();
-    DBUG_PRINT("info",("spider field_holder=%p", field_holder));
-    if (!field_holder)
-      DBUG_RETURN(HA_ERR_OUT_OF_MEM);
-    field_holder->field = field_arg;
-    first_field_holder = field_holder;
-    last_field_holder = field_holder;
-  } else {
-    field_holder = first_field_holder;
-    do {
-      if (field_holder->field == field_arg)
-        break;
-    } while ((field_holder = field_holder->next));
-    if (!field_holder)
-    {
-      field_holder = create_field_holder();
-      DBUG_PRINT("info",("spider field_holder=%p", field_holder));
-      if (!field_holder)
-        DBUG_RETURN(HA_ERR_OUT_OF_MEM);
-      field_holder->field = field_arg;
-      last_field_holder->next = field_holder;
-      last_field_holder = field_holder;
-    }
-  }
-  field_chain = create_field_chain();
-  DBUG_PRINT("info",("spider field_chain=%p", field_chain));
-  if (!field_chain)
-    DBUG_RETURN(HA_ERR_OUT_OF_MEM);
-  field_chain->field_holder = field_holder;
-  if (!first_field_chain)
-  {
-    first_field_chain = field_chain;
-    last_field_chain = field_chain;
-  } else {
-    last_field_chain->next = field_chain;
-    last_field_chain = field_chain;
-  }
-  DBUG_RETURN(0);
-}
-
-SPIDER_FIELD_HOLDER *spider_fields::create_field_holder(
-) {
-  DBUG_ENTER("spider_fields::create_field_holder");
-  DBUG_PRINT("info",("spider this=%p", this));
-  DBUG_RETURN((SPIDER_FIELD_HOLDER *)
-    spider_malloc(spider_current_trx, 250, sizeof(SPIDER_FIELD_HOLDER),
-    MYF(MY_WME | MY_ZEROFILL)));
-}
-
-void spider_fields::set_pos_to_first_field_holder(
-) {
-  DBUG_ENTER("spider_fields::set_pos_to_first_field_holder");
-  DBUG_PRINT("info",("spider this=%p", this));
-  current_field_holder = first_field_holder;
-  DBUG_VOID_RETURN;
-}
-
-SPIDER_FIELD_HOLDER *spider_fields::get_next_field_holder(
-) {
-  SPIDER_FIELD_HOLDER *return_field_holder = current_field_holder;
-  DBUG_ENTER("spider_fields::get_next_field_holder");
-  DBUG_PRINT("info",("spider this=%p", this));
-  if (current_field_holder)
-    current_field_holder = current_field_holder->next;
-  DBUG_RETURN(return_field_holder);
-}
-
-SPIDER_FIELD_CHAIN *spider_fields::create_field_chain(
-) {
-  DBUG_ENTER("spider_fields::create_field_chain");
-  DBUG_PRINT("info",("spider this=%p", this));
-  DBUG_RETURN((SPIDER_FIELD_CHAIN *)
-    spider_malloc(spider_current_trx, 251, sizeof(SPIDER_FIELD_CHAIN),
-    MYF(MY_WME | MY_ZEROFILL)));
-}
-
-void spider_fields::set_pos_to_first_field_chain(
-) {
-  DBUG_ENTER("spider_fields::set_pos_to_first_field_chain");
-  DBUG_PRINT("info",("spider this=%p", this));
-  current_field_chain = first_field_chain;
-  DBUG_VOID_RETURN;
-}
-
-SPIDER_FIELD_CHAIN *spider_fields::get_next_field_chain(
-) {
-  SPIDER_FIELD_CHAIN *return_field_chain = current_field_chain;
-  DBUG_ENTER("spider_fields::get_next_field_chain");
-  DBUG_PRINT("info",("spider this=%p", this));
-  if (current_field_chain)
-    current_field_chain = current_field_chain->next;
-  DBUG_RETURN(return_field_chain);
-}
-
 void spider_fields::set_field_ptr(
   Field **field_arg
 ) {
   DBUG_ENTER("spider_fields::set_field_ptr");
   DBUG_PRINT("info",("spider this=%p", this));
   DBUG_PRINT("info",("spider field_ptr=%p", field_arg));
-  first_field_ptr = field_arg;
   current_field_ptr = field_arg;
   DBUG_VOID_RETURN;
 }
@@ -1115,15 +945,13 @@ int spider_fields::ping_table_mon_from_table(
   ha_spider *tmp_spider;
   SPIDER_SHARE *tmp_share;
   int tmp_link_idx;
-  SPIDER_TABLE_HOLDER *table_holder;
   SPIDER_LINK_IDX_HOLDER *link_idx_holder;
   DBUG_ENTER("spider_fields::ping_table_mon_from_table");
   set_pos_to_first_table_on_link_idx_chain(link_idx_chain);
-  set_pos_to_first_table_holder();
-  while ((table_holder = get_next_table_holder()))
+  for (uint i= 0; i < table_count; i++)
   {
     link_idx_holder = get_next_table_on_link_idx_chain(link_idx_chain);
-    tmp_spider = table_holder->spider;
+    tmp_spider = table_holder[i].spider;
     tmp_link_idx = link_idx_holder->link_idx;
     tmp_share = tmp_spider->share;
     if (tmp_share->monitoring_kind[tmp_link_idx])
@@ -1160,9 +988,7 @@ spider_group_by_handler::spider_group_by_handler(
   query(*query_arg), fields(fields_arg)
 {
   DBUG_ENTER("spider_group_by_handler::spider_group_by_handler");
-  fields->set_pos_to_first_table_holder();
-  SPIDER_TABLE_HOLDER *table_holder = fields->get_next_table_holder();
-  spider = table_holder->spider;
+  spider = fields->get_first_table_holder()->spider;
   trx = spider->trx;
   DBUG_VOID_RETURN;
 }
@@ -1170,6 +996,7 @@ spider_group_by_handler::spider_group_by_handler(
 spider_group_by_handler::~spider_group_by_handler()
 {
   DBUG_ENTER("spider_group_by_handler::~spider_group_by_handler");
+  spider_free(spider_current_trx, fields->get_first_table_holder(), MYF(0));
   delete fields;
   DBUG_VOID_RETURN;
 }
@@ -1286,7 +1113,6 @@ int spider_group_by_handler::init_scan()
   {
     dbton_hdl = spider->dbton_handler[dbton_id];
     result_list->direct_distinct = query.distinct;
-    fields->set_pos_to_first_field_chain();
     if ((error_num = dbton_hdl->reset_sql(SPIDER_SQL_TYPE_SELECT_SQL)))
     {
       DBUG_RETURN(error_num);
@@ -1596,7 +1422,8 @@ group_by_handler *spider_create_group_by_handler(
   bool keep_going;
   bool find_dbton = FALSE;
   spider_fields *fields = NULL, *fields_arg = NULL;
-  uint table_idx, dbton_id;
+  SPIDER_TABLE_HOLDER *table_holder;
+  uint table_idx, dbton_id, table_count= 0;
   long tgt_link_status;
   DBUG_ENTER("spider_create_group_by_handler");
 
@@ -1618,6 +1445,7 @@ group_by_handler *spider_create_group_by_handler(
     DBUG_PRINT("info",("spider from=%p", from));
     if (from->table->const_table)
       continue;
+    ++table_count;
     if (from->table->part_info)
     {
       DBUG_PRINT("info",("spider partition handler"));
@@ -1639,6 +1467,9 @@ group_by_handler *spider_create_group_by_handler(
   } while ((from = from->next_local));
 #endif
 
+  if (!(table_holder= spider_create_table_holder(table_count)))
+    DBUG_RETURN(NULL);
+
   table_idx = 0;
   from = query->from;
   while (from && from->table->const_table)
@@ -1648,7 +1479,7 @@ group_by_handler *spider_create_group_by_handler(
   if (!from)
   {
     /* all tables are const_table */
-    DBUG_RETURN(NULL);
+    goto skip_free_table_holder;
   }
 #if defined(PARTITION_HAS_GET_CHILD_HANDLERS)
   if (from->table->part_info)
@@ -1667,6 +1498,11 @@ group_by_handler *spider_create_group_by_handler(
   share = spider->share;
   spider->idx_for_direct_join = table_idx;
   ++table_idx;
+  if (!spider_add_table_holder(spider, table_holder))
+  {
+    DBUG_PRINT("info",("spider can not add a table"));
+    goto skip_free_table_holder;
+  }
   memset(dbton_bitmap, 0, spider_bitmap_size(SPIDER_DBTON_SIZE));
   for (roop_count = 0; roop_count < (int) share->use_dbton_count; ++roop_count)
   {
@@ -1699,6 +1535,11 @@ group_by_handler *spider_create_group_by_handler(
     share = spider->share;
     spider->idx_for_direct_join = table_idx;
     ++table_idx;
+    if (!spider_add_table_holder(spider, table_holder))
+    {
+      DBUG_PRINT("info",("spider can not add a table"));
+      goto skip_free_table_holder;
+    }
     memset(dbton_bitmap_tmp, 0, spider_bitmap_size(SPIDER_DBTON_SIZE));
     for (roop_count = 0; roop_count < (int) share->use_dbton_count; ++roop_count)
     {
@@ -1752,10 +1593,9 @@ group_by_handler *spider_create_group_by_handler(
       {
         fields_arg = new spider_fields();
         if (!fields_arg)
-        {
-          DBUG_RETURN(NULL);
-        }
+          goto skip_free_table_holder;
       }
+      fields_arg->set_table_holder(table_holder, table_count);
       keep_going = TRUE;
       it.init(*query->select);
       while ((item = it++))
@@ -1862,15 +1702,7 @@ group_by_handler *spider_create_group_by_handler(
     }
   }
   if (!find_dbton)
-  {
-    DBUG_RETURN(NULL);
-  }
-
-  if (fields->create_table_holder(table_idx))
-  {
-    delete fields;
-    DBUG_RETURN(NULL);
-  }
+    goto skip_free_table_holder;
 
   from = query->from;
   while (from->table->const_table)
@@ -1901,12 +1733,6 @@ group_by_handler *spider_create_group_by_handler(
   }
   DBUG_PRINT("info",("spider s->db=%s", from->table->s->db.str));
   DBUG_PRINT("info",("spider s->table_name=%s", from->table->s->table_name.str));
-  if (!fields->add_table(spider))
-  {
-    DBUG_PRINT("info",("spider can not add a table"));
-    delete fields;
-    DBUG_RETURN(NULL);
-  }
   for (
     roop_count = spider_conn_link_idx_next(share->link_statuses,
       spider->conn_link_idx, -1, share->link_count,
@@ -1920,10 +1746,7 @@ group_by_handler *spider_create_group_by_handler(
     {
       DBUG_PRINT("info",("spider direct_join does not support use_handler"));
       if (lock_mode)
-      {
-        delete fields;
-        DBUG_RETURN(NULL);
-      }
+        goto skip_free_fields;
       continue;
     }
     conn = spider->conns[roop_count];
@@ -1935,8 +1758,7 @@ group_by_handler *spider_create_group_by_handler(
       DBUG_PRINT("info",("spider direct_join does not support with lock tables yet"));
       if (lock_mode)
       {
-        delete fields;
-        DBUG_RETURN(NULL);
+        goto skip_free_fields;
       }
       continue;
     }
@@ -1944,20 +1766,17 @@ group_by_handler *spider_create_group_by_handler(
       share->access_balances[spider->conn_link_idx[roop_count]]))
     {
       DBUG_PRINT("info",("spider can not create conn_holder"));
-      delete fields;
-      DBUG_RETURN(NULL);
+      goto skip_free_fields;
     }
     if (fields->add_link_idx(conn->conn_holder_for_direct_join, spider, roop_count))
     {
       DBUG_PRINT("info",("spider can not create link_idx_holder"));
-      delete fields;
-      DBUG_RETURN(NULL);
+      goto skip_free_fields;
     }
   }
   if (!fields->has_conn_holder())
   {
-    delete fields;
-    DBUG_RETURN(NULL);
+    goto skip_free_fields;
   }
 
   while ((from = from->next_local))
@@ -1981,12 +1800,6 @@ group_by_handler *spider_create_group_by_handler(
     }
 #endif
     share = spider->share;
-    if (!fields->add_table(spider))
-    {
-      DBUG_PRINT("info",("spider can not add a table"));
-      delete fields;
-      DBUG_RETURN(NULL);
-    }
     DBUG_PRINT("info",("spider s->db=%s", from->table->s->db.str));
     DBUG_PRINT("info",("spider s->table_name=%s", from->table->s->table_name.str));
     for (
@@ -2003,10 +1816,7 @@ group_by_handler *spider_create_group_by_handler(
       {
         DBUG_PRINT("info",("spider direct_join does not support use_handler"));
         if (lock_mode)
-        {
-          delete fields;
-          DBUG_RETURN(NULL);
-        }
+          goto skip_free_fields;
         continue;
       }
       conn = spider->conns[roop_count];
@@ -2016,17 +1826,13 @@ group_by_handler *spider_create_group_by_handler(
         DBUG_PRINT("info",("spider connection %p can not be used for this query with locking",
           conn));
         if (lock_mode)
-        {
-          delete fields;
-          DBUG_RETURN(NULL);
-        }
+          goto skip_free_fields;
         continue;
       }
       if (fields->add_link_idx(conn->conn_holder_for_direct_join, spider, roop_count))
       {
         DBUG_PRINT("info",("spider can not create link_idx_holder"));
-        delete fields;
-        DBUG_RETURN(NULL);
+        goto skip_free_fields;
       }
     }
 
@@ -2035,30 +1841,20 @@ group_by_handler *spider_create_group_by_handler(
       if (lock_mode)
       {
         DBUG_PRINT("info",("spider some connections can not be used for this query with locking"));
-        delete fields;
-        DBUG_RETURN(NULL);
+        goto skip_free_fields;
       }
     }
     if (!fields->has_conn_holder())
     {
-      delete fields;
-      DBUG_RETURN(NULL);
+      goto skip_free_fields;
     }
   }
 
-  if (!fields->all_query_fields_are_query_table_members())
-  {
-    DBUG_PRINT("info", ("spider found a query field that is not a query table member"));
-    delete fields;
-    DBUG_RETURN(NULL);
-  }
-
   fields->check_support_dbton(dbton_bitmap);
   if (!fields->has_conn_holder())
   {
     DBUG_PRINT("info",("spider all chosen connections can't match dbton_id"));
-    delete fields;
-    DBUG_RETURN(NULL);
+    goto skip_free_fields;
   }
 
   /* choose a connection */
@@ -2070,16 +1866,14 @@ group_by_handler *spider_create_group_by_handler(
   if (fields->make_link_idx_chain(tgt_link_status))
   {
     DBUG_PRINT("info",("spider can not create link_idx_chain"));
-    delete fields;
-    DBUG_RETURN(NULL);
+    goto skip_free_fields;
   }
 
   /* choose link_id */
   if (fields->check_link_ok_chain())
   {
     DBUG_PRINT("info",("spider do not have link ok status"));
-    delete fields;
-    DBUG_RETURN(NULL);
+    goto skip_free_fields;
   }
 
   fields->set_first_link_idx();
@@ -2087,8 +1881,7 @@ group_by_handler *spider_create_group_by_handler(
   if (!(group_by_handler = new spider_group_by_handler(thd, query, fields)))
   {
     DBUG_PRINT("info",("spider can't create group_by_handler"));
-    delete fields;
-    DBUG_RETURN(NULL);
+    goto skip_free_fields;
   }
   query->distinct = FALSE;
   query->where = NULL;
@@ -2096,5 +1889,11 @@ group_by_handler *spider_create_group_by_handler(
   query->having = NULL;
   query->order_by = NULL;
   DBUG_RETURN(group_by_handler);
+
+skip_free_fields:
+  delete fields;
+skip_free_table_holder:
+  spider_free(spider_current_trx, table_holder, MYF(0));
+  DBUG_RETURN(NULL);
 }
 #endif