MDEV-23485: Change table to merge engine may cause table data lost. #1651

zbdba · 2020-08-14T15:55:17Z

When I create a Innodb storage engine table, and I change it to MRG_MYISAM engine, the table data will be lost.

MariaDB [zbdba]> show create table zbdba\G

*************************** 1. row ***************************
Table: zbdba
Create Table: CREATE TABLE zbdba (
id int(11) NOT NULL,
name varchar(20) DEFAULT NULL,
PRIMARY KEY (id)
) ENGINE=InnoDB DEFAULT CHARSET=utf8
1 row in set (0.000 sec)

MariaDB [zbdba]>
MariaDB [zbdba]>
MariaDB [zbdba]> insert into zbdba() values(1, 'aa');
Query OK, 1 row affected (0.001 sec)

MariaDB [zbdba]> insert into zbdba() values(2, 'bb');
Query OK, 1 row affected (0.001 sec)

MariaDB [zbdba]> insert into zbdba() values(3, 'cc');
Query OK, 1 row affected (0.001 sec)

MariaDB [zbdba]> select * from zbdba;
+----+------+
| id | name |
+----+------+
| 1 | aa |
| 2 | bb |
| 3 | cc |
+----+------+
3 rows in set (0.001 sec)

MariaDB [zbdba]>
MariaDB [zbdba]>
MariaDB [zbdba]> ALTER TABLE zbdba ENGINE = MRG_MYISAM;
Query OK, 0 rows affected (0.005 sec)
Records: 0 Duplicates: 0 Warnings: 0

MariaDB [zbdba]> select * from zbdba;
Empty set (0.000 sec)

grooverdan · 2020-08-15T00:24:01Z

Nice simple case of preventing data loss. Well done.mysql-test/main/merge.test

Can I request:

Rebase against 10.1 branch, this probably fails there right?
Edit your commit message to begin with 'MDEV-23485: '
The problem is more generic based on your soultion, altering any table to engine=MyISAM_MRG will loose data. Consider the description of your commit message in light of this.
Include a more verbose commit message that describes more fully the nature of the problem/solution.
Include a test in mysql-test/t/merge.test (though use MyISAM as the origin table otherwise the test needs to check if innodb is compiled in).

Keep up the good work. Much appreciated.

zbdba · 2020-08-15T04:51:41Z

@grooverdan I have retry this case on the 10.1 branch and have the same problem. the result is :

Welcome to the MariaDB monitor. Commands end with ; or \g.
Your MariaDB connection id is 4
Server version: 10.1.47-MariaDB Source distribution

Type 'help;' or '\h' for help. Type '\c' to clear the current input statement.

MariaDB [(none)]> use zbdba
Database changed
MariaDB [zbdba]> create table zbdba(id int, name varchar(20), primary key(id));
Query OK, 0 rows affected (0.00 sec)

MariaDB [zbdba]> insert into zbdba() values(1, 'aa');
Query OK, 1 row affected (0.00 sec)

MariaDB [zbdba]> insert into zbdba() values(2, 'bb');
Query OK, 1 row affected (0.01 sec)

MariaDB [zbdba]> insert into zbdba() values(3, 'cc');
Query OK, 1 row affected (0.00 sec)

MariaDB [zbdba]> select * from zbdba;
+----+------+
| id | name |
+----+------+
| 1 | aa |
| 2 | bb |
| 3 | cc |
+----+------+
3 rows in set (0.00 sec)

MariaDB [zbdba]> ALTER TABLE zbdba ENGINE = MRG_MYISAM;
Query OK, 0 rows affected (0.01 sec)
Records: 0 Duplicates: 0 Warnings: 0

MariaDB [zbdba]> select * from zbdba;
Empty set (0.00 sec)

MariaDB [zbdba]>

When I add test case in the merge test suite, I have found there have another test suite rely on ALTER TABLE .... engine=MERGE or ALTER TABLE ..... UNION(t1, t2). this will make them falied. the failed test suite is:
1.rpl.rpl_row_merge_engine
2.grant2
3.alter_table_online
4.merge( which I add test case in)
5.ps_ddl

you can find the detail error log in the CI build.

should I fix all of the problem test suite?

grooverdan

Looking firstly at rpl.rpl_row_merge_engine selecting all the output from the merge table at the end

CREATE TABLE t1 (a int) ENGINE=MyISAM;
CREATE TABLE t2 (a int) ENGINE=MyISAM;
INSERT INTO t1 VALUES (1), (2), (3);
INSERT INTO t2 VALUES (4), (5), (6);
# Changed a little to check also an issue reported on BUG#20574550
CREATE TEMPORARY TABLE IF NOT EXISTS tt1_merge LIKE t1;
ALTER TABLE tt1_merge ENGINE=MERGE UNION (t2, t1);
SELECT * FROM tt1_merge;

The result is

$ mysql-test/mtr --mem rpl.rpl_row_merge_engine
...
@@ -7,6 +7,14 @@
 INSERT INTO t2 VALUES (4), (5), (6);
 CREATE TEMPORARY TABLE IF NOT EXISTS tt1_merge LIKE t1;
 ALTER TABLE tt1_merge ENGINE=MERGE UNION (t2, t1);
+SELECT * FROM tt1_merge;
+a
+4
+5
+6
+1
+2
+3
 CREATE TABLE t1_merge LIKE tt1_merge;

Indicating that a myisam table originally can be ALTER TABLE to a merge table correctly.

As such it seems this current patch is insufficient. Some detailed check in an existing ha_myisammrg::iXXXalter_table function on the source table type and returning an existing cannot covert error if it cannot be converted.

zbdba · 2020-08-15T05:59:34Z

@grooverdan ok, I will check it and fix it.

grooverdan · 2020-08-17T00:00:06Z

Thinking a bit more as to what constitutes a valid alter table where ENGINE=MERGE I've come up with:

The table time was MERGE; or
The original wasn't MERGE and there where no rows in the table.

Alternate interpretations welcome.

If this is getting too complicated I won't be offended I you drop it.

zbdba · 2020-08-17T14:21:18Z

@grooverdan If we want to change a table to merge engine, the original table should't be merge engine table and the table have no rows.I can't find a good way to confirm table have rows or not. and now I can just use the init_read_record method to get record from table to confirm it.Is there have some good way?

grooverdan · 2020-08-20T05:14:12Z

Yes, handler api to check for a row is right. init_read_record seems right.

zbdba · 2020-08-22T12:56:40Z

@grooverdan I have add this logic and run the test, I find threre have a test case which change a MyISAM engine to merge engine and the original table have rows.the test case is in the main.merge test suite. and the test statement is:

--echo #
ALTER TABLE m1 ENGINE=MyISAM;
SHOW CREATE TABLE m1;
INSERT INTO m1 VALUES (511, 521);
SELECT * FROM m1;
--echo #
ALTER TABLE m1 ENGINE=MRG_MyISAM UNION=(t1,t2)
INSERT_METHOD=LAST;
SELECT * FROM m1;
SELECT * FROM t1;
SELECT * FROM t2;

shall we should change this test case?

grooverdan · 2020-08-24T00:21:10Z

That test has a goal of showing that a previous myisam_mrg table can changed to an operational myisam tables. Followed truncate table m1 before changing continuing the test, and/or testing the error message that your code generates, is perfectly acceptable.

zbdba · 2020-08-24T09:57:43Z

@grooverdan I have commit, please help review it, thanks.

grooverdan · 2020-08-24T20:48:36Z

sql/sql_table.cc

+   * should't be merge engine table and the table have no rows, otherwise
+   * may cause table data lost. */
+  if (table->file->ht->db_type != DB_TYPE_MRG_MYISAM &&
+      create_info->db_type->db_type == DB_TYPE_MRG_MYISAM) {


Please keep the comments in a consistent style to existing comments.
Syntax formatting of if statements have a { on the new line.

grooverdan · 2020-08-24T20:54:38Z

sql/sql_table.cc

+      DBUG_RETURN(true);
+    }
+
+    while (likely(!(error= info.read_record())))


So this is just an if statement then? If so you don't need the variable have_rows and can flow it to the error condition below.

Can you remove the likely here. Its just harder to read and really isn't a performance critical path.

How are you doing to deal with errors here?

grooverdan · 2020-08-24T21:14:21Z

sql/sql_table.cc

+    if (have_rows) {
+      DBUG_PRINT("info", ("The original table is not merge table "
+                          "and have rows doesn't support alter"));
+      my_error(ER_ILLEGAL_HA, MYF(0), hton_name(table->s->db_type())->str,


DBUG_PRINT statements are only in debug builds for helping developers out. We really want this error message to be for the user.

As we using such precise works in a error message like ER_ALTER_OPERATION_NOT_SUPPORTED. The arguements to it however should be syntax and not some readable string otherwise translation won't be possible
.
So please:
a) rebase on 10.6 - we can't add new error message in a GA release
b) create a new error message sql/share/errmsg-utf8.txt

The alter errors are pushed back in the context of the original alter table statement so the db/table name isn't that significant. The wording of error should be close to the DBUG_PRINT text however make it have have a similar read to the other ER_ALTER_OPERATION_NOT_SUPPORTED errors.

grooverdan · 2020-08-24T21:20:27Z

sql/sql_table.cc

+      create_info->db_type->db_type == DB_TYPE_MRG_MYISAM) {
+    READ_RECORD info;
+    bool have_rows= false;
+    if (init_read_record(&info, thd, table, NULL, NULL, 1, 1, FALSE)) {


init_read_record needs to be freed with end_read_record(&info)

grooverdan · 2020-08-24T22:03:56Z

sql/sql_table.cc

@@ -10130,6 +10130,33 @@ bool mysql_alter_table(THD *thd, const LEX_CSTRING *new_db,
    DBUG_RETURN(true);
  }

+  /* MDEV-23485: Change table to merge engine may cause table data lost.


Is there any earlier hooks into the ha_myisammrg handler that are called so this addition can be in the handler rather than the main alter table implementation? I ask because I assume you've looked. I'll check back on this alter.

I have checked there have no hooks are called in the ha_myisammrg handler, except the inplace support check.

grooverdan · 2020-08-24T23:41:37Z

mysql-test/main/merge.result

@@ -3445,6 +3447,8 @@ c1	c2
 411	421
 511	521
 #
+# MDEV-23485 - Change table to merge engine may cause table data lost.
+truncate table m1;


its not necessary to comment this line or the above truncate table. Its covered in the commit message and could distract a reader to think this is part of the MDEV-23485 test .

The test block below is good but you'll notice in the results that the error message isn't that useful for a user (hence review requesting another error message).

zbdba · 2020-09-01T07:04:52Z

@grooverdan I have update the commit, and how rebase to 10.6? I have try but will cause the older commit to merge together.

grooverdan · 2020-09-01T13:21:31Z

I've changed the based of the PR. Try do git rebase --onto origin/10.6 bottomofcommits MDEV-23485 and then force push.

zbdba · 2020-09-01T15:40:24Z

@grooverdan thanks, I have done.

Problem: If we want to change a table to merge engine, the original table is not merge engine table and have rows may cause table data lost. Solution: Add check in the mysql_alter_table method, check if the original table want to change to merge engine, it should't be merge engine and have no rows. closes #1651

grooverdan · 2020-10-05T00:48:32Z

I got some feedback from @montywi

There is couple of issues with the patch:

The code test for specific engines. This should be avoided with all costs if possible. The whole idea with the engine interrface is that the sql layer should not know anything specific about any engine.

There is a code to test if the table has zero rows. The way of testing is wrong. If one would like to know how many rows the  table has, there is an info calls about that which is always exact for 0 or 1 rows.

How this should be done:

Add a HTON flag to MERGE of type HTON_NO_CHANGE_TO_ENGINE.

The flag should be added to handler.h and specify that the engine doesn't support ALTER TABLE from another engine to this engine.

Add a test to sql_table.cc that in case of engine change the TO engine doesn't have this flag and if yes, give an error that alter table is unsupported for this engine (we should already have an error message for this)

We do miss the case to alter a empty table to MERGE, but I don't think that is important. Better to be consistent.

zbdba · 2020-12-22T02:29:38Z

Okay, I was busy preparing a share before, now I have time to solve this problem.

vuvova · 2020-12-22T11:50:45Z

@grooverdan, instead of new HTON_NO_CHANGE_TO_ENGINE flag, I'd rather use exiting HA_NO_COPY_ON_ALTER.

It was originally created for MERGE and used with the comment "We do not copy data for MERGE tables. Only the children have data". It seems logical that if only children have data that one should not be able to alter InnoDB table (with data) into a MERGE table (without data) as it'll cause a data loss.

Since its introduction (in 2005?) this flag HA_NO_COPY_ON_ALTER is now also used for SPIDER and outward CONNECT tables. And this seems appropriate too, outward CONNECT tables don't own the data, so one should not be able to alter InnoDB table into an outward connect table. So again, using this flag to disallow alter seems correct.

Or, let's put it another way, altering InnoDB table into a MERGE table (into any other engine, really) would inevitably involve copying of the data. And HA_NO_COPY_ON_ALTER prohibits that. No need for a new flag.

Problem: If we want to change a table to merge engine, the original table is not merge engine table may cause table data lost. Solution: Add check in the mysql_alter_table method, check the other table engine can't change to merge engine.

zbdba · 2020-12-26T10:15:56Z

@grooverdan @vuvova HA_NO_COPY_ON_ALTER seem to can be used here. Use HTON flag to control other table engine can't change to merge engine will cause some test case failed, I have fixed it and please help review it.

grooverdan self-requested a review August 15, 2020 00:24

grooverdan added this to the 10.1 milestone Aug 15, 2020

grooverdan self-assigned this Aug 15, 2020

grooverdan requested changes Aug 15, 2020

View reviewed changes

zbdba changed the title ~~MDEV-23485:Modify Innodb storage engine to MyISAM_MRG will make data lost.~~ MDEV-23485: Change table to merge engine may cause table data lost. Aug 24, 2020

zbdba force-pushed the MDEV-23485 branch from 69cf969 to c32037c Compare August 24, 2020 09:55

grooverdan requested changes Aug 24, 2020

View reviewed changes

grooverdan reviewed Aug 24, 2020

View reviewed changes

zbdba force-pushed the MDEV-23485 branch from 8a3d1f3 to 665a1d4 Compare September 1, 2020 06:56

zbdba changed the base branch from 10.5 to 10.6 September 1, 2020 06:57

zbdba changed the base branch from 10.6 to 10.5 September 1, 2020 07:01

grooverdan changed the base branch from 10.5 to 10.6 September 1, 2020 13:18

zbdba force-pushed the MDEV-23485 branch 2 times, most recently from 630120d to 0c9f3eb Compare September 1, 2020 15:38

zbdba force-pushed the MDEV-23485 branch from 0c9f3eb to f3e32da Compare December 26, 2020 10:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDEV-23485: Change table to merge engine may cause table data lost. #1651

MDEV-23485: Change table to merge engine may cause table data lost. #1651

zbdba commented Aug 14, 2020

grooverdan commented Aug 15, 2020

zbdba commented Aug 15, 2020

grooverdan left a comment

zbdba commented Aug 15, 2020

grooverdan commented Aug 17, 2020

zbdba commented Aug 17, 2020

grooverdan commented Aug 20, 2020

zbdba commented Aug 22, 2020 •

edited

grooverdan commented Aug 24, 2020

zbdba commented Aug 24, 2020

grooverdan Aug 24, 2020

grooverdan Aug 24, 2020

grooverdan Aug 24, 2020

grooverdan Aug 24, 2020

grooverdan Aug 24, 2020

zbdba Aug 25, 2020

grooverdan Aug 24, 2020

zbdba commented Sep 1, 2020

grooverdan commented Sep 1, 2020

zbdba commented Sep 1, 2020

grooverdan commented Oct 5, 2020

zbdba commented Dec 22, 2020

vuvova commented Dec 22, 2020 •

edited

zbdba commented Dec 26, 2020

MDEV-23485: Change table to merge engine may cause table data lost. #1651

Are you sure you want to change the base?

MDEV-23485: Change table to merge engine may cause table data lost. #1651

Conversation

zbdba commented Aug 14, 2020

MariaDB [zbdba]> show create table zbdba\G

grooverdan commented Aug 15, 2020

zbdba commented Aug 15, 2020

grooverdan left a comment

Choose a reason for hiding this comment

zbdba commented Aug 15, 2020

grooverdan commented Aug 17, 2020

zbdba commented Aug 17, 2020

grooverdan commented Aug 20, 2020

zbdba commented Aug 22, 2020 • edited

grooverdan commented Aug 24, 2020

zbdba commented Aug 24, 2020

grooverdan Aug 24, 2020

Choose a reason for hiding this comment

grooverdan Aug 24, 2020

Choose a reason for hiding this comment

grooverdan Aug 24, 2020

Choose a reason for hiding this comment

grooverdan Aug 24, 2020

Choose a reason for hiding this comment

grooverdan Aug 24, 2020

Choose a reason for hiding this comment

zbdba Aug 25, 2020

Choose a reason for hiding this comment

grooverdan Aug 24, 2020

Choose a reason for hiding this comment

zbdba commented Sep 1, 2020

grooverdan commented Sep 1, 2020

zbdba commented Sep 1, 2020

grooverdan commented Oct 5, 2020

zbdba commented Dec 22, 2020

vuvova commented Dec 22, 2020 • edited

zbdba commented Dec 26, 2020

zbdba commented Aug 22, 2020 •

edited

vuvova commented Dec 22, 2020 •

edited