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 <partition_element> 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 <ha_myisam
> *>(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=1
--
Sergey Vojtovich <svoj@stripped>
MySQL AB, Software Engineer
Izhevsk, Russia, www.mysql.com
