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 secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp