Skip to content
Permalink
Browse files
MDEV-16334 Incorrect ALTER TABLE for changing column option 
commit 2dbeebd accidentally changed
ALTER_COLUMN_OPTION and ALTER_COLUMN_STORAGE_TYPE to be separate flags.
InnoDB and Mroonga are only checking for the latter;
the example storage engine is checking for the former only.

The impact of this bug should be incorrect operation of Mroonga when
the column options GROONGA_TYPE, FLAGS are changed.

InnoDB does not define any column options, only table options,
so the flag ALTER_COLUMN_OPTION should never have been set.

Also, remove the unused flag ALTER_DROP_HISTORICAL.
  • Loading branch information
@dr-m
dr-m committed May 30, 2018
1 parent c0f9771 commit 682e7b8
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 41 deletions.
54 sql/handler.h
View file
@@ -631,89 +631,87 @@ typedef ulonglong alter_table_operations;
ALTER_DROP_STORED_COLUMN)
#define ALTER_COLUMN_DEFAULT ALTER_CHANGE_COLUMN_DEFAULT

#define ALTER_DROP_HISTORICAL (1ULL << 35)

// Add non-unique, non-primary index
#define ALTER_ADD_NON_UNIQUE_NON_PRIM_INDEX (1ULL << 36)
#define ALTER_ADD_NON_UNIQUE_NON_PRIM_INDEX (1ULL << 35)

// Drop non-unique, non-primary index
#define ALTER_DROP_NON_UNIQUE_NON_PRIM_INDEX (1ULL << 37)
#define ALTER_DROP_NON_UNIQUE_NON_PRIM_INDEX (1ULL << 36)

// Add unique, non-primary index
#define ALTER_ADD_UNIQUE_INDEX (1ULL << 38)
#define ALTER_ADD_UNIQUE_INDEX (1ULL << 37)

// Drop unique, non-primary index
#define ALTER_DROP_UNIQUE_INDEX (1ULL << 39)
#define ALTER_DROP_UNIQUE_INDEX (1ULL << 38)

// Add primary index
#define ALTER_ADD_PK_INDEX (1ULL << 40)
#define ALTER_ADD_PK_INDEX (1ULL << 39)

// Drop primary index
#define ALTER_DROP_PK_INDEX (1ULL << 41)
#define ALTER_DROP_PK_INDEX (1ULL << 40)

// Virtual generated column
#define ALTER_ADD_VIRTUAL_COLUMN (1ULL << 42)
#define ALTER_ADD_VIRTUAL_COLUMN (1ULL << 41)
// Stored base (non-generated) column
#define ALTER_ADD_STORED_BASE_COLUMN (1ULL << 43)
#define ALTER_ADD_STORED_BASE_COLUMN (1ULL << 42)
// Stored generated column
#define ALTER_ADD_STORED_GENERATED_COLUMN (1ULL << 44)
#define ALTER_ADD_STORED_GENERATED_COLUMN (1ULL << 43)

// Drop column
#define ALTER_DROP_VIRTUAL_COLUMN (1ULL << 45)
#define ALTER_DROP_STORED_COLUMN (1ULL << 46)
#define ALTER_DROP_VIRTUAL_COLUMN (1ULL << 44)
#define ALTER_DROP_STORED_COLUMN (1ULL << 45)

// Rename column (verified; ALTER_RENAME_COLUMN may use original name)
#define ALTER_COLUMN_NAME (1ULL << 47)
#define ALTER_COLUMN_NAME (1ULL << 46)

// Change column datatype
#define ALTER_VIRTUAL_COLUMN_TYPE (1ULL << 48)
#define ALTER_STORED_COLUMN_TYPE (1ULL << 49)
#define ALTER_VIRTUAL_COLUMN_TYPE (1ULL << 47)
#define ALTER_STORED_COLUMN_TYPE (1ULL << 48)

/**
Change column datatype in such way that new type has compatible
packed representation with old type, so it is theoretically
possible to perform change by only updating data dictionary
without changing table rows.
*/
#define ALTER_COLUMN_EQUAL_PACK_LENGTH (1ULL << 50)
#define ALTER_COLUMN_EQUAL_PACK_LENGTH (1ULL << 49)

// Reorder column
#define ALTER_STORED_COLUMN_ORDER (1ULL << 51)
#define ALTER_STORED_COLUMN_ORDER (1ULL << 50)

// Reorder column
#define ALTER_VIRTUAL_COLUMN_ORDER (1ULL << 52)
#define ALTER_VIRTUAL_COLUMN_ORDER (1ULL << 51)

// Change column from NOT NULL to NULL
#define ALTER_COLUMN_NULLABLE (1ULL << 53)
#define ALTER_COLUMN_NULLABLE (1ULL << 52)

// Change column from NULL to NOT NULL
#define ALTER_COLUMN_NOT_NULLABLE (1ULL << 54)
#define ALTER_COLUMN_NOT_NULLABLE (1ULL << 53)

// Change column generation expression
#define ALTER_VIRTUAL_GCOL_EXPR (1ULL << 55)
#define ALTER_STORED_GCOL_EXPR (1ULL << 56)
#define ALTER_VIRTUAL_GCOL_EXPR (1ULL << 54)
#define ALTER_STORED_GCOL_EXPR (1ULL << 55)

// column's engine options changed, something in field->option_struct
#define ALTER_COLUMN_OPTION (1ULL << 57)
#define ALTER_COLUMN_OPTION (1ULL << 56)

// MySQL alias for the same thing:
#define ALTER_COLUMN_STORAGE_TYPE (1ULL << 58)
#define ALTER_COLUMN_STORAGE_TYPE ALTER_COLUMN_OPTION

// Change the column format of column
#define ALTER_COLUMN_COLUMN_FORMAT (1ULL << 59)
#define ALTER_COLUMN_COLUMN_FORMAT (1ULL << 57)

/**
Changes in generated columns that affect storage,
for example, when a vcol type or expression changes
and this vcol is indexed or used in a partitioning expression
*/
#define ALTER_COLUMN_VCOL (1ULL << 60)
#define ALTER_COLUMN_VCOL (1ULL << 58)

/**
ALTER TABLE for a partitioned table. The engine needs to commit
online alter of all partitions atomically (using group_commit_ctx)
*/
#define ALTER_PARTITIONED (1ULL << 61)
#define ALTER_PARTITIONED (1ULL << 59)

/*
Flags set in partition_flags when altering partitions
4 storage/innobase/handler/handler0alter.cc
View file
@@ -99,7 +99,6 @@ static const alter_table_operations INNOBASE_ALTER_DATA
/** Operations for altering a table that InnoDB does not care about */
static const alter_table_operations INNOBASE_INPLACE_IGNORE
= ALTER_COLUMN_DEFAULT
| ALTER_CHANGE_COLUMN_DEFAULT
| ALTER_PARTITIONED
| ALTER_COLUMN_COLUMN_FORMAT
| ALTER_COLUMN_STORAGE_TYPE
@@ -7205,8 +7204,7 @@ ha_innobase::inplace_alter_table(
ctx->add_index, ctx->add_key_numbers, ctx->num_to_add_index,
altered_table, ctx->defaults, ctx->col_map,
ctx->add_autoinc, ctx->sequence, ctx->skip_pk_sort,
ctx->m_stage, add_v, eval_table,
ha_alter_info->handler_flags & ALTER_DROP_HISTORICAL);
ctx->m_stage, add_v, eval_table);

#ifndef DBUG_OFF
oom:
4 storage/innobase/include/row0merge.h
View file
@@ -322,7 +322,6 @@ this function and it will be passed to other functions for further accounting.
@param[in] add_v new virtual columns added along with indexes
@param[in] eval_table mysql table used to evaluate virtual column
value, see innobase_get_computed_value().
@param[in] drop_historical whether to drop historical system rows
@return DB_SUCCESS or error code */
dberr_t
row_merge_build_indexes(
@@ -341,8 +340,7 @@ row_merge_build_indexes(
bool skip_pk_sort,
ut_stage_alter_t* stage,
const dict_add_v_col_t* add_v,
struct TABLE* eval_table,
bool drop_historical)
struct TABLE* eval_table)
MY_ATTRIBUTE((warn_unused_result));

/********************************************************************//**
11 storage/innobase/row/row0merge.cc
View file
@@ -1700,8 +1700,7 @@ row_merge_read_clustered_index(
ut_stage_alter_t* stage,
double pct_cost,
row_merge_block_t* crypt_block,
struct TABLE* eval_table,
bool drop_historical)
struct TABLE* eval_table)
{
dict_index_t* clust_index; /* Clustered index */
mem_heap_t* row_heap; /* Heap memory to create
@@ -2298,7 +2297,7 @@ row_merge_read_clustered_index(
}

if (old_table->versioned()) {
if ((!new_table->versioned() || drop_historical)
if (!new_table->versioned()
&& clust_index->vers_history_row(rec, offsets)) {
continue;
}
@@ -4548,7 +4547,6 @@ this function and it will be passed to other functions for further accounting.
@param[in] add_v new virtual columns added along with indexes
@param[in] eval_table mysql table used to evaluate virtual column
value, see innobase_get_computed_value().
@param[in] drop_historical whether to drop historical system rows
@return DB_SUCCESS or error code */
dberr_t
row_merge_build_indexes(
@@ -4567,8 +4565,7 @@ row_merge_build_indexes(
bool skip_pk_sort,
ut_stage_alter_t* stage,
const dict_add_v_col_t* add_v,
struct TABLE* eval_table,
bool drop_historical)
struct TABLE* eval_table)
{
merge_file_t* merge_files;
row_merge_block_t* block;
@@ -4732,7 +4729,7 @@ row_merge_build_indexes(
fts_sort_idx, psort_info, merge_files, key_numbers,
n_indexes, defaults, add_v, col_map, add_autoinc,
sequence, block, skip_pk_sort, &tmpfd, stage,
pct_cost, crypt_block, eval_table, drop_historical);
pct_cost, crypt_block, eval_table);

stage->end_phase_read_pk();

0 comments on commit 682e7b8

Please sign in to comment.