Uploaded image for project: 'MariaDB Server'
  1. MariaDB Server
  2. MDEV-16288

ALTER TABLE…ALGORITHM=DEFAULT does not override alter_algorithm

Details

    Description

      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
      		 
      CREATE TABLE t(id INT PRIMARY KEY, u INT UNSIGNED NOT NULL UNIQUE)
      		 
      ENGINE=InnoDB;
      		 
      INSERT INTO t(id,u) VALUES(1,1),(2,2),(3,3);
      		 
       
      		 
      SET alter_algorithm=instant;
      		 
      ALTER TABLE t ADD COLUMN d DATETIME DEFAULT current_timestamp();
      		 
      --error ER_ALTER_OPERATION_NOT_SUPPORTED
      		 
      ALTER TABLE t DROP COLUMN u;
      		 
      --error ER_ALTER_OPERATION_NOT_SUPPORTED
      		 
      ALTER TABLE t DROP COLUMN u, ALGORITHM=NOCOPY;
      		 
      # this should succeed, but currently fails!
      		 
      ALTER TABLE t DROP COLUMN u, ALGORITHM=DEFAULT;
      		 
      DROP TABLE t;

      Attachments

        Issue Links

          is caused by

          Task - A task that needs to be done. MDEV-13134 Introduce ALTER TABLE attributes ALGORITHM=NOCOPY and ALGORITHM=INSTANT

          • Major - Major loss of function.
          • Closed
          relates to

          Bug - A problem which impairs or prevents the functions of the product. MDEV-18570 ALGORITHM=INSTANT or NOCOPY fails to fail on MyISAM tables

          • Major - Major loss of function.
          • Confirmed

          Bug - A problem which impairs or prevents the functions of the product. MDEV-21855 Document difference between DEFAULT and 'DEFAULT' when setting alter_algorithm or system_versioning_asof

          • Major - Major loss of function.
          • Closed

          Activity

            Ascending order - Click to sort in descending order
            marko 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;

            marko 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;
            thiru 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

            thiru 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
            thiru Thirunarayanan Balathandayuthapani added a comment -

            Patch is in bb-10.3-MDEV-16288

            thiru Thirunarayanan Balathandayuthapani added a comment - Patch is in bb-10.3- MDEV-16288
            bar 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.

            bar 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.
            bar 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.

            bar 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.
            bar Alexander Barkov added a comment -

            This patch is OK to push:
            https://github.com/MariaDB/server/commit/a37708ecea0d09787fc5e7b71b47279a2c8e4735

            bar Alexander Barkov added a comment - This patch is OK to push: https://github.com/MariaDB/server/commit/a37708ecea0d09787fc5e7b71b47279a2c8e4735

            People

              thiru Thirunarayanan Balathandayuthapani
              marko Marko Mäkelä
              Votes:
              0 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved:

                Git Integration

                  Error rendering 'com.xiplink.jira.git.jira_git_plugin:git-issue-webpanel'. Please contact your Jira administrators.