maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita,

comments below refer to a combined diff of both commits

On May 23, Nikita Malyavin wrote:
> revision-id: c457f237511 (mariadb-11.0.1-124-gc457f237511)
> parent(s): f4b04ec534d
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2023-05-15 21:07:50 +0300
> message:
> 
> MDEV-30984 Online ALTER table is denied with non-informative error messages

Two main thoughts.

First, users will generally don't do show warnings after an error -
mariadb already puts some useful info into a warning after an error in
some (few) cases, and it's always a surprise when I write in a bug
report "use SHOW WARNINGS". It's not very intuitive. And in your case
it's even unnecessary, because your error message is

  LOCK=NONE is not supported. Reason: %s. Try LOCK=SHARED

and "Reason" part is perfectly suited for an explanation that you
generate, instead of a misleading "COPY algorithm requires a lock"
(because it doesn't).

Second thought, your messages are hard-coded and won't be translated
even if the user will configure the server to use, say, spanish or
Japanese error messages. See how it was solved for "COPY algorithm
requires a lock", ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY. But I
think the message will still be comprehensible if you'll simply print
the offending sql clause as the reason, this won't need any
localization. See below where I've shown what your error messages would
look like if you do that.

If you think that's too cryptic, then, please, use the
ER_ALTER_OPERATION_NOT_SUPPORTED_REASON_COPY approach.

Two more comments at the very end but I'll repeat them here, because
they're easy to overlook: AUTO_INCREMENT instead of AUTOINC, and a test
for DROP SYSTEM VERSIONING.

> diff --git a/mysql-test/main/alter_table_online.result b/mysql-test/main/alter_table_online.result
> index 8b726ccb591..bcf7afea9df 100644
> --- a/mysql-test/main/alter_table_online.result
> +++ b/mysql-test/main/alter_table_online.result
> @@ -4,6 +4,10 @@
>  create table t (a int);
>  alter ignore table t add primary key (a), algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: ALTER IGNORE. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	ALTER IGNORE TABLE is incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  drop table t;
>  #
>  # MDEV-28771 Assertion `table->in_use&&tdc->flushed' failed after ALTER
> @@ -92,12 +96,20 @@ on update cascade) engine=InnoDB;
>  insert into t2 values (1),(2),(3);
>  alter table t2 add c int, algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: FOREIGN KEY ... ON UPDATE CASCADE. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  alter table t2 add c int, algorithm=inplace, lock=none;
>  create or replace table t2 (b int, foreign key (b)
>  references t1 (a)
>  on delete set null) engine=InnoDB;
>  alter table t2 add c int, algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: FOREIGN KEY ... ON DELETE SET NULL. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  alter table t2 add c int, algorithm=inplace, lock=none;
>  create or replace table t2 (b int, foreign key (b)
>  references t1 (a)
> @@ -117,6 +129,10 @@ b int references t1 (b) on update cascade) engine=InnoDB;
>  insert into t2 values (1, 1),(2, 2);
>  alter table t2 add c int, algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: FOREIGN KEY ... ON DELETE SET NULL. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	Tables with CASCADE/SET NULL foreign keys are incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  alter table t2 add c int, algorithm=copy;
>  alter table t2 add d int, algorithm=inplace;
>  drop table t2, t1;
> @@ -132,6 +148,10 @@ period for system_time (row_start, row_end))
>  engine=innodb with system versioning;
>  alter table t1 add c int, algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: BIGINT GENERATED ALWAYS AS ROW_START. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	Transaction-versioned tables are incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  alter table t1 add c int, algorithm=inplace;
>  alter table t1 add d int, lock=none;
>  set system_versioning_alter_history= default;
> @@ -143,6 +163,10 @@ drop table t1;
>  create table t (a serial, b int) engine=innodb;
>  alter table t drop a, modify b serial, algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  set statement sql_mode= NO_AUTO_VALUE_ON_ZERO for
>  alter table t drop a, modify b serial, algorithm=copy, lock=none;
>  create or replace table t (a serial, b int) engine=innodb;
> @@ -157,6 +181,10 @@ t	CREATE TABLE `t` (
>  # Only unsafe approach is fine because of possible collisions. 
>  alter table t modify a int, modify b serial, algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  #
>  # Check that we treat autoinc columns correctly modify old autoinc is  
>  # fine, adding new autoinc for existed column is unsafe.
> @@ -180,22 +208,38 @@ t	CREATE TABLE `t` (
>  ) ENGINE=InnoDB DEFAULT CHARSET=latin1 COLLATE=latin1_swedish_ci
>  alter table t drop b, change c c serial, algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... SERIAL. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  # Check existed unique keys.
>  create or replace table t(a int, b int not null, c int not null, d int);
>  # No unique in the old table;
>  alter table t add unique(b, c), modify d int auto_increment, add key(d),
>  algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  alter table t add unique(a, b);
>  # Unique in the old table has nulls;
>  alter table t modify d int auto_increment, add key(d), 
>  algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  alter table t add unique(b, c);
>  # Change unique'scolumn;
>  alter table t change b x int, modify d int auto_increment, add key(d),
>  algorithm=copy, lock=none;
>  ERROR 0A000: LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED

ERROR 0A000: LOCK=NONE is not supported. Reason: MODIFY COLUMN ... AUTO_INCREMENT. Try LOCK=SHARED

> +show warnings;
> +Level	Code	Message
> +Note	1846	Adding AUTOINC to an existing column for a table without a primary key is incompatible with LOCK=NONE, ALGORITHM=COPY
> +Error	1846	LOCK=NONE is not supported. Reason: COPY algorithm requires a lock. Try LOCK=SHARED
>  # Finally good.
>  alter table t modify d int auto_increment, add key(d), 
>  algorithm=copy, lock=none;
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index 99570008ddb..22447db42af 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -9916,6 +9916,67 @@ bool online_alter_check_autoinc(const THD *thd, const Alter_info *alter_info,
>    return online;
>  }
>  
> +bool online_alter_is_supported(THD *thd, const Alter_info *alter_info,
> +                               const TABLE *table)
> +{
> +  const char *reason= NULL;
> +  List<FOREIGN_KEY_INFO> fk_list;
> +
> +  if (alter_info->flags & ALTER_DROP_SYSTEM_VERSIONING)
> +  {
> +    reason= "DROP SYSTEM VERSIONING is";

no test for that

> +    goto unsupported;
> +  }
> +
> +  if (thd->lex->ignore)
> +  {
> +    reason= "ALTER IGNORE TABLE is";
> +    goto unsupported;
> +  }
> +
> +  if (table->s->tmp_table)
> +  {
> +    reason= "Temporary tables are";
> +    goto unsupported;
> +  }
> +
> +  if (table->versioned(VERS_TRX_ID))
> +  {
> +    reason= "Transaction-versioned tables are";
> +    goto unsupported;
> +  }
> +
> +  table->file->get_foreign_key_list(thd, &fk_list);
> +  for (auto &fk: fk_list)
> +  {
> +    if (fk_modifies_child(fk.delete_method)
> +        || fk_modifies_child(fk.update_method))
> +    {
> +      reason= "Tables with CASCADE/SET NULL foreign keys are";
> +      goto unsupported;
> +    }
> +  }
> +
> +  if (!online_alter_check_autoinc(thd, alter_info, table))
> +  {
> +    reason= "Adding AUTOINC to an existing column for a table without a "

AUTO_INCREMENT, please, don't abbrev user visible messages

> +            "primary key is";
> +    goto unsupported;
> +  }
> +
> +  return true;
> +
> +unsupported:
> +  if (alter_info->requested_lock == Alter_info::ALTER_TABLE_LOCK_NONE
> +      && alter_info->algorithm(thd) == Alter_info::ALTER_TABLE_ALGORITHM_COPY)
> +    push_warning_printf(thd, Sql_condition::WARN_LEVEL_NOTE,
> +                        ER_ALTER_OPERATION_NOT_SUPPORTED_REASON,
> +                        "%s incompatible with "
> +                        "LOCK=NONE, ALGORITHM=COPY",
> +                        reason);
> +  return false;
> +}
> +
>  
>  /**
>    Alter table
 
Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx