From: Sergey Vojtovich Date: February 3 2011 1:29pm Subject: Re: bzr commit into mysql-5.5 branch (mattias.jonsson:3271) Bug#59316 List-Archive: http://lists.mysql.com/commits/130316 Message-Id: <20110203132948.GA16346@june> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Hi Mattias, sorry for the delay - it came to be that I ended up with too much questions regarding this patch. Let's try to settle down API questions first. You suggest to extend API and make "is_clone_of" and "clone_mem_root" members of handler class. I can see that they're used only during ha_open(). I can see that they're used only by some specific handlers. Aren't these variables too excessive to be generic? What is lifetime of "is_clone_of"? As we do it part of handler class, I would expect valid value until handler object is destroyed or closed. It is probably true now. But I have feeling that it is too optimistic promise. We actually need valid value only during ::clone() call. You suggest that we move all logics from original::clone() to clone::open(). But that's totally different solution. It makes ::clone() useless. What were the reasons? This question is more about ref, MyISAM, MEMORY and MERGE. Though useless, ::clone() method is still preserved. And all it does it singals ::open() that it opens from cloned handler. Why not to overload ::open() instead? E.g.: /* Regular open */ ha_partition::open(const char *name, int mode, uint test_if_locked) { return open(name, mode, test_if_locked, NULL, NULL); } /* Open cloned */ ha_partition::open(handler *is_clone_of, MEM_ROOT *clone_mem_root) { return open(whatever, whatever, whatever, is_clone_of, clone_mem_root); } /* Common open */ ha_partition::open(const char *name, int mode, uint test_if_locked, handler *is_clone_of, MEM_ROOT *clone_mem_root) { ... } Regards, Sergey On Mon, Jan 24, 2011 at 03:47:33PM -0000, Mattias Jonsson wrote: > #At file:///Users/mattiasj/mysql-bzr/b59316-55_2/ based on revid:martin.hansson@stripped > > 3271 Mattias Jonsson 2011-01-24 > Bug#59316: Partitioning and index_merge memory leak > > When executing row-ordered-retrieval index merge, > the handler was cloned, but it used the wrong > memory root, so instead of allocating memory > on the thread/query's mem_root, it used the table's > mem_root, resulting in non released memory in the > table object, and was not freed until the table was > closed. > > Solution was to ensure that memory used during cloning > of a handler was allocated from the correct memory root. > @ sql/ha_partition.cc > Created a new ha_partition constructor for use when > cloning. > Also set the partitions handlers to be clones of the > original tables partitions. > Moved code from get_from_handler_file to a new function > read_handler_file. > @ sql/ha_partition.h > Moved is_clone variable to handler.h. > Added helper function get_handler_for_partition. > @ sql/handler.cc > Moved allocation of ref to ha_open, also for > clones. > @ sql/handler.h > Added is_clone_of and clone_mem_root. > For more generic handling of clones > @ storage/heap/ha_heap.cc > Removed ha_heap::clone(), using handler::clone() instead. > Moved clone specific code to ::open() instead. > @ storage/heap/ha_heap.h > Removed clone, added helper function hp_info(). > @ storage/myisam/ha_myisam.cc > Removed clone(), using handler::clone() instead. > Moved clone specific code to ::open() instead. > @ storage/myisam/ha_myisam.h > removed clone(). > @ storage/myisammrg/ha_myisammrg.cc > Moved is_cloned variable to handler::is_clone_of. > Removed ::clone(). > Moved clone specific code to ::open(). > @ storage/myisammrg/ha_myisammrg.h > Moved is_cloned to handler::is_clone_of. > removed ::clone(), using handler::clone instead. > > modified: > sql/ha_partition.cc > sql/ha_partition.h > sql/handler.cc > sql/handler.h > storage/heap/ha_heap.cc > storage/heap/ha_heap.h > storage/myisam/ha_myisam.cc > storage/myisam/ha_myisam.h > storage/myisammrg/ha_myisammrg.cc > storage/myisammrg/ha_myisammrg.h > === modified file 'sql/ha_partition.cc' > --- a/sql/ha_partition.cc 2010-12-17 20:58:40 +0000 > +++ b/sql/ha_partition.cc 2011-01-24 15:47:28 +0000 > @@ -150,15 +150,13 @@ static uint alter_table_flags(uint flags > > const uint ha_partition::NO_CURRENT_PART_ID= 0xFFFFFFFF; > > -/* > - Constructor method > +/** > + ha_partition constructor method > > - SYNOPSIS > - ha_partition() > - table Table object > + @param hton Handlerton (partition_hton) > + @param share Table share object > > - RETURN VALUE > - NONE > + @return New partition handler > */ > > ha_partition::ha_partition(handlerton *hton, TABLE_SHARE *share) > @@ -171,15 +169,40 @@ ha_partition::ha_partition(handlerton *h > } > > > -/* > - Constructor method > +/** > + ha_partition constructor method used by ha_partition::clone() > > - SYNOPSIS > - ha_partition() > - part_info Partition info > + @param hton Handlerton (partition_hton) > + @param share Table share object > + @param part_info_arg partition_info to use > + @param clone_arg ha_partition to clone > + @param clme_mem_root_arg MEM_ROOT to use > + > + @return New partition handler > +*/ > + > +ha_partition::ha_partition(handlerton *hton, TABLE_SHARE *share, > + partition_info *part_info_arg, > + handler *clone_arg, > + MEM_ROOT *clone_mem_root_arg) > + :handler(hton, share), m_part_info(part_info_arg), m_create_handler(TRUE), > + m_is_sub_partitioned(m_part_info->is_sub_partitioned()) > +{ > + DBUG_ENTER("ha_partition::ha_partition(clone)"); > + is_clone_of= clone_arg; > + clone_mem_root= clone_mem_root_arg; > + init_handler_variables(); > + DBUG_VOID_RETURN; > +} > > - RETURN VALUE > - NONE > + > +/** > + ha_partition constructor method > + > + @param hton Handlerton (partition_hton) > + @param part_info partition_info to use > + > + @return New partition handler > */ > > ha_partition::ha_partition(handlerton *hton, partition_info *part_info) > @@ -187,8 +210,8 @@ ha_partition::ha_partition(handlerton *h > m_is_sub_partitioned(m_part_info->is_sub_partitioned()) > { > DBUG_ENTER("ha_partition::ha_partition(part_info)"); > - init_handler_variables(); > DBUG_ASSERT(m_part_info); > + init_handler_variables(); > DBUG_VOID_RETURN; > } > > @@ -243,7 +266,6 @@ void ha_partition::init_handler_variable > m_rec0= 0; > m_curr_key_info[0]= NULL; > m_curr_key_info[1]= NULL; > - is_clone= FALSE, > m_part_func_monotonicity_info= NON_MONOTONIC; > auto_increment_lock= FALSE; > auto_increment_safe_stmt_log_lock= FALSE; > @@ -2142,6 +2164,7 @@ bool ha_partition::create_handler_file(c > DBUG_PRINT("info", ("table name = %s, num_parts = %u", name, > num_parts)); > tot_name_len= 0; > + /* Calculate the total storage needed for all partitioning names */ > for (i= 0; i < num_parts; i++) > { > part_elem= part_it++; > @@ -2191,7 +2214,7 @@ bool ha_partition::create_handler_file(c > tot_len_byte= 4 * tot_len_words; > if (!(file_buffer= (uchar *) my_malloc(tot_len_byte, MYF(MY_ZEROFILL)))) > DBUG_RETURN(TRUE); > - engine_array= (file_buffer + 12); > + engine_array= (file_buffer + PAR_FILE_DB_TYPE_OFFSET); > name_buffer_ptr= (char*) (file_buffer + ((4 + tot_partition_words) * 4)); > part_it.rewind(); > for (i= 0; i < num_parts; i++) > @@ -2231,7 +2254,8 @@ bool ha_partition::create_handler_file(c > chksum= 0; > int4store(file_buffer, tot_len_words); > int4store(file_buffer + 8, tot_parts); > - int4store(file_buffer + 12 + (tot_partition_words * 4), tot_name_len); > + int4store(file_buffer + PAR_FILE_DB_TYPE_OFFSET + (tot_partition_words * 4), > + tot_name_len); > for (i= 0; i < tot_len_words; i++) > chksum^= uint4korr(file_buffer + 4 * i); > int4store(file_buffer + 4, chksum); > @@ -2269,10 +2293,19 @@ void ha_partition::clear_handler_file() > { > if (m_engine_array) > plugin_unlock_list(NULL, m_engine_array, m_tot_parts); > - my_free(m_file_buffer); > - my_free(m_engine_array); > - m_file_buffer= NULL; > - m_engine_array= NULL; > + DBUG_PRINT("info", ("m_file_buffer %p m_name_buffer_ptr %p free+file->NULL", > + m_file_buffer, m_name_buffer_ptr)); > + if (m_file_buffer) > + { > + my_free(m_file_buffer); > + m_file_buffer= NULL; > + m_name_buffer_ptr= NULL; > + } > + if (m_engine_array) > + { > + my_free(m_engine_array); > + m_engine_array= NULL; > + } > } > > /* > @@ -2294,6 +2327,9 @@ bool ha_partition::create_handlers(MEM_R > handlerton *hton0; > DBUG_ENTER("create_handlers"); > > + DBUG_PRINT("info", ("mem_root %p thd: %p table: %p", > + mem_root, ha_thd()->mem_root, &table->mem_root)); > + DBUG_ASSERT(!is_clone_of); > if (!(m_file= (handler **) alloc_root(mem_root, alloc_len))) > DBUG_RETURN(TRUE); > m_file_tot_parts= m_tot_parts; > @@ -2301,8 +2337,7 @@ bool ha_partition::create_handlers(MEM_R > for (i= 0; i < m_tot_parts; i++) > { > handlerton *hton= plugin_data(m_engine_array[i], handlerton*); > - if (!(m_file[i]= get_new_handler(table_share, mem_root, > - hton))) > + if (!(m_file[i]= get_new_handler(table_share, mem_root, hton))) > DBUG_RETURN(TRUE); > DBUG_PRINT("info", ("engine_type: %u", hton->db_type)); > } > @@ -2342,6 +2377,8 @@ bool ha_partition::new_handlers_from_par > List_iterator_fast part_it(m_part_info->partitions); > DBUG_ENTER("ha_partition::new_handlers_from_part_info"); > > + DBUG_PRINT("info", ("mem_root %p thd: %p table: %p", > + mem_root, ha_thd()->mem_root, &table->mem_root)); > if (!(m_file= (handler **) alloc_root(mem_root, alloc_len))) > { > mem_alloc_error(alloc_len); > @@ -2364,20 +2401,38 @@ bool ha_partition::new_handlers_from_par > { > for (j= 0; j < m_part_info->num_subparts; j++) > { > - if (!(m_file[part_count++]= get_new_handler(table_share, mem_root, > - part_elem->engine_type))) > + if (!(m_file[part_count]= get_new_handler(table_share, mem_root, > + part_elem->engine_type))) > goto error; > + if (is_clone_of) > + { > + uint i= part_count; > + DBUG_ASSERT(clone_mem_root == mem_root); > + ha_partition *file= (ha_partition*) is_clone_of; > + m_file[i]->is_clone_of= file->get_handler_for_partition(i); > + m_file[i]->clone_mem_root= clone_mem_root; > + } > + part_count++; > DBUG_PRINT("info", ("engine_type: %u", > (uint) ha_legacy_type(part_elem->engine_type))); > } > } > else > { > - if (!(m_file[part_count++]= get_new_handler(table_share, mem_root, > - part_elem->engine_type))) > + if (!(m_file[part_count]= get_new_handler(table_share, mem_root, > + part_elem->engine_type))) > goto error; > DBUG_PRINT("info", ("engine_type: %u", > (uint) ha_legacy_type(part_elem->engine_type))); > + if (is_clone_of) > + { > + uint i= part_count; > + DBUG_ASSERT(clone_mem_root == mem_root); > + ha_partition *file= (ha_partition*) is_clone_of; > + m_file[i]->is_clone_of= file->get_handler_for_partition(i); > + m_file[i]->clone_mem_root= clone_mem_root; > + } > + part_count++; > } > } while (++i < m_part_info->num_parts); > if (part_elem->engine_type == myisam_hton) > @@ -2393,35 +2448,15 @@ error_end: > } > > > -/* > - Get info about partition engines and their names from the .par file > - > - SYNOPSIS > - get_from_handler_file() > - name Full path of table name > - mem_root Allocate memory through this > - > - RETURN VALUE > - TRUE Error > - FALSE Success > - > - DESCRIPTION > - Open handler file to get partition names, engine types and number of > - partitions. > -*/ > - > -bool ha_partition::get_from_handler_file(const char *name, MEM_ROOT *mem_root) > +bool ha_partition::read_handler_file(const char *name) > { > char buff[FN_REFLEN], *address_tot_name_len; > File file; > - char *file_buffer, *name_buffer_ptr; > - handlerton **engine_array; > + char *file_buffer; > uint i, len_bytes, len_words, tot_partition_words, tot_name_words, chksum; > - DBUG_ENTER("ha_partition::get_from_handler_file"); > + DBUG_ENTER("ha_partition::read_handler_file"); > DBUG_PRINT("enter", ("table name: '%s'", name)); > > - if (m_file_buffer) > - DBUG_RETURN(FALSE); > fn_format(buff, name, "", ha_par_ext, MY_APPEND_EXT); > > /* Following could be done with mysql_file_stat to read in whole file */ > @@ -2438,6 +2473,7 @@ bool ha_partition::get_from_handler_file > if (mysql_file_read(file, (uchar *) file_buffer, len_bytes, MYF(MY_NABP))) > goto err2; > > + (void) mysql_file_close(file, MYF(0)); > chksum= 0; > for (i= 0; i < len_words; i++) > chksum ^= uint4korr((file_buffer) + 4 * i); > @@ -2446,48 +2482,90 @@ bool ha_partition::get_from_handler_file > m_tot_parts= uint4korr((file_buffer) + 8); > DBUG_PRINT("info", ("No of parts = %u", m_tot_parts)); > tot_partition_words= (m_tot_parts + 3) / 4; > - engine_array= (handlerton **) my_alloca(m_tot_parts * sizeof(handlerton*)); > - for (i= 0; i < m_tot_parts; i++) > - { > - engine_array[i]= ha_resolve_by_legacy_type(ha_thd(), > - (enum legacy_db_type) > - *(uchar *) ((file_buffer) + > - 12 + i)); > - if (!engine_array[i]) > - goto err3; > - } > - address_tot_name_len= file_buffer + 12 + 4 * tot_partition_words; > + address_tot_name_len= file_buffer + PAR_FILE_DB_TYPE_OFFSET + > + 4 * tot_partition_words; > tot_name_words= (uint4korr(address_tot_name_len) + 3) / 4; > if (len_words != (tot_partition_words + tot_name_words + 4)) > - goto err3; > - name_buffer_ptr= file_buffer + 16 + 4 * tot_partition_words; > - (void) mysql_file_close(file, MYF(0)); > + goto err2; > + m_name_buffer_ptr= file_buffer + 16 + 4 * tot_partition_words; > m_file_buffer= file_buffer; // Will be freed in clear_handler_file() > - m_name_buffer_ptr= name_buffer_ptr; > - > - if (!(m_engine_array= (plugin_ref*) > - my_malloc(m_tot_parts * sizeof(plugin_ref), MYF(MY_WME)))) > - goto err3; > + DBUG_RETURN(FALSE); > +err2: > + my_free(file_buffer); > + DBUG_RETURN(TRUE); > +err1: > + (void) mysql_file_close(file, MYF(0)); > + DBUG_RETURN(TRUE); > +} > > - for (i= 0; i < m_tot_parts; i++) > - m_engine_array[i]= ha_lock_engine(NULL, engine_array[i]); > +/* > + Get info about partition engines and their names from the .par file > + > + SYNOPSIS > + get_from_handler_file() > + name Full path of table name > + mem_root Allocate memory through this > > - my_afree((gptr) engine_array); > + RETURN VALUE > + TRUE Error > + FALSE Success > + > + DESCRIPTION > + Open handler file to get partition names, engine types and number of > + partitions. > +*/ > +bool ha_partition::get_from_handler_file(const char *name, MEM_ROOT *mem_root) > +{ > + DBUG_ENTER("ha_partition::get_from_handler_file"); > + > + if (!m_file_buffer && !m_name_buffer_ptr) > + { > + if (read_handler_file(name)) > + DBUG_RETURN(TRUE); > + } > + DBUG_ASSERT(m_file_buffer && m_name_buffer_ptr); > + /* > + If m_file is already created, only the names was needed. This happens > + during ::clone() in ::open(). > + TODO: Save the partitions names in the table_share for easier access. > + */ > + if (m_file) > + DBUG_RETURN(FALSE); > + > + DBUG_ASSERT(!m_engine_array); > + if (!m_engine_array) > + { > + enum legacy_db_type first_db_type, db_type; > + uint i; > + handlerton *first_engine; > + first_db_type= (enum legacy_db_type) m_file_buffer[PAR_FILE_DB_TYPE_OFFSET]; > + first_engine= ha_resolve_by_legacy_type(ha_thd(), first_db_type); > + if (!first_engine) > + DBUG_RETURN(TRUE); > > - if (!m_file && create_handlers(mem_root)) > + if (!(m_engine_array= (plugin_ref*) > + my_malloc(m_tot_parts * sizeof(plugin_ref), MYF(MY_WME)))) > + DBUG_RETURN(TRUE); > + > + for (i= 0; i < m_tot_parts; i++) > + { > + db_type= (enum legacy_db_type) m_file_buffer[PAR_FILE_DB_TYPE_OFFSET + i]; > + if (db_type != first_db_type) > + { > + /* We have never allowed different engines */ > + DBUG_ASSERT(0); > + DBUG_RETURN(TRUE); > + } > + m_engine_array[i]= ha_lock_engine(NULL, first_engine); > + } > + } > + > + if (create_handlers(mem_root)) > { > clear_handler_file(); > DBUG_RETURN(TRUE); > } > DBUG_RETURN(FALSE); > - > -err3: > - my_afree((gptr) engine_array); > -err2: > - my_free(file_buffer); > -err1: > - (void) mysql_file_close(file, MYF(0)); > - DBUG_RETURN(TRUE); > } > > > @@ -2533,13 +2611,14 @@ void ha_data_partition_destroy(HA_DATA_P > > int ha_partition::open(const char *name, int mode, uint test_if_locked) > { > - char *name_buffer_ptr= m_name_buffer_ptr; > + char *name_buffer_ptr; > int error; > uint alloc_len; > handler **file; > char name_buff[FN_REFLEN]; > bool is_not_tmp_table= (table_share->tmp_table == NO_TMP_TABLE); > ulonglong check_table_flags= 0; > + MEM_ROOT *mem_root; > DBUG_ENTER("ha_partition::open"); > > DBUG_ASSERT(table->s == table_share); > @@ -2547,8 +2626,15 @@ int ha_partition::open(const char *name, > m_mode= mode; > m_open_test_lock= test_if_locked; > m_part_field_array= m_part_info->full_part_field_array; > - if (get_from_handler_file(name, &table->mem_root)) > + if (clone_mem_root) > + mem_root= clone_mem_root; > + else > + mem_root= &table->mem_root; > + /* Create handlers and/or partition names if not done before */ > + if (get_from_handler_file(name, mem_root)) > DBUG_RETURN(1); > + DBUG_ASSERT(m_file && m_name_buffer_ptr); > + name_buffer_ptr= m_name_buffer_ptr; > m_start_key.length= 0; > m_rec0= table->record[0]; > m_rec_length= table_share->reclength; > @@ -2584,7 +2670,7 @@ int ha_partition::open(const char *name, > DBUG_RETURN(1); > bitmap_clear_all(&m_bulk_insert_started); > /* Initialize the bitmap we use to determine what partitions are used */ > - if (!is_clone) > + if (!is_clone_of) > { > if (bitmap_init(&(m_part_info->used_partitions), NULL, m_tot_parts, TRUE)) > { > @@ -2691,7 +2777,7 @@ err_handler: > while (file-- != m_file) > (*file)->close(); > bitmap_free(&m_bulk_insert_started); > - if (!is_clone) > + if (!is_clone_of) > bitmap_free(&(m_part_info->used_partitions)); > > DBUG_RETURN(error); > @@ -2699,16 +2785,26 @@ err_handler: > > handler *ha_partition::clone(MEM_ROOT *mem_root) > { > - handler *new_handler= get_new_handler(table->s, mem_root, > - table->s->db_type()); > - ((ha_partition*)new_handler)->m_part_info= m_part_info; > - ((ha_partition*)new_handler)->is_clone= TRUE; > - if (new_handler && !new_handler->ha_open(table, > - table->s->normalized_path.str, > - table->db_stat, > - HA_OPEN_IGNORE_IF_LOCKED)) > - return new_handler; > - return NULL; > + ha_partition *file; > + > + DBUG_ENTER("ha_partition::clone"); > + > + file= new (mem_root) ha_partition(table->s->db_type(), table->s, > + m_part_info, this, mem_root); > + if (!file) > + DBUG_RETURN(NULL); > + > + if (file->initialize_partition(mem_root)) > + goto error; > + > + if (file->ha_open(table, table->s->normalized_path.str, table->db_stat, > + HA_OPEN_IGNORE_IF_LOCKED)) > + goto error; > + > + DBUG_RETURN(file); > +error: > + delete file; > + DBUG_RETURN(NULL); > } > > > @@ -2739,7 +2835,7 @@ int ha_partition::close(void) > DBUG_ASSERT(table->s == table_share); > delete_queue(&m_queue); > bitmap_free(&m_bulk_insert_started); > - if (!is_clone) > + if (!is_clone_of) > bitmap_free(&(m_part_info->used_partitions)); > file= m_file; > > > === modified file 'sql/ha_partition.h' > --- a/sql/ha_partition.h 2010-12-03 09:33:29 +0000 > +++ b/sql/ha_partition.h 2011-01-24 15:47:28 +0000 > @@ -31,6 +31,7 @@ enum partition_keywords > > > #define PARTITION_BYTES_IN_POS 2 > +#define PAR_FILE_DB_TYPE_OFFSET 12 > #define PARTITION_ENABLED_TABLE_FLAGS (HA_FILE_BASED | HA_REC_NOT_IN_SEQ) > #define PARTITION_DISABLED_TABLE_FLAGS (HA_CAN_GEOMETRY | \ > HA_CAN_FULLTEXT | \ > @@ -149,10 +150,10 @@ private: > THR_LOCK_DATA lock; /* MySQL lock */ > > /* > - TRUE <=> this object was created with ha_partition::clone and doesn't > - "own" the m_part_info structure. > + if handler::is_clone_of != NULL this object was created with > + ha_partition::clone and doesn't "own" the m_part_info structure. > */ > - bool is_clone; > + > bool auto_increment_lock; /**< lock reading/updating auto_inc */ > /** > Flag to keep the auto_increment lock through out the statement. > @@ -171,6 +172,12 @@ public: > m_part_info= part_info; > m_is_sub_partitioned= part_info->is_sub_partitioned(); > } > + handler *get_handler_for_partition(uint index) > + { > + if (!m_file || index >= m_tot_parts) > + return NULL; > + return m_file[index]; > + } > /* > ------------------------------------------------------------------------- > MODULE create/delete handler object > @@ -183,6 +190,9 @@ public: > ------------------------------------------------------------------------- > */ > ha_partition(handlerton *hton, TABLE_SHARE * table); > + ha_partition(handlerton *hton, TABLE_SHARE * table, > + partition_info *part_info_arg, > + handler *clone_arg, MEM_ROOT *clone_mem_root_arg); > ha_partition(handlerton *hton, partition_info * part_info); > ~ha_partition(); > /* > @@ -254,6 +264,7 @@ private: > And one method to read it in. > */ > bool create_handler_file(const char *name); > + bool read_handler_file(const char *name); > bool get_from_handler_file(const char *name, MEM_ROOT *mem_root); > bool new_handlers_from_part_info(MEM_ROOT *mem_root); > bool create_handlers(MEM_ROOT *mem_root); > > === modified file 'sql/handler.cc' > --- a/sql/handler.cc 2011-01-11 11:33:28 +0000 > +++ b/sql/handler.cc 2011-01-24 15:47:28 +0000 > @@ -2076,22 +2076,33 @@ int ha_delete_table(THD *thd, handlerton > /**************************************************************************** > ** General handler functions > ****************************************************************************/ > + > +/** > + Clone the handler > + @param mem_root MEM_ROOT to allocate memory on. > + > + @return Clone of handler or NULL > + > + @note The original handler must be opened and locked. > +*/ > + > handler *handler::clone(MEM_ROOT *mem_root) > { > - handler *new_handler= get_new_handler(table->s, mem_root, table->s->db_type()); > - /* > - Allocate handler->ref here because otherwise ha_open will allocate it > - on this->table->mem_root and we will not be able to reclaim that memory > - when the clone handler object is destroyed. > - */ > - if (!(new_handler->ref= (uchar*) alloc_root(mem_root, ALIGN_SIZE(ref_length)*2))) > + handler *new_handler; > + > + new_handler= get_new_handler(table->s, mem_root, table->s->db_type()); > + if (!new_handler) > + return NULL; > + new_handler->is_clone_of= this; > + new_handler->clone_mem_root= mem_root; > + if (new_handler->ha_open(table, table->s->normalized_path.str, > + table->db_stat, > + HA_OPEN_IGNORE_IF_LOCKED)) > + { > + delete new_handler; > return NULL; > - if (new_handler && !new_handler->ha_open(table, > - table->s->normalized_path.str, > - table->db_stat, > - HA_OPEN_IGNORE_IF_LOCKED)) > - return new_handler; > - return NULL; > + } > + return new_handler; > } > > > @@ -2154,13 +2165,18 @@ int handler::ha_open(TABLE *table_arg, c > } > else > { > + MEM_ROOT *mem_root; > if (table->s->db_options_in_use & HA_OPTION_READ_ONLY_DATA) > table->db_stat|=HA_READ_ONLY; > (void) extra(HA_EXTRA_NO_READCHECK); // Not needed in SQL > > - /* ref is already allocated for us if we're called from handler::clone() */ > - if (!ref && !(ref= (uchar*) alloc_root(&table->mem_root, > - ALIGN_SIZE(ref_length)*2))) > + /* Use the mem_root given in ::clone() or the tables mem_root */ > + if (clone_mem_root) > + mem_root= clone_mem_root; > + else > + mem_root= &table->mem_root; > + if (!ref && > + !(ref= (uchar*) alloc_root(mem_root, ALIGN_SIZE(ref_length)*2))) > { > close(); > error=HA_ERR_OUT_OF_MEM; > > === modified file 'sql/handler.h' > --- a/sql/handler.h 2010-11-05 11:01:10 +0000 > +++ b/sql/handler.h 2011-01-24 15:47:28 +0000 > @@ -1237,6 +1237,9 @@ public: > */ > PSI_table *m_psi; > > + handler *is_clone_of; > + MEM_ROOT *clone_mem_root; > + > handler(handlerton *ht_arg, TABLE_SHARE *share_arg) > :table_share(share_arg), table(0), > estimation_rows_to_insert(0), ht(ht_arg), > @@ -1246,7 +1249,7 @@ public: > locked(FALSE), implicit_emptied(0), > pushed_cond(0), next_insert_id(0), insert_id_for_cur_row(0), > auto_inc_intervals_count(0), > - m_psi(NULL) > + m_psi(NULL), is_clone_of(NULL), clone_mem_root(NULL) > {} > virtual ~handler(void) > { > > === modified file 'storage/heap/ha_heap.cc' > --- a/storage/heap/ha_heap.cc 2010-10-06 14:34:28 +0000 > +++ b/storage/heap/ha_heap.cc 2011-01-24 15:47:28 +0000 > @@ -101,6 +101,16 @@ const char **ha_heap::bas_ext() const > int ha_heap::open(const char *name, int mode, uint test_if_locked) > { > internal_table= test(test_if_locked & HA_OPEN_INTERNAL_TABLE); > + > + /* > + Use the original file->s->name instead of table->s->path if cloned. > + This is needed by Windows where the clone() call sees > + '/'-delimited path in table->s->path, while ha_heap::open() was called > + with '\'-delimited path. > + */ > + if (is_clone_of) > + name= ((ha_heap*)is_clone_of)->hp_info()->s->name; > + > if (internal_table || (!(file= heap_open(name, mode)) && my_errno == ENOENT)) > { > HP_CREATE_INFO create_info; > @@ -152,26 +162,6 @@ int ha_heap::close(void) > > > /* > - Create a copy of this table > - > - DESCRIPTION > - Do same as default implementation but use file->s->name instead of > - table->s->path. This is needed by Windows where the clone() call sees > - '/'-delimited path in table->s->path, while ha_peap::open() was called > - with '\'-delimited path. > -*/ > - > -handler *ha_heap::clone(MEM_ROOT *mem_root) > -{ > - handler *new_handler= get_new_handler(table->s, mem_root, table->s->db_type()); > - if (new_handler && !new_handler->ha_open(table, file->s->name, table->db_stat, > - HA_OPEN_IGNORE_IF_LOCKED)) > - return new_handler; > - return NULL; /* purecov: inspected */ > -} > - > - > -/* > Compute which keys to use for scanning > > SYNOPSIS > > === modified file 'storage/heap/ha_heap.h' > --- a/storage/heap/ha_heap.h 2010-10-06 14:34:28 +0000 > +++ b/storage/heap/ha_heap.h 2011-01-24 15:47:28 +0000 > @@ -35,7 +35,6 @@ class ha_heap: public handler > public: > ha_heap(handlerton *hton, TABLE_SHARE *table); > ~ha_heap() {} > - handler *clone(MEM_ROOT *mem_root); > const char *table_type() const > { > return (table->in_use->variables.sql_mode & MODE_MYSQL323) ? > @@ -118,6 +117,7 @@ public: > return memcmp(ref1, ref2, sizeof(HEAP_PTR)); > } > bool check_if_incompatible_data(HA_CREATE_INFO *info, uint table_changes); > + HP_INFO *hp_info() { return file; } > private: > void update_key_stats(); > }; > > === modified file 'storage/myisam/ha_myisam.cc' > --- a/storage/myisam/ha_myisam.cc 2011-01-11 09:07:37 +0000 > +++ b/storage/myisam/ha_myisam.cc 2011-01-24 15:47:28 +0000 > @@ -643,14 +643,6 @@ ha_myisam::ha_myisam(handlerton *hton, T > can_enable_indexes(1) > {} > > -handler *ha_myisam::clone(MEM_ROOT *mem_root) > -{ > - ha_myisam *new_handler= static_cast (handler::clone(mem_root)); > - if (new_handler) > - new_handler->file->state= file->state; > - return new_handler; > -} > - > > static const char *ha_myisam_exts[] = { > ".MYI", > @@ -746,6 +738,8 @@ int ha_myisam::open(const char *name, in > table->key_info[i].block_size= file->s->keyinfo[i].block_length; > } > my_errno= 0; > + if (is_clone_of) > + file->state= ((ha_myisam*)is_clone_of)->file_ptr()->state; > goto end; > err: > this->close(); > > === modified file 'storage/myisam/ha_myisam.h' > --- a/storage/myisam/ha_myisam.h 2010-10-06 14:34:28 +0000 > +++ b/storage/myisam/ha_myisam.h 2011-01-24 15:47:28 +0000 > @@ -50,7 +50,6 @@ class ha_myisam: public handler > public: > ha_myisam(handlerton *hton, TABLE_SHARE *table_arg); > ~ha_myisam() {} > - handler *clone(MEM_ROOT *mem_root); > const char *table_type() const { return "MyISAM"; } > const char *index_type(uint key_number); > const char **bas_ext() const; > > === modified file 'storage/myisammrg/ha_myisammrg.cc' > --- a/storage/myisammrg/ha_myisammrg.cc 2010-10-20 19:02:59 +0000 > +++ b/storage/myisammrg/ha_myisammrg.cc 2011-01-24 15:47:28 +0000 > @@ -118,7 +118,7 @@ static handler *myisammrg_create_handler > */ > > ha_myisammrg::ha_myisammrg(handlerton *hton, TABLE_SHARE *table_arg) > - :handler(hton, table_arg), file(0), is_cloned(0) > + :handler(hton, table_arg), file(0) > { > init_sql_alloc(&children_mem_root, > FN_REFLEN + ALLOC_ROOT_MIN_BLOCK_SIZE, 0); > @@ -371,8 +371,10 @@ int ha_myisammrg::open(const char *name, > my_errno= 0; > > /* retrieve children table list. */ > - if (is_cloned) > + if (is_clone_of) > { > + MYRG_TABLE *u_table,*newu_table; > + MYRG_INFO *clone_file= ((ha_myisammrg*)is_clone_of)->myrg_info(); > /* > Open and attaches the MyISAM tables,that are under the MERGE table > parent, on the MyISAM storage engine interface directly within the > @@ -390,6 +392,20 @@ int ha_myisammrg::open(const char *name, > file->children_attached= TRUE; > > info(HA_STATUS_NO_LOCK | HA_STATUS_VARIABLE | HA_STATUS_CONST); > + /* > + Iterate through the original child tables and > + copy the state into the cloned child tables. > + We need to do this because all the child tables > + can be involved in delete. > + */ > + newu_table= file->open_tables; > + for (u_table= clone_file->open_tables; > + u_table < clone_file->end_table; > + u_table++) > + { > + newu_table->table->state= u_table->table->state; > + newu_table++; > + } > } > else if (!(file= myrg_parent_open(name, myisammrg_parent_open_callback, this))) > { > @@ -677,56 +693,6 @@ CPP_UNNAMED_NS_END > > > /** > - Returns a cloned instance of the current handler. > - > - @return A cloned handler instance. > - */ > -handler *ha_myisammrg::clone(MEM_ROOT *mem_root) > -{ > - MYRG_TABLE *u_table,*newu_table; > - ha_myisammrg *new_handler= > - (ha_myisammrg*) get_new_handler(table->s, mem_root, table->s->db_type()); > - if (!new_handler) > - return NULL; > - > - /* Inform ha_myisammrg::open() that it is a cloned handler */ > - new_handler->is_cloned= TRUE; > - /* > - Allocate handler->ref here because otherwise ha_open will allocate it > - on this->table->mem_root and we will not be able to reclaim that memory > - when the clone handler object is destroyed. > - */ > - if (!(new_handler->ref= (uchar*) alloc_root(mem_root, ALIGN_SIZE(ref_length)*2))) > - { > - delete new_handler; > - return NULL; > - } > - > - if (new_handler->ha_open(table, table->s->normalized_path.str, table->db_stat, > - HA_OPEN_IGNORE_IF_LOCKED)) > - { > - delete new_handler; > - return NULL; > - } > - > - /* > - Iterate through the original child tables and > - copy the state into the cloned child tables. > - We need to do this because all the child tables > - can be involved in delete. > - */ > - newu_table= new_handler->file->open_tables; > - for (u_table= file->open_tables; u_table < file->end_table; u_table++) > - { > - newu_table->table->state= u_table->table->state; > - newu_table++; > - } > - > - return new_handler; > - } > - > - > -/** > Attach children to a MERGE table. > > @return status > @@ -1033,7 +999,7 @@ int ha_myisammrg::close(void) > There are cases where children are not explicitly detached before > close. detach_children() protects itself against double detach. > */ > - if (!is_cloned) > + if (!is_clone_of) > detach_children(); > > rc= myrg_close(file); > @@ -1394,7 +1360,7 @@ int ha_myisammrg::external_lock(THD *thd > If this handler instance has been cloned, we still must call > myrg_lock_database(). > */ > - if (is_cloned) > + if (is_clone_of) > return myrg_lock_database(file, lock_type); > return 0; > } > > === modified file 'storage/myisammrg/ha_myisammrg.h' > --- a/storage/myisammrg/ha_myisammrg.h 2010-10-06 14:34:28 +0000 > +++ b/storage/myisammrg/ha_myisammrg.h 2011-01-24 15:47:28 +0000 > @@ -70,7 +70,6 @@ public: > class ha_myisammrg: public handler > { > MYRG_INFO *file; > - my_bool is_cloned; /* This instance has been cloned */ > > public: > MEM_ROOT children_mem_root; /* mem root for children list */ > @@ -110,7 +109,6 @@ public: > int add_children_list(void); > int attach_children(void); > int detach_children(void); > - virtual handler *clone(MEM_ROOT *mem_root); > int close(void); > int write_row(uchar * buf); > int update_row(const uchar * old_data, uchar * new_data); > > > -- > MySQL Code Commits Mailing List > For list archives: http://lists.mysql.com/commits > To unsubscribe: http://lists.mysql.com/commits?unsub=svoj@stripped -- Sergey Vojtovich MySQL AB, Software Engineer Izhevsk, Russia, www.mysql.com