Even if ALGORITHM=DEFAULT is explicitly specified in ALTER TABLE, the variable alter_algorithm will be substituted. That variable should only be substituted when no ALGORITHM has been specified.
--source include/have_innodb.inc
CREATETABLE t(id INTPRIMARYKEY, u INT UNSIGNED NOTNULLUNIQUE)
ENGINE=InnoDB;
INSERTINTO t(id,u) VALUES(1,1),(2,2),(3,3);
SET alter_algorithm=instant;
ALTERTABLE t ADDCOLUMN d DATETIME DEFAULTcurrent_timestamp();
createtable t(a intprimarykey, b int) engine=innodb;
insertinto t set a=1;
--enable_info
--echo # FIXME: This should succeed, and return "affected rows: 0"
--error ER_ALTER_OPERATION_NOT_SUPPORTED
altertable t dropcolumn b, algorithm=default;
--echo # FIXME: This should succeed, and return "affected rows: 1"
--error ER_ALTER_OPERATION_NOT_SUPPORTED
altertable t dropprimarykey, algorithm=default;
--disable_info
droptable t;
Marko Mäkelä
added a comment - Another test case:
--source include/have_innodb.inc
SET alter_algorithm=instant;
create table t(a int primary key , b int ) engine=innodb;
insert into t set a=1;
--enable_info
--echo # FIXME: This should succeed, and return "affected rows: 0"
--error ER_ALTER_OPERATION_NOT_SUPPORTED
alter table t drop column b, algorithm= default ;
--echo # FIXME: This should succeed, and return "affected rows: 1"
--error ER_ALTER_OPERATION_NOT_SUPPORTED
alter table t drop primary key , algorithm= default ;
--disable_info
drop table t;
Thirunarayanan Balathandayuthapani
added a comment - The following patch could solve the issue:
Set requested_algorithm to ALTER_TABLE_ALGORITHM_NONE when there is no mention of
algorithm in alter table.
mdev16288-10.3-61c0df94655f2dc3146456e49f3f51610251e79fv1.patch
Please review it
I'd only like you to add the entire two test cases from this report into the patch.
Thanks.
Alexander Barkov
added a comment - Hi Thiru,
The patch https://jira.mariadb.org/secure/attachment/51404/51404_mdev16288-10.3-61c0df94655f2dc3146456e49f3f51610251e79fv1.patch looks ok to push for me.
I'd only like you to add the entire two test cases from this report into the patch.
Thanks.
So you can use it in multiple points in the code, instead of duplicating the code.
I suggest to let Alter_info::requested_algorithm stay ALTER_TABLE_ALGORITHM_NONE. Let's encapsulate to make sure that the caller do not test the member directly and uses the method only.
Alexander Barkov
added a comment - - edited One more thought:
I feel slightly non-comfortable with this code duplication:
/* Use alter_algorithm when user didn't specify algorithm in alter */
if (requested_algorithm == Alter_info::ALTER_TABLE_ALGORITHM_NONE)
requested_algorithm = (Alter_info::enum_alter_table_algorithm) thd->variables.alter_algorithm;
if (alter_info->requested_algorithm ==
Alter_info::ALTER_TABLE_ALGORITHM_NONE)
{
alter_info->requested_algorithm=
(Alter_info::enum_alter_table_algorithm)
thd->variables.alter_algorithm;
}
Why not have a method:
enum_alter_table_algorithm Alter_info::algorithm(const THD *thd) const
{
if (requested_algorithm == ALTER_TABLE_ALGORITHM_NONE)
return (Alter_info::enum_alter_table_algorithm) thd->variables.alter_algorithm;
return requested_algorithm;
}
?
So you can use it in multiple points in the code, instead of duplicating the code.
I suggest to let Alter_info::requested_algorithm stay ALTER_TABLE_ALGORITHM_NONE. Let's encapsulate to make sure that the caller do not test the member directly and uses the method only.
Bug
Major
MDEV-13134 Introduce ALTER TABLE attributes ALGORITHM=NOCOPY and ALGORITHM=INSTANT
Another test case:
--source include/have_innodb.inc--enable_info--echo # FIXME: This should succeed, and return "affected rows: 0"--error ER_ALTER_OPERATION_NOT_SUPPORTED--echo # FIXME: This should succeed, and return "affected rows: 1"--error ER_ALTER_OPERATION_NOT_SUPPORTED--disable_info