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@mariadb.org