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

ALTER TABLE…ALGORITHM=INPLACE fails with non-constant DEFAULT values

Details

    Description

      Apparently InnoDB doesn't copy blob data during online ALTER TABLE. In a meanwhile it calls innobase_rec_reset(), which may reallocate memory for blobs.

      It wasn't a problem before column compression was introduced, since in most (if not all) cases memory reallocation was not needed. With compressed columns reallocation is always there.

      Grep for MDEV-12586 in source code for relevant markers. Re-enable online ALTER TABLE and run main.column_compression test.

      Test not involving column compression:

      CREATE TABLE t1(a INT) ENGINE=InnoDB;
      INSERT INTO t1 VALUES(1),(2),(3);
      ALTER TABLE t1 ADD COLUMN b TEXT DEFAULT REPEAT("a", 5*a);
      SELECT * FROM t1;
      a	b
      1	NULL
      2	NULL
      3	NULL
      DROP TABLE t1;
      CREATE TABLE t1(a INT) ENGINE=MyISAM;
      INSERT INTO t1 VALUES(1),(2),(3);
      ALTER TABLE t1 ADD COLUMN b TEXT DEFAULT REPEAT("a", 5*a);
      SELECT * FROM t1;
      a	b
      1	aaaaa
      2	aaaaaaaaaa
      3	aaaaaaaaaaaaaaa
      DROP TABLE t1;
      

      Attachments

        Issue Links

          Activity

            For the record:
            ALTER TABLE…ALGORITHM=INPLACE only deals with TABLE::record[] for reporting duplicate key errors. The function innobase_rec_reset() is related to that (reset those columns to NULL for which we do not know the values).
            When a table is being rebuilt online, data stored off-page is copied from the old copy of the table. When creating secondary indexes on column prefixes, the necessary prefix of BLOB columns will be written to the sort buffers and online_log and ultimately be inserted into the index.

            marko Marko Mäkelä added a comment - For the record: ALTER TABLE…ALGORITHM=INPLACE only deals with TABLE::record[] for reporting duplicate key errors. The function innobase_rec_reset() is related to that (reset those columns to NULL for which we do not know the values). When a table is being rebuilt online, data stored off-page is copied from the old copy of the table. When creating secondary indexes on column prefixes, the necessary prefix of BLOB columns will be written to the sort buffers and online_log and ultimately be inserted into the index.

            The issue is not only with compressed fields, but with any fields that has a default that is not constant

            Here is an example for this:

            create or replace table t1 (a int auto_increment primary key, b int) engine=myisam;
            insert into t1 values (1,1),(2,2);
            ALTER TABLE t1 ADD COLUMN c int default b;
            select * from t1;

            Innodb result is:
            -----------

            a b c

            -----------

            1 1 NULL
            2 2 NULL

            -----------

            While MYISAM gives the right result:
            -----------

            a b c

            -----------

            1 1 1
            2 2 2

            -----------

            The problem is that InnoDB only calls set_default() only once, when it should call it for every row for fields with "field->default_value" set.

            monty Michael Widenius added a comment - The issue is not only with compressed fields, but with any fields that has a default that is not constant Here is an example for this: create or replace table t1 (a int auto_increment primary key, b int) engine=myisam; insert into t1 values (1,1),(2,2); ALTER TABLE t1 ADD COLUMN c int default b; select * from t1; Innodb result is: -- ---- ----- a b c -- ---- ----- 1 1 NULL 2 2 NULL -- ---- ----- While MYISAM gives the right result: -- ---- ----- a b c -- ---- ----- 1 1 1 2 2 2 -- ---- ----- The problem is that InnoDB only calls set_default() only once, when it should call it for every row for fields with "field->default_value" set.

            I think that we need a quick solution before the 10.2 GA release: simply refuse ALGORITHM=INPLACE if there are any non-constant DEFAULT values for added columns.

            Fixing this is somewhat tricky, because InnoDB would need to convert each row to MySQL format for evaluating the non-constant default values. Currently InnoDB uses a ‘default record’ of constant values that it will substitute for added columns.
            When fixing this properly, we could at the same time relax some restrictions that MySQL 5.7 introduced for ALTER TABLE…LOCK=NONE when indexed virtual columns exist.

            marko Marko Mäkelä added a comment - I think that we need a quick solution before the 10.2 GA release: simply refuse ALGORITHM=INPLACE if there are any non-constant DEFAULT values for added columns. Fixing this is somewhat tricky, because InnoDB would need to convert each row to MySQL format for evaluating the non-constant default values. Currently InnoDB uses a ‘default record’ of constant values that it will substitute for added columns. When fixing this properly, we could at the same time relax some restrictions that MySQL 5.7 introduced for ALTER TABLE…LOCK=NONE when indexed virtual columns exist.

            I believe there're 2 different issues:

            • with non-constant default values we get NULL when we subsequently query this data
            • with compressed columns we get buffer filled with 0x8F (presumably after TRASH_FREE)
            svoj Sergey Vojtovich added a comment - I believe there're 2 different issues: with non-constant default values we get NULL when we subsequently query this data with compressed columns we get buffer filled with 0x8F (presumably after TRASH_FREE)

            The fix to MDEV-8392 needs some refinement.

            In ADD COLUMN as well as when changing a NULL column to NOT NULL, we must currently reject ALGORITHM=INPLACE when the DEFAULT expression is not a constant.

            marko Marko Mäkelä added a comment - The fix to MDEV-8392 needs some refinement. In ADD COLUMN as well as when changing a NULL column to NOT NULL, we must currently reject ALGORITHM=INPLACE when the DEFAULT expression is not a constant.
            marko Marko Mäkelä added a comment - bb-10.2-marko

            ok to push after test is repeatable.

            jplindst Jan Lindström (Inactive) added a comment - ok to push after test is repeatable.

            marko, I appreciate your effort fixing this bug. But this task was created to reference issue found during column compression implementation, which I worked around by disabling INPLACE for compressed columns.

            Original goal of this task was to support INPLACE for compressed columns.

            I understand that it revealed another more serious issue, which was also worked around by disabling INPLACE for non-const DEFAULT.

            But now we have no open task neither for compressed columns nor for non-const DEFAULT to support INPLACE.

            I think it is not fair.

            svoj Sergey Vojtovich added a comment - marko , I appreciate your effort fixing this bug. But this task was created to reference issue found during column compression implementation, which I worked around by disabling INPLACE for compressed columns. Original goal of this task was to support INPLACE for compressed columns. I understand that it revealed another more serious issue, which was also worked around by disabling INPLACE for non-const DEFAULT. But now we have no open task neither for compressed columns nor for non-const DEFAULT to support INPLACE. I think it is not fair.

            People

              marko Marko Mäkelä
              svoj Sergey Vojtovich
              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.