maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexey!

That looked reasonable enough. I didn't find anything big.
Small comments - see below.

On Jun 01, Alexey Botchkov wrote:
> revision-id: 2603dc0d56ec90f308678a3c002f1b75af69f554 (mariadb-10.1.23-51-g2603dc0)
> parent(s): e4d10e09cf318aad237143254c45458d16009f70
> committer: Alexey Botchkov
> timestamp: 2017-06-01 17:33:34 +0400
> message:
> 
> MDEV-11084 Select statement with partition selection against MyISAM table opens all partitions.
> 
>         Now SELECT FROM t PARTITION(x) only opens the 'x' file.
>         The table->partition_names parameter is sent to ha_open
>         so it only opens the required partitions.
>         If the table is reopened, the
>         change_partition_to_open(partition_names) is called that
>         closes and opens partitions specified for the query.
> 
> diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
> index 29054c7..53a128c 100644
> --- a/sql/ha_partition.cc
> +++ b/sql/ha_partition.cc
> @@ -3481,12 +3498,16 @@ int ha_partition::open(const char *name, int mode, uint test_if_locked)
>        }
>        name_buffer_ptr+= strlen(name_buffer_ptr) + 1;
>      }
> +    file_sample= m_file[0];

are you sure that m_file[0] is opened?

>    }
>    else
>    {
>      file= m_file;
>      do
>      {
> +      if (!bitmap_is_set(&m_opened_partitions, file - m_file))
> +        continue;
> +
>        create_partition_name(name_buff, name, name_buffer_ptr, NORMAL_PART_NAME,
>                              FALSE);
>        table->s->connect_string = m_connect_string[(uint)(file-m_file)];
> @@ -3494,20 +3515,21 @@ int ha_partition::open(const char *name, int mode, uint test_if_locked)
>                                     test_if_locked | HA_OPEN_NO_PSI_CALL)))
>          goto err_handler;
>        bzero(&table->s->connect_string, sizeof(LEX_STRING));
> -      if (m_file == file)
> -        m_num_locks= (*file)->lock_count();
> +      if (!file_sample)
> +        m_num_locks= (file_sample= (*file))->lock_count();
>        DBUG_ASSERT(m_num_locks == (*file)->lock_count());
> -      name_buffer_ptr+= strlen(name_buffer_ptr) + 1;
> -    } while (*(++file));
> +    } while (name_buffer_ptr+= strlen(name_buffer_ptr) + 1, *(++file));

why?

>    }
>    
>    file= m_file;
> -  ref_length= (*file)->ref_length;
> -  check_table_flags= (((*file)->ha_table_flags() &
> +  ref_length= file_sample->ref_length;
> +  check_table_flags= ((file_sample->ha_table_flags() &
>                         ~(PARTITION_DISABLED_TABLE_FLAGS)) |
>                        (PARTITION_ENABLED_TABLE_FLAGS));
>    while (*(++file))
>    {
> +    if (!bitmap_is_set(&m_opened_partitions, file - m_file))
> +      continue;
>      /* MyISAM can have smaller ref_length for partitions with MAX_ROWS set */
>      set_if_bigger(ref_length, ((*file)->ref_length));
>      /*
> @@ -6782,6 +6809,63 @@ void ha_partition::get_dynamic_partition_info(PARTITION_STATS *stat_info,
>  }
>  
>  
> +void ha_partition::set_partitions_to_open(List<String> *partition_names)
> +{
> +  m_partitions_to_open= partition_names;
> +}
> +
> +
> +int ha_partition::change_partitions_to_open(List<String> *partition_names)
> +{
> +  char name_buff[FN_REFLEN];
> +  handler **file;
> +  char *name_buffer_ptr;
> +  int error= 0;
> +
> +  if (m_is_clone_of)
> +    return 0;
> +
> +  m_partitions_to_open= partition_names;
> +  if ((error= m_part_info->set_partition_bitmaps(partition_names)))
> +    goto err_handler;
> +
> +  if (bitmap_cmp(&m_opened_partitions, &m_part_info->read_partitions) != 0)
> +    return 0;
> +
> +  if ((error= read_par_file(table->s->normalized_path.str)))
> +    goto err_handler;
> +
> +  name_buffer_ptr= m_name_buffer_ptr;
> +  file= m_file;
> +  do
> +  {
> +    int n_file= file-m_file;
> +    int is_open= bitmap_is_set(&m_opened_partitions, n_file);
> +    int should_be_open= bitmap_is_set(&m_part_info->read_partitions, n_file);
> +
> +    if (is_open && !should_be_open)
> +      (*file)->ha_close();
> +    else if (!is_open && should_be_open)
> +    {
> +      create_partition_name(name_buff, table->s->normalized_path.str,
> +                            name_buffer_ptr, NORMAL_PART_NAME, FALSE);
> +      table->s->connect_string = m_connect_string[(uint)(file-m_file)];
> +      if ((error= (*file)->ha_open(table, name_buff, m_mode,
> +                                   m_open_test_lock | HA_OPEN_NO_PSI_CALL)))
> +        goto err_handler;
> +      bzero(&table->s->connect_string, sizeof(LEX_STRING));
> +    }
> +  } while (name_buffer_ptr+= strlen(name_buffer_ptr) + 1, *(++file));

Could you reuse that, instead of copying from ::open?
Perhaps ::open() can reset the bitmap to be empty and then
call ::change_partitions_to_open() ?

Or the common code could be moved into a third function that these both
will use...

> +  
> +  bitmap_copy(&m_opened_partitions, &m_part_info->read_partitions);
> +
> +  clear_handler_file();
> +
> +err_handler:
> +  return error;
> +}
> +
> +
>  /**
>    General function to prepare handler for certain behavior.
>  
> diff --git a/sql/partition_info.cc b/sql/partition_info.cc
> index bc0db9e..7fb8f70 100644
> --- a/sql/partition_info.cc
> +++ b/sql/partition_info.cc
> @@ -283,6 +281,27 @@ bool partition_info::set_partition_bitmaps(TABLE_LIST *table_list)
>  
>  
>  /**
> +  Set read/lock_partitions bitmap over non pruned partitions
> +
> +  @param table_list   Possible TABLE_LIST which can contain
> +                      list of partition names to query
> +
> +  @return Operation status
> +    @retval FALSE  OK
> +    @retval TRUE   Failed to allocate memory for bitmap or list of partitions
> +                   did not match
> +
> +  @note OK to call multiple times without the need for free_bitmaps.
> +*/
> +bool partition_info::set_partition_bitmaps_from_table(TABLE_LIST *table_list)
> +{
> +  List<String> *partition_names= table_list ?
> +                                   NULL : table_list->partition_names;
> +  return set_partition_bitmaps(partition_names);
> +}

This looks unused.

> +
> +
> +/**
>    Checks if possible to do prune partitions on insert.
>  
>    @param thd           Thread context
> diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> index 9c56b7d..2f9ec3e 100644
> --- a/sql/sql_base.cc
> +++ b/sql/sql_base.cc
> @@ -2533,6 +2533,12 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx)
>    {
>      DBUG_ASSERT(table->file != NULL);
>      MYSQL_REBIND_TABLE(table->file);
> +    if (table->part_info)
> +    {
> +      /* Set all [named] partitions as used. */
> +      if (table->file->change_partitions_to_open(table_list->partition_names))
> +        DBUG_RETURN(true);
> +    }
>    }
>    else
>    {
> @@ -2549,7 +2555,8 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx)
>                                           HA_TRY_READ_ONLY),
>                                   (READ_KEYINFO | COMPUTE_TYPES |
>                                    EXTRA_RECORD),
> -                                 thd->open_options, table, FALSE);
> +                                 thd->open_options, table, FALSE,
> +                                 table_list->partition_names);

Please, make sure you code compiles without partitioning too.
(in this case I wouldn't make the last argument of open_table_from_share
conditional, but I'd rather make table_list->partition_names unconditional).

>  
>      if (error)
>      {
> @@ -2598,9 +2605,11 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx)
>  #ifdef WITH_PARTITION_STORAGE_ENGINE
>    if (table->part_info)
>    {
> +#ifdef CODE_TO_REMOVE
>      /* Set all [named] partitions as used. */
>      if (table->part_info->set_partition_bitmaps(table_list))
>        DBUG_RETURN(true);
> +#endif /*CODE_TO_REMOVE*/

why not to remove at once?

>    }
>    else if (table_list->partition_names)
>    {

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx