1
0
mirror of https://github.com/MariaDB/server.git synced 2025-08-08 11:22:35 +03:00

MDEV-31957 Concurrent ALTER and ANALYZE collecting statistics can result in stale statistical data

Example of what causes the problem:
T1: ANALYZE TABLE starts to collect statistics
T2: ALTER TABLE starts by deleting statistics for all changed fields,
    then creates a temp table and copies data to it.
T1: ANALYZE ends and writes to the statistics tables.
T2: ALTER TABLE renames temp table in place of the old table.

Now the statistics from analyze matches the old deleted tables.

Fixed by waiting to delete old statistics until ALTER TABLE is
the only one using the old table and ensure that rename of columns
can handle swapping of column names.

rename_columns_in_stat_table() (former rename_column_in_stat_tables())
now takes a list of columns to rename. It uses the following algorithm
to update column_stats to be able to handle circular renames

- While there are columns to be renamed and it is the first loop or
  last rename loop did change something.
  - Loop over all columns to be renamed
    - Change column name in column_stat
      - If fail because of duplicate key
      - If this is first change attempt for this column
         - Change column name to a temporary column name
         - If there was a conflicting row, replace it with the current row.
    else
     - Remove entry from column list

- Loop over all remaining columns in the list
 - Remove the conflicting row
 - Change column from temporary name to final name in column_stat

Other things:
- Don't flush tables for every operation. Only flush when all updates
  are done.
- Rename of columns was not handled in case of ALGORITHM=copy (old bug).
  - Fixed that we do not collect statistics for hidden hash columns
    used by UNIQUE constraint on long values.
  - Fixed that we do not collect statistics for blob columns referred by
    generated virtual columns. This was achieved by storing the fields for
    which we want to have statistics in table->has_value_set instead of
    in table->read_set.
- Rename of indexes was not handled for persistent statistics.
  - This is now handled similar as rename of columns. Renamed columns
    are now stored in 'rename_stat_indexes' and handled in
    Alter_info::delete_statistics() together with drooped indexes.
- ALTER TABLE .. ADD INDEX may instead of creating a new index rename
  an existing generated foreign key index. This was not reflected in
  the index_stats table because this was handled in
  mysql_prepare_create_table instead instead of in the mysql_alter() code.
  Fixed by adding a call in mysql_prepare_create_table() to drop the
  changed index.
  I also had to change the code that 'marked the index' to be ignored
  with code that would not destroy the original index name.

Reviewer: Sergei Petrunia <sergey@mariadb.com>
This commit is contained in:
Monty
2023-08-18 18:35:02 +03:00
parent 388296a1e6
commit e3b36b8f1b
12 changed files with 1313 additions and 206 deletions

View File

@@ -2803,8 +2803,6 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
bool primary_key=0,unique_key=0;
Key *key, *key2;
uint tmp, key_number;
/* special marker for keys to be ignored */
static char ignore_key[1];
/* Calculate number of key segements */
*key_count= 0;
@@ -2852,17 +2850,17 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
both, is 'generated', and a generated key is a prefix of the other
key. Then we do not need the generated shorter key.
*/
if (key2->type != Key::FOREIGN_KEY && key2->name.str != ignore_key &&
if (key2->type != Key::FOREIGN_KEY && key2->type != Key::IGNORE_KEY &&
is_foreign_key_prefix(key, key2))
{
/* mark that the generated key should be ignored */
if (!key2->generated ||
(key->generated && key->columns.elements <
key2->columns.elements))
key->name.str= ignore_key;
key->type= Key::IGNORE_KEY;
else
{
key2->name.str= ignore_key;
key2->type= Key::IGNORE_KEY;
key_parts-= key2->columns.elements;
(*key_count)--;
}
@@ -2870,7 +2868,7 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
}
}
}
if (key->name.str != ignore_key)
if (key->type != Key::IGNORE_KEY)
key_parts+=key->columns.elements;
else
(*key_count)--;
@@ -2900,7 +2898,14 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
key_iterator.rewind();
while ((key=key_iterator++))
{
if (key->name.str == ignore_key || key->type == Key::FOREIGN_KEY)
if (key->type == Key::IGNORE_KEY)
{
/* The key was replaced by another key */
if (alter_info->add_stat_drop_index(thd, &key->name))
DBUG_RETURN(true);
continue;
}
if (key->type == Key::FOREIGN_KEY)
continue;
/* Create the key name based on the first column (if not given) */
if (key->type == Key::PRIMARY)
@@ -2962,12 +2967,12 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
Key_part_spec *column;
is_hash_field_needed= false;
if (key->name.str == ignore_key)
if (key->type == Key::IGNORE_KEY)
{
/* ignore redundant keys */
do
key=key_iterator++;
while (key && key->name.str == ignore_key);
while (key && key->type == Key::IGNORE_KEY);
if (!key)
break;
}
@@ -2995,6 +3000,9 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
case Key::FOREIGN_KEY:
key_number--; // Skip this key
continue;
case Key::IGNORE_KEY:
DBUG_ASSERT(0);
break;
default:
key_info->flags = HA_NOSAME;
break;
@@ -3198,6 +3206,8 @@ mysql_prepare_create_table(THD *thd, HA_CREATE_INFO *create_info,
key_add_part_check_null(file, key_info, sql_field, column))
DBUG_RETURN(TRUE);
break;
case Key::IGNORE_KEY:
break;
}
if (MTYP_TYPENR(sql_field->unireg_check) == Field::NEXT_NUMBER)
@@ -6445,7 +6455,9 @@ static bool fill_alter_inplace_info(THD *thd, TABLE *table, bool varchar,
if (table->s->tmp_table == NO_TMP_TABLE)
{
delete_statistics_for_column(thd, table, field);
if (alter_info->drop_stat_fields.push_back(field, thd->mem_root))
DBUG_RETURN(true);
KEY *key_info= table->key_info;
for (uint i= 0; i < table->s->keys; i++, key_info++)
{
@@ -6457,9 +6469,10 @@ static bool fill_alter_inplace_info(THD *thd, TABLE *table, bool varchar,
{
if (key_info->key_part[j].fieldnr - 1 == field->field_index)
{
delete_statistics_for_index(
thd, table, key_info,
j >= key_info->user_defined_key_parts);
if (alter_info->add_stat_drop_index(key_info,
j >= key_info->user_defined_key_parts,
thd->mem_root))
DBUG_RETURN(true);
break;
}
}
@@ -6516,13 +6529,17 @@ static bool fill_alter_inplace_info(THD *thd, TABLE *table, bool varchar,
ha_alter_info->handler_flags|= ALTER_STORED_COLUMN_TYPE;
}
/* Check if field was renamed (case-sensitive for detecting case change) */
/*
Check if field was renamed (case-sensitive for detecting case change)
*/
if (cmp(&field->field_name, &new_field->field_name))
{
field->flags|= FIELD_IS_RENAMED;
ha_alter_info->handler_flags|= ALTER_COLUMN_NAME;
rename_column_in_stat_tables(thd, table, field,
new_field->field_name.str);
if (alter_info->add_stat_rename_field(field,
&new_field->field_name,
thd->mem_root))
DBUG_RETURN(true);
}
/* Check that NULL behavior is same for old and new fields */
@@ -7481,6 +7498,8 @@ static bool mysql_inplace_alter_table(THD *thd,
in case of crash it should use the new one and log the query
to the binary log.
*/
ha_alter_info->alter_info->apply_statistics_deletes_renames(thd, table);
ddl_log_update_phase(ddl_log_state, DDL_ALTER_TABLE_PHASE_INPLACE_COPIED);
debug_crash_here("ddl_log_alter_after_log");
@@ -7585,6 +7604,7 @@ static bool mysql_inplace_alter_table(THD *thd,
DBUG_RETURN(true);
}
/**
maximum possible length for certain blob types.
@@ -7833,7 +7853,10 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
create_info->used_fields|=HA_CREATE_USED_AUTO;
}
if (table->s->tmp_table == NO_TMP_TABLE)
(void) delete_statistics_for_column(thd, table, field);
{
if (alter_info->drop_stat_fields.push_back(field, thd->mem_root))
DBUG_RETURN(true);
}
dropped_sys_vers_fields|= field->flags;
drop_it.remove();
dropped_fields= &table->tmp_set;
@@ -7845,13 +7868,19 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
{
vers_system_invisible= true;
}
/* invisible versioning column is dropped automatically on DROP SYSTEM VERSIONING */
/*
invisible versioning column is dropped automatically on
DROP SYSTEM VERSIONING
*/
if (!drop && field->invisible >= INVISIBLE_SYSTEM &&
field->flags & VERS_SYSTEM_FIELD &&
alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING)
{
if (table->s->tmp_table == NO_TMP_TABLE)
(void) delete_statistics_for_column(thd, table, field);
{
if (alter_info->drop_stat_fields.push_back(field, thd->mem_root))
DBUG_RETURN(true);
}
continue;
}
@@ -8214,19 +8243,24 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
{
if (table->s->tmp_table == NO_TMP_TABLE)
{
(void) delete_statistics_for_index(thd, table, key_info, FALSE);
if (alter_info->add_stat_drop_index(key_info, FALSE, thd->mem_root))
DBUG_RETURN(true);
if (primary_key)
{
KEY *tab_key_info= table->key_info;
for (uint j=0; j < table->s->keys; j++, tab_key_info++)
{
if (tab_key_info->user_defined_key_parts !=
if (tab_key_info != key_info &&
tab_key_info->user_defined_key_parts !=
tab_key_info->ext_key_parts)
(void) delete_statistics_for_index(thd, table, tab_key_info,
TRUE);
}
{
if (alter_info->add_stat_drop_index(tab_key_info, TRUE,
thd->mem_root))
DBUG_RETURN(true);
}
}
}
}
}
drop_it.remove();
continue;
}
@@ -8263,8 +8297,11 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
goto err;
}
key_name= rename_key->new_name.str;
key_name= rename_key->new_name.str; // New name of current key_info
rename_key_it.remove();
alter_info->add_stat_rename_index(key_info, &rename_key->new_name,
thd->mem_root);
/*
If the user has explicitly renamed the key, we should no longer
treat it as generated. Otherwise this key might be automatically
@@ -8374,10 +8411,17 @@ mysql_prepare_alter_table(THD *thd, TABLE *table,
if (table->s->tmp_table == NO_TMP_TABLE)
{
if (delete_index_stat)
(void) delete_statistics_for_index(thd, table, key_info, FALSE);
{
if (alter_info->add_stat_drop_index(key_info, FALSE, thd->mem_root))
DBUG_RETURN(true);
}
else if (alter_ctx->modified_primary_key &&
key_info->user_defined_key_parts != key_info->ext_key_parts)
(void) delete_statistics_for_index(thd, table, key_info, TRUE);
{
if (alter_info->add_stat_drop_index(key_info, FALSE,
thd->mem_root))
DBUG_RETURN(true);
}
}
if (!user_keyparts && key_parts.elements)
@@ -10196,6 +10240,13 @@ do_continue:;
alter_info->flags|= ALTER_INDEX_ORDER;
create_info->alias= alter_ctx.table_name;
thd->abort_on_warning= !ignore && thd->is_strict_mode();
/*
This is to be able to call Alter_info::add_stat_drop_index(thd, key_name)
from mysql_prepare_create_table()
*/
alter_info->original_table= table;
/*
Create the .frm file for the new table. Storage engine table will not be
created at this stage.
@@ -10466,6 +10517,16 @@ do_continue:;
thd->count_cuted_fields= CHECK_FIELD_WARN; // calc cuted fields
thd->cuted_fields=0L;
/*
Collect fields that was renamed.
We do not do that if fill_alter_inplace_info() has
already collected renamed fields.
*/
if (alter_info->flags & (ALTER_CHANGE_COLUMN | ALTER_RENAME_COLUMN) &&
alter_info->rename_stat_fields.is_empty())
if (alter_info->collect_renamed_fields(thd))
goto err_new_table_cleanup;
/*
We do not copy data for MERGE tables. Only the children have data.
MERGE tables have HA_NO_COPY_ON_ALTER set.
@@ -10662,6 +10723,9 @@ do_continue:;
if (wait_while_table_is_used(thd, table, HA_EXTRA_PREPARE_FOR_RENAME))
goto err_new_table_cleanup;
/* Now we are the only user. Update the data in EITS tables */
alter_info->apply_statistics_deletes_renames(thd, table);
close_all_tables_for_name(thd, table->s,
alter_ctx.is_table_renamed() ?
HA_EXTRA_PREPARE_FOR_RENAME: