[MDEV-26137] ALTER TABLE IMPORT enhancement Created: 2021-07-13  Updated: 2024-01-12  Resolved: 2023-07-05

Status: Closed
Project: MariaDB Server
Component/s: Storage Engine - InnoDB
Fix Version/s: 11.2.1

Type: Task Priority: Critical
Reporter: Eugene Kosov (Inactive) Assignee: Yuchen Pei
Resolution: Fixed Votes: 1
Labels: Preview_11.2

Issue Links:
Blocks
blocks MDEV-11658 Simpler, faster IMPORT of InnoDB tables Open
blocks MDEV-15225 Can't import .ibd file with temporal ... Closed
blocks MDEV-18762 Support easy restore of partial backup Closed
is blocked by MDEV-31010 Q2 2023 release merge Closed
Relates
relates to MDEV-33226 ALTER TABLE IMPORT enhancement for Pa... Open
relates to MDEV-10568 Implement support for ALTER TABLE ...... Open
relates to MDEV-15225 Can't import .ibd file with temporal ... Closed
relates to MDEV-26155 Mangled data after IMPORT tablespace ... Open

 Description   

The feature introduces simplified workflow:

FLUSH TABLES t1 FOR EXPORT;
 
--copy_file $MYSQLD_DATADIR/test/t1.cfg $MYSQLD_DATADIR/test/t2.cfg
 
--copy_file $MYSQLD_DATADIR/test/t1.frm $MYSQLD_DATADIR/test/t2.frm
 
--copy_file $MYSQLD_DATADIR/test/t1.ibd $MYSQLD_DATADIR/test/t2.ibd
 
UNLOCK TABLES;
 
ALTER TABLE t2 IMPORT TABLESPACE;

Previously, CREATE TABLE and ALTER TABLE…DISCARD TABLESPACE had to be executed, and the exact table definition from an existing .frm file could not be reused. This simplification addresses MDEV-15225.

ha_innodb::open() should check for .ibd and .cfg files before actually opening a table.

 Comments   
Comment by Sergei Golubchik [ 2021-07-14 ]

Why

ha_innodb::open() should check for .ibd and .cfg files before actually opening a table.

?

I'd think InnoDB first try to do a normal open, and only if the table isn't found it should look for files and try to import.

Comment by Marko Mäkelä [ 2023-02-01 ]

serg, you are right, it should be the other way around.

I think that this will be needed for MDEV-11658. The added feature of MDEV-11658 would be that no .cfg file would be needed for .ibd files that are in a new format that includes a directory of secondary index root page numbers.

Comment by Yuchen Pei [ 2023-02-07 ]

mtr testcase / specification (based on similar feature of myisam):

mysql-test/suite/innodb/t/improved_import.test

 
--source include/have_innodb.inc
 
--echo #
 
--echo # MDEV-26137 ALTER TABLE IMPORT enhancement
 
--echo #
 
let $MYSQLD_DATADIR = `SELECT @@datadir`;
 
CREATE TABLE t1 (a int) ENGINE=InnoDB;
 
INSERT INTO t1 VALUES(42);
 
FLUSH TABLES t1 FOR EXPORT;
 
--copy_file $MYSQLD_DATADIR/test/t1.cfg $MYSQLD_DATADIR/test/t2.cfg
 
--copy_file $MYSQLD_DATADIR/test/t1.frm $MYSQLD_DATADIR/test/t2.frm
 
--copy_file $MYSQLD_DATADIR/test/t1.ibd $MYSQLD_DATADIR/test/t2.ibd
 
UNLOCK TABLES; # See [1] below.
 
SELECT a FROM t2;
 
DROP TABLE t1;
 
DROP TABLE t2;

mysql-test/suite/innodb/r/improved_import.result

 
#
 
# MDEV-26137 ALTER TABLE IMPORT enhancement
 
#
 
CREATE TABLE t1(a int) ENGINE=InnoDB;
 
INSERT INTO t1 VALUES(42);
 
FLUSH TABLES t1 FOR EXPORT;
 
SELECT a FROM t2;
 
a
 
42
 
DROP TABLE t1;
 
DROP TABLE t2;

P.S. The myisam example:

let $MYSQLD_DATADIR = `SELECT @@datadir`;
 
CREATE TABLE t1 (a int) ENGINE=MyISAM;
 
INSERT INTO t1 VALUES (42);
 
FLUSH TABLES t1;
 
--copy_file $MYSQLD_DATADIR/test/t1.frm $MYSQLD_DATADIR/test/t2.frm
 
--copy_file $MYSQLD_DATADIR/test/t1.MYD $MYSQLD_DATADIR/test/t2.MYD
 
--copy_file $MYSQLD_DATADIR/test/t1.MYI $MYSQLD_DATADIR/test/t2.MYI
 
SELECT a FROM t2;
 
DROP TABLE t1;
 
DROP TABLE t2;

[1] Do we need UNLOCK TABLES;? The myisam does not require it, because it does not require FOR EXPORT in the FLUSH statement, but it seems as long as we need the .cfg files (its removal is part of MDEV-11658), we need the FOR EXPORT. Thus perhaps we need the UNLOCK statement, so that we don't get blocked in the sql layer (with the weird ER_TABLE_NOT_LOCKED error), before even attempting to open with the storage engine (open_table_from_share()).

And for comparison, the current innodb import workflow:

--source include/have_innodb.inc
 
let $MYSQLD_DATADIR = `SELECT @@datadir`;
 
CREATE TABLE t1 (a int) ENGINE=InnoDB;
 
INSERT INTO t1 VALUES(42);
 
CREATE TABLE t2 LIKE t1;
 
ALTER TABLE t2 DISCARD TABLESPACE;
 
FLUSH TABLES t1 FOR EXPORT;
 
--copy_file $MYSQLD_DATADIR/test/t1.cfg $MYSQLD_DATADIR/test/t2.cfg
 
--copy_file $MYSQLD_DATADIR/test/t1.ibd $MYSQLD_DATADIR/test/t2.ibd
 
UNLOCK TABLES;
 
ALTER TABLE t2 IMPORT TABLESPACE;
 
SELECT * from t2;
 
DROP TABLE t1;
 
DROP TABLE t2;

Comment by Marko Mäkelä [ 2023-02-13 ]

InnoDB does need explicit locking statements at the SQL level (FLUSH TABLES…FOR EXPORT to lock, and UNLOCK TABLES to unlock) in order to prevent the data file from being modified by the purge of committed transaction history, which is analogous to the Postgres VACUUM. Without such locking, it would be unsafe to copy the .ibd file while InnoDB is running.

Comment by Yuchen Pei [ 2023-02-20 ]

New spec, based on discussions at #innodb:

mysql-test/suite/innodb/t/improved_import.test

 
--source include/have_innodb.inc
 
--echo #
 
--echo # MDEV-26137 ALTER TABLE IMPORT enhancement
 
--echo #
 
let $MYSQLD_DATADIR = `SELECT @@datadir`;
 
CREATE TABLE t1 (a int) ENGINE=InnoDB;
 
INSERT INTO t1 VALUES(42);
 
FLUSH TABLES t1 FOR EXPORT;
 
--copy_file $MYSQLD_DATADIR/test/t1.cfg $MYSQLD_DATADIR/test/t2.cfg
 
--copy_file $MYSQLD_DATADIR/test/t1.frm $MYSQLD_DATADIR/test/t2.frm
 
--copy_file $MYSQLD_DATADIR/test/t1.ibd $MYSQLD_DATADIR/test/t2.ibd
 
UNLOCK TABLES;
 
ALTER TABLE t2 IMPORT TABLESPACE;
 
SELECT a FROM t2;
 
DROP TABLE t1;
 
DROP TABLE t2;

Comment by Yuchen Pei [ 2023-02-21 ]

Hi thiru, PTAL thanks

https://github.com/MariaDB/server/commit/3d942ceb701cd9484c22ca34f58d2ebecf04e2ea

Comment by Thirunarayanan Balathandayuthapani [ 2023-02-21 ]
  • CREATE_INFO doesn't have any value. Better to add few more test cases.
Comment by Yuchen Pei [ 2023-03-02 ]

thiru as discussed at #innodb, please take a look at the general approach of getting the row format from ibd and cfg in the following patch, thank you

https://github.com/MariaDB/server/commit/bee3d9ee39a86b65ca26da6e60069a8c6a0cef92

Comment by Yuchen Pei [ 2023-03-09 ]

thiru Thanks for the comments. I've addressed all of them and other comments in #innodb discussions. PTAL thanks https://github.com/MariaDB/server/commit/da65c00ddea

Comment by Yuchen Pei [ 2023-03-09 ]

Fixed a typo, sorry about the problem. Updated patch:

https://github.com/MariaDB/server/commit/937e042add5e64af4343ded0f6e07f3ede040d81

Comment by Yuchen Pei [ 2023-03-10 ]

Thanks for the comments thiru - I have addressed all of them and the updated patch is available at https://github.com/MariaDB/server/commit/69e1a3bb8b5

Comment by Yuchen Pei [ 2023-03-14 ]

Thanks for the comments thiru, PTAL thanks: https://github.com/MariaDB/server/commit/676038b1076756666e985aaa9d01641566584e8f

squashed commit at https://github.com/MariaDB/server/commit/790e5ca64b2

Comment by Thirunarayanan Balathandayuthapani [ 2023-03-14 ]

Gave few review comments. Please address them, Commit message could be more elaborate.

Comment by Yuchen Pei [ 2023-03-15 ]

Thanks for the comments thiru, especially finding a new failing testcase. Updated (and squashed) commit at [1], and individual commits in the branch at [2]. Would you like to take another look?

[1] https://github.com/MariaDB/server/commit/5f8f9de5c6e308239c5e99147304742e69c54ef1
[2] https://github.com/MariaDB/server/compare/bb-11.0-mdev-26137-unsquashed

Comment by Marko Mäkelä [ 2023-03-15 ]

Please address my comment regarding crash injection testing.

Comment by Yuchen Pei [ 2023-03-16 ]

Thanks for the comments marko thiru. All your comments are addressed, and the CI looks in a much better state now [1][2] Please find the updated commit at https://github.com/MariaDB/server/commit/653e4905904, thank you.
The diff with the previous comment is at
https://github.com/MariaDB/server/commit/d59cffd55b4

[1] https://buildbot.mariadb.org/#/grid?branch=bb-11.1-mdev-26137
[2] https://buildbot.mariadb.org/#/grid?branch=bb-11.0-mdev-26137

Comment by Thirunarayanan Balathandayuthapani [ 2023-03-16 ]

I have only one review comment. Don't have anything significant issue though. Approved from my side

Comment by Yuchen Pei [ 2023-03-16 ]

thiru Thanks for the review. I have addressed your comments, as well as comments about removing rdiffs - all rdiffs are gone now. Updated patch:

https://github.com/MariaDB/server/commit/5d0cce962f3441f6061f1ef9c5608ad3f9475513

diff with previous commit:

https://github.com/MariaDB/server/commit/605b8fd8462b17a5681e33a3691b8d4c7bf862a7

marko: Since thiru has approved the patch, would you like to take a look?

Comment by Marko Mäkelä [ 2023-03-20 ]

Please try to minimize the changes to row0import.h. I think that we should try to keep as much of the low-level code related to IMPORT in row0import.cc as possible, and only export higher-level interfaces.

Comment by Yuchen Pei [ 2023-03-21 ]

Thanks for the comments marko - I have addressed all of them, and the updated patch can be found at

https://github.com/MariaDB/server/commit/3e840a9a621

The diff from the previous one at

https://github.com/MariaDB/server/commit/f4cbd42401d

Comment by Yuchen Pei [ 2023-03-28 ]

Addressed some formatting and debug_sync comments at #innodb and pushed a small commit at https://github.com/MariaDB/server/commit/86d1d970961

Comment by Marko Mäkelä [ 2023-03-29 ]

I posted some suggested cleanup to the tests and the code. If you agree with them, please squash and rebase everything to a single commit based on the latest 11.1. The next step would be some stress testing.

Comment by Yuchen Pei [ 2023-03-29 ]

Thanks marko, I have adjusted the commits accordingly (https://github.com/MariaDB/server/commit/44558f70c7c) and pushed the squashed version rebased at 11.1 to

https://github.com/MariaDB/server/commit/2d36352c138

I've changed the ticket status to request testing. Shall I re-assign it to mleich?

Comment by Matthias Leich [ 2023-04-05 ] 

origin/bb-11.1-mdev-26137 44558f70c7cf246dc939b388b0fb786f3fb8c6ec 2023-03-30T10:25:02+11:00 behaved well in RQG testing.
 
The bad effects observed are in origin/11.1 2b61ff8f2221745f0a96855a0feb0825c426f993 2023-03-29T17:23:21+03:00 too.

Comment by Matthias Leich [ 2023-06-12 ] 

Preliminary results of RQG testing:
 
pluto:/data/results/1686581815/TBR-1938$ _RR_TRACE_DIR=./1/rr/ rr replay --mark-stdio
 
[rr 4081243 823272]mysqld: /data/Server/bb-11.1-MDEV-26137/storage/innobase/handler/handler0alter.cc:6223: bool innobase_instant_try(const Alter_inplace_info*, ha_innobase_inplace_ctx*, const TABLE*, const TABLE*, trx_t*): Assertion `"wrong page type" == 0' failed.
 
(rr) bt
 
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
 
#1  0x00007ffd4bde5859 in __GI_abort () at abort.c:79
 
#2  0x00007ffd4bde5729 in __assert_fail_base (fmt=0x7ffd4bf7b588 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=0x5584833e8160 "\"wrong page type\" == 0", 
    file=0x5584833d64a0 "/data/Server/bb-11.1-MDEV-26137/storage/innobase/handler/handler0alter.cc", line=6223, function=<optimized out>) at assert.c:92
 
#3  0x00007ffd4bdf6f36 in __GI___assert_fail (assertion=assertion@entry=0x5584833e8160 "\"wrong page type\" == 0", 
    file=file@entry=0x5584833d64a0 "/data/Server/bb-11.1-MDEV-26137/storage/innobase/handler/handler0alter.cc", line=line@entry=6223, 
    function=function@entry=0x5584833e7160 "bool innobase_instant_try(const Alter_inplace_info*, ha_innobase_inplace_ctx*, const TABLE*, const TABLE*, trx_t*)") at assert.c:101
 
#4  0x0000558482136237 in innobase_instant_try (ha_alter_info=ha_alter_info@entry=0x5858556caaf0, ctx=ctx@entry=0x629000db95d8, altered_table=altered_table@entry=0x5858556cb1f0, table=table@entry=0x61900050c898, 
    trx=trx@entry=0x16183f04f440) at /data/Server/bb-11.1-MDEV-26137/storage/innobase/handler/handler0alter.cc:6223
 
#5  0x00005584821385fb in commit_try_norebuild (ha_alter_info=ha_alter_info@entry=0x5858556caaf0, ctx=ctx@entry=0x629000db95d8, altered_table=altered_table@entry=0x5858556cb1f0, old_table=<optimized out>, 
    trx=trx@entry=0x16183f04f440, table_name=<optimized out>) at /data/Server/bb-11.1-MDEV-26137/storage/innobase/handler/handler0alter.cc:10621
 
#6  0x00005584820f8833 in ha_innobase::commit_inplace_alter_table (this=<optimized out>, altered_table=<optimized out>, ha_alter_info=<optimized out>, commit=<optimized out>)
 
    at /data/Server/bb-11.1-MDEV-26137/storage/innobase/handler/handler0alter.cc:11384
 
#7  0x00005584816efd51 in handler::ha_commit_inplace_alter_table (this=0x61d000c5b2b8, altered_table=altered_table@entry=0x5858556cb1f0, ha_alter_info=ha_alter_info@entry=0x5858556caaf0, commit=commit@entry=true)
 
    at /data/Server/bb-11.1-MDEV-26137/sql/handler.cc:5432
 
#8  0x000055848106da39 in mysql_inplace_alter_table (thd=thd@entry=0x62b00019d218, table_list=<optimized out>, table=table@entry=0x61900050c898, altered_table=<optimized out>, ha_alter_info=<optimized out>, 
    target_mdl_request=<optimized out>, ddl_log_state=<optimized out>, trigger_param=<optimized out>, alter_ctx=<optimized out>, partial_alter=<optimized out>, start_alter_id=<optimized out>, if_exists=<optimized out>)
 
    at /data/Server/bb-11.1-MDEV-26137/sql/sql_table.cc:7810
 
#9  0x00005584810a764e in mysql_alter_table (thd=thd@entry=0x62b00019d218, new_db=new_db@entry=0x62b0001a1d40, new_name=new_name@entry=0x62b0001a2190, create_info=0x5858556cd5b0, table_list=<optimized out>, 
    table_list@entry=0x629000db63f8, recreate_info=<optimized out>, alter_info=<optimized out>, order_num=<optimized out>, order=<optimized out>, ignore=<optimized out>, if_exists=<optimized out>)
 
    at /data/Server/bb-11.1-MDEV-26137/sql/sql_table.cc:10846
 
#10 0x000055848123e3d7 in Sql_cmd_alter_table::execute (this=<optimized out>, thd=0x62b00019d218) at /data/Server/bb-11.1-MDEV-26137/sql/structs.h:568
 
#11 0x0000558480d91f11 in mysql_execute_command (thd=thd@entry=0x62b00019d218, is_called_from_prepared_stmt=is_called_from_prepared_stmt@entry=false) at /data/Server/bb-11.1-MDEV-26137/sql/sql_parse.cc:5766
 
#12 0x0000558480d4975f in mysql_parse (thd=thd@entry=0x62b00019d218, rawbuf=<optimized out>, length=<optimized out>, parser_state=parser_state@entry=0x5858556cf130) at /data/Server/bb-11.1-MDEV-26137/sql/sql_parse.cc:7769
 
#13 0x0000558480d786d9 in dispatch_command (command=command@entry=COM_QUERY, thd=thd@entry=0x62b00019d218, 
    packet=packet@entry=0x62900129d219 " ALTER TABLE t4 DROP COLUMN IF EXISTS col_int_g, ALGORITHM = NOCOPY, LOCK = NONE  /* E_R Thread2 QNO 13025 CON_ID 97 */ ", packet_length=packet_length@entry=120, 
    blocking=blocking@entry=true) at /data/Server/bb-11.1-MDEV-26137/sql/sql_class.h:1367
 
#14 0x0000558480d7f527 in do_command (thd=0x62b00019d218, blocking=blocking@entry=true) at /data/Server/bb-11.1-MDEV-26137/sql/sql_parse.cc:1405
 
#15 0x0000558481226e8b in do_handle_one_connection (connect=<optimized out>, connect@entry=0x608000002f38, put_in_cache=put_in_cache@entry=true) at /data/Server/bb-11.1-MDEV-26137/sql/sql_connect.cc:1416
 
#16 0x00005584812279d4 in handle_one_connection (arg=0x608000002f38) at /data/Server/bb-11.1-MDEV-26137/sql/sql_connect.cc:1318
 
#17 0x000015372bb0f609 in start_thread (arg=<optimized out>) at pthread_create.c:477
 
#18 0x00007ffd4bee2293 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95
 
(rr) 
Hint:   col_int_g is a virtual column.

Comment by Matthias Leich [ 2023-06-13 ]

origin/bb-11.1-MDEV-26137 7ae4e57e4e679c5ef14d42a79b6861c6a41f96c5 2023-06-08T15:34:24+03:00 performed well in RQG testing.
The problems observed are on other trees too, in JIRA etc.

Comment by Yuchen Pei [ 2023-06-15 ]

Thanks mleich, is it ok for me to push the commit to 11.2?

Comment by Matthias Leich [ 2023-06-16 ]

From my side yes

Comment by Marko Mäkelä [ 2023-06-30 ]

There are some occasional failures of the added regression test innodb.import_recovery, where InnoDB would fail to start up. The test is intentionally corrupting some InnoDB data files. It may be necessary to force an InnoDB checkpoint or page flush before doing that, to get deterministic results on a subsequent server restart. The the test innodb.page_cleaner could be useful there.

Comment by Yuchen Pei [ 2023-07-03 ]

Thanks for the suggestion marko. I looked into
innodb.page_cleaner, studied the relevant variables, and made an
attempt towards that direction[1], but given I am not able to
reproduce the failure locally, I cannot tell whether it would work
without waiting for the CI

[1] https://github.com/MariaDB/server/commit/e8c2d177d76

Comment by Yuchen Pei [ 2023-07-03 ]

Updated patch at https://github.com/MariaDB/server/commit/f3d86acf1fe, checking CI...

Comment by Yuchen Pei [ 2023-07-04 ]

Squashed the following 32bit fixup and import_recovery fixup commits
(see the list below) into https://github.com/MariaDB/server/commit/9b431d714fd

Comment by Marko Mäkelä [ 2023-07-04 ]

Thank you, this looks good to me.

Comment by Yuchen Pei [ 2023-07-05 ]

Pushed 9b431d714fdf030e372ed6eeaec0619dcffdde6a to 11.2, closing

Generated at Thu Feb 08 09:43:01 UTC 2024 using Jira 8.20.16#820016-sha1:9d11dbea5f4be3d4cc21f03a88dd11d8c8687422.