List:Commits« Previous MessageNext Message »
From:Sergey Vojtovich Date:February 3 2011 1:29pm
Subject:Re: bzr commit into mysql-5.5 branch (mattias.jonsson:3271)
Bug#59316
View as plain text  
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
Thread
bzr commit into mysql-5.5 branch (mattias.jonsson:3271) Bug#59316Mattias Jonsson24 Jan
  • Re: bzr commit into mysql-5.5 branch (mattias.jonsson:3271)Bug#59316Sergey Vojtovich3 Feb