Skip to content

Commit 5f51a3a

Browse files
bnesterebnestere
committed
MDEV-36906: RBR crashes upon DML after CONVERT PARTITION
MDEV-33658 part 1’s refactoring ecaedbe introduced a new function init_key_info which (in part) aims to calculate the total key length; however, it doesn’t account for the key already having been initialized (as happens when called via ALTER TABLE .. CONVERT PARTITION .. TO TABLE). This leads to crashes when this key is later iterated over, because the iterator will try to iterate over additional key parts which don’t exist because the length reports as longer than the actual memory owned. The crash reported by MDEV-36906 highlights this in function key_copy. To explain how the keys already have been initialized, init_key_info is called multiple times. That is, init_key_info is called from mysql_prepare_create_table, which prepares a table and its key structures for table creation, which is in turn called by mysql_write_frm when using flags MFRM_WRITE_SHADOW and MFRM_WRITE_CONVERTED_TO. The ALTER TABLE .. CONVERT PARTITION .. TO TABLE use case (see function fast_alter_partition_table), calls mysql_write_frm multiple times with both of these flags set (first with MFRM_WRITE_CONVERTED_TO and then with MFRM_WRITE_SHADOW). Raising it up a level, mysql_prepare_create_table doesn't need to be called again after it has already been invoked when just writing frms. Init_key_info is the only place in that function which leads to side effects, but the rest is redundant and can be skipped on the second call (i.e. when writing the shadow). The patch fixes this by skipping the call to mysql_prepare_create_table in mysql_write_frm in the MFRM_WRITE_SHADOW block when it has already been called previously. To track whether or not it has been previously called, we add a new flag for the mysql_write_frm function, MFRM_ALTER_INFO_PREPARED, which is hard-coded into the function call on the later invocation. Test case based on work by Elena Stepanova <elenst@mariadb.com> Reviewed By: ============ Sergei Golubchik <serg@mariadb.org> Nikita Malyavin <nikita.malyavin@mariadb.com>
1 parent 33e8455 commit 5f51a3a

File tree

5 files changed

+239
-8
lines changed

5 files changed

+239
-8
lines changed

‎mysql-test/suite/rpl/r/rpl_alter_convert_partition.result‎

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
include/master-slave.inc
2+
[connection master]
3+
#
4+
# Ensure initial CREATE TABLE with partitioned data is replicated
5+
# correctly
6+
connection master;
7+
create table t (a int, b int, key(a)) engine=innodb partition by range (b) (partition p1 values less than (10), partition pn values less than (maxvalue));
8+
insert into t values (1,5),(2,100);
9+
include/save_master_gtid.inc
10+
connection slave;
11+
include/sync_with_master_gtid.inc
12+
include/diff_tables.inc [master:test.t,slave:test.t]
13+
#
14+
# Ensure ALTER TABLE .. CONVERT PARTITION .. TO TABLE replicates
15+
# correctly
16+
connection master;
17+
alter table t convert partition p1 to table offspring;
18+
include/save_master_gtid.inc
19+
connection slave;
20+
include/sync_with_master_gtid.inc
21+
include/diff_tables.inc [master:test.t,slave:test.t]
22+
include/diff_tables.inc [master:test.offspring,slave:test.offspring]
23+
#
24+
# Ensure data can be inserted into existing table after
25+
# ALTER TABLE .. CONVERT PARTITION .. TO TABLE
26+
connection master;
27+
insert into t values (3, 6);
28+
include/save_master_gtid.inc
29+
connection slave;
30+
include/sync_with_master_gtid.inc
31+
include/diff_tables.inc [master:test.t,slave:test.t]
32+
#
33+
# Ensure data can be inserted into offspring table after
34+
# ALTER TABLE .. CONVERT PARTITION .. TO TABLE
35+
connection master;
36+
insert into offspring values (4, 101);
37+
include/save_master_gtid.inc
38+
connection slave;
39+
include/sync_with_master_gtid.inc
40+
include/diff_tables.inc [master:test.offspring,slave:test.offspring]
41+
#
42+
# Ensure data can be updated in existing table after
43+
# ALTER TABLE .. CONVERT PARTITION .. TO TABLE
44+
connection master;
45+
update t set b=b+1 where a=3;
46+
include/save_master_gtid.inc
47+
connection slave;
48+
include/sync_with_master_gtid.inc
49+
include/diff_tables.inc [master:test.t,slave:test.t]
50+
#
51+
# Ensure data can be updated in offspring table after
52+
# ALTER TABLE .. CONVERT PARTITION .. TO TABLE
53+
connection master;
54+
update offspring set b=b+1 where a=4;
55+
include/save_master_gtid.inc
56+
connection slave;
57+
include/sync_with_master_gtid.inc
58+
include/diff_tables.inc [master:test.offspring,slave:test.offspring]
59+
#
60+
# Ensure data can be deleted in existing table after
61+
# ALTER TABLE .. CONVERT PARTITION .. TO TABLE
62+
connection master;
63+
delete from t;
64+
include/save_master_gtid.inc
65+
connection slave;
66+
include/sync_with_master_gtid.inc
67+
include/diff_tables.inc [master:test.t,slave:test.t]
68+
#
69+
# Ensure data can be deleted in offspring table after
70+
# ALTER TABLE .. CONVERT PARTITION .. TO TABLE
71+
connection master;
72+
delete from offspring;
73+
include/save_master_gtid.inc
74+
connection slave;
75+
include/sync_with_master_gtid.inc
76+
include/diff_tables.inc [master:test.offspring,slave:test.offspring]
77+
connection master;
78+
drop table t, offspring;
79+
include/rpl_end.inc
80+
# End of rpl_alter_convert_partition

‎mysql-test/suite/rpl/t/rpl_alter_convert_partition.test‎

Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
#
2+
# This test ensures that ALTER TABLE ... CONVERT PARTITION ... TO TABLE
3+
# works with replication. I.e., the partitioning is done correctly, and
4+
# after partitioning, both tables can be updated correctly.
5+
#
6+
# References:
7+
# MDEV-36906: RBR crashes upon DML after CONVERT PARTITION
8+
#
9+
10+
--source include/have_innodb.inc
11+
--source include/have_partition.inc
12+
--source include/master-slave.inc
13+
14+
15+
--echo #
16+
--echo # Ensure initial CREATE TABLE with partitioned data is replicated
17+
--echo # correctly
18+
--connection master
19+
create table t (a int, b int, key(a)) engine=innodb partition by range (b) (partition p1 values less than (10), partition pn values less than (maxvalue));
20+
insert into t values (1,5),(2,100);
21+
--source include/save_master_gtid.inc
22+
--connection slave
23+
--source include/sync_with_master_gtid.inc
24+
--let $diff_tables=master:test.t,slave:test.t
25+
26+
--source include/diff_tables.inc
27+
28+
--echo #
29+
--echo # Ensure ALTER TABLE .. CONVERT PARTITION .. TO TABLE replicates
30+
--echo # correctly
31+
--connection master
32+
alter table t convert partition p1 to table offspring;
33+
--source include/save_master_gtid.inc
34+
--connection slave
35+
--source include/sync_with_master_gtid.inc
36+
--let $diff_tables=master:test.t,slave:test.t
37+
--source include/diff_tables.inc
38+
--let $diff_tables=master:test.offspring,slave:test.offspring
39+
--source include/diff_tables.inc
40+
41+
42+
--echo #
43+
--echo # Ensure data can be inserted into existing table after
44+
--echo # ALTER TABLE .. CONVERT PARTITION .. TO TABLE
45+
--connection master
46+
insert into t values (3, 6);
47+
--source include/save_master_gtid.inc
48+
--connection slave
49+
--source include/sync_with_master_gtid.inc
50+
--let $diff_tables=master:test.t,slave:test.t
51+
--source include/diff_tables.inc
52+
53+
54+
--echo #
55+
--echo # Ensure data can be inserted into offspring table after
56+
--echo # ALTER TABLE .. CONVERT PARTITION .. TO TABLE
57+
--connection master
58+
insert into offspring values (4, 101);
59+
--source include/save_master_gtid.inc
60+
--connection slave
61+
--source include/sync_with_master_gtid.inc
62+
--let $diff_tables=master:test.offspring,slave:test.offspring
63+
--source include/diff_tables.inc
64+
65+
66+
--echo #
67+
--echo # Ensure data can be updated in existing table after
68+
--echo # ALTER TABLE .. CONVERT PARTITION .. TO TABLE
69+
--connection master
70+
update t set b=b+1 where a=3;
71+
--source include/save_master_gtid.inc
72+
--connection slave
73+
--source include/sync_with_master_gtid.inc
74+
--let $diff_tables=master:test.t,slave:test.t
75+
--source include/diff_tables.inc
76+
77+
78+
--echo #
79+
--echo # Ensure data can be updated in offspring table after
80+
--echo # ALTER TABLE .. CONVERT PARTITION .. TO TABLE
81+
--connection master
82+
update offspring set b=b+1 where a=4;
83+
--source include/save_master_gtid.inc
84+
--connection slave
85+
--source include/sync_with_master_gtid.inc
86+
--let $diff_tables=master:test.offspring,slave:test.offspring
87+
--source include/diff_tables.inc
88+
89+
90+
--echo #
91+
--echo # Ensure data can be deleted in existing table after
92+
--echo # ALTER TABLE .. CONVERT PARTITION .. TO TABLE
93+
--connection master
94+
delete from t;
95+
--source include/save_master_gtid.inc
96+
--connection slave
97+
--source include/sync_with_master_gtid.inc
98+
--let $diff_tables=master:test.t,slave:test.t
99+
--source include/diff_tables.inc
100+
101+
102+
--echo #
103+
--echo # Ensure data can be deleted in offspring table after
104+
--echo # ALTER TABLE .. CONVERT PARTITION .. TO TABLE
105+
--connection master
106+
delete from offspring;
107+
--source include/save_master_gtid.inc
108+
--connection slave
109+
--source include/sync_with_master_gtid.inc
110+
--let $diff_tables=master:test.offspring,slave:test.offspring
111+
--source include/diff_tables.inc
112+
113+
114+
--connection master
115+
drop table t, offspring;
116+
--source include/rpl_end.inc
117+
--echo # End of rpl_alter_convert_partition

‎sql/sql_partition.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7626,7 +7626,7 @@ uint fast_alter_partition_table(THD *thd, TABLE *table,
76267626
ERROR_INJECT("convert_partition_1") ||
76277627
write_log_drop_shadow_frm(lpt) ||
76287628
ERROR_INJECT("convert_partition_2") ||
7629-
mysql_write_frm(lpt, WFRM_WRITE_SHADOW) ||
7629+
mysql_write_frm(lpt, WFRM_WRITE_SHADOW|WFRM_ALTER_INFO_PREPARED) ||
76307630
ERROR_INJECT("convert_partition_3") ||
76317631
wait_while_table_is_used(thd, table, HA_EXTRA_NOT_USED) ||
76327632
ERROR_INJECT("convert_partition_4") ||

‎sql/sql_table.cc‎

Lines changed: 39 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -721,16 +721,30 @@ uint build_table_shadow_filename(char *buff, size_t bufflen,
721721
lpt Struct carrying many parameters needed for this
722722
method
723723
flags Flags as defined below
724-
WFRM_INITIAL_WRITE If set we need to prepare table before
725-
creating the frm file
726-
WFRM_INSTALL_SHADOW If set we should install the new frm
727-
WFRM_KEEP_SHARE If set we know that the share is to be
724+
WFRM_WRITE_SHADOW If set, we need to prepare the table before
725+
creating the frm file. Note it is possible that
726+
mysql_write_frm was already called with
727+
WFRM_WRITE_CONVERTED_TO, which would have
728+
already called mysql_prepare_create_table, in
729+
which case, we can skip that specific step in
730+
the preparation.
731+
WFRM_INSTALL_SHADOW If set, we should install the new frm
732+
WFRM_KEEP_SHARE If set, we know that the share is to be
728733
retained and thus we should ensure share
729734
object is correct, if not set we don't
730735
set the new partition syntax string since
731736
we know the share object is destroyed.
732-
WFRM_PACK_FRM If set we should pack the frm file and delete
733-
the frm file
737+
WFRM_WRITE_CONVERTED_TO Similar to WFRM_WRITE_SHADOW but for
738+
ALTER TABLE ... CONVERT PARTITION .. TO TABLE,
739+
i.e., we need to prepare the table before
740+
creating the frm file. Though in this case,
741+
mysql_write_frm will be called again with
742+
WFRM_WRITE_SHADOW, where the
743+
prepare_create_table step will be skipped.
744+
WFRM_BACKUP_ORIGINAL If set, will back up the existing frm file
745+
before creating the new frm file.
746+
WFRM_ALTER_INFO_PREPARED If set, the prepare_create_table step should be
747+
skipped when WFRM_WRITE_SHADOW is set.
734748
735749
RETURN VALUES
736750
TRUE Error
@@ -775,7 +789,15 @@ bool mysql_write_frm(ALTER_PARTITION_PARAM_TYPE *lpt, uint flags)
775789
strxmov(shadow_frm_name, shadow_path, reg_ext, NullS);
776790
if (flags & WFRM_WRITE_SHADOW)
777791
{
778-
if (mysql_prepare_create_table(lpt->thd, lpt->create_info, lpt->alter_info,
792+
/*
793+
It is possible mysql_prepare_create_table was already called in our
794+
create/alter_info context and we don't need to call it again. That is, if
795+
in the context of `ALTER TABLE ... CONVERT PARTITION .. TO TABLE` then
796+
mysql_prepare_create_table would have already been called through a prior
797+
invocation of mysql_write_frm with flag MFRM_WRITE_CONVERTED_TO.
798+
*/
799+
if (!(flags & WFRM_ALTER_INFO_PREPARED) &&
800+
mysql_prepare_create_table(lpt->thd, lpt->create_info, lpt->alter_info,
779801
&lpt->db_options, lpt->table->file,
780802
&lpt->key_info_buffer, &lpt->key_count,
781803
C_ALTER_TABLE))
@@ -850,6 +872,11 @@ bool mysql_write_frm(ALTER_PARTITION_PARAM_TYPE *lpt, uint flags)
850872
ERROR_INJECT("create_before_create_frm"))
851873
DBUG_RETURN(TRUE);
852874

875+
/*
876+
For WFRM_WRITE_CONVERTED_TO, we always need to call
877+
mysql_prepare_create_table
878+
*/
879+
DBUG_ASSERT(!(flags & WFRM_ALTER_INFO_PREPARED));
853880
if (mysql_prepare_create_table(thd, create_info, lpt->alter_info,
854881
&lpt->db_options, file,
855882
&lpt->key_info_buffer, &lpt->key_count,
@@ -3004,6 +3031,11 @@ my_bool init_key_info(THD *thd, Alter_info *alter_info,
30043031

30053032
for (Key &key: alter_info->key_list)
30063033
{
3034+
/*
3035+
Ensure we aren't re-initializing keys that were already initialized.
3036+
*/
3037+
DBUG_ASSERT(!key.length);
3038+
30073039
if (key.type == Key::FOREIGN_KEY)
30083040
continue;
30093041

‎sql/sql_table.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,13 @@ enum enum_explain_filename_mode
5555
/* depends on errmsg.txt Database `db`, Table `t` ... */
5656
#define EXPLAIN_FILENAME_MAX_EXTRA_LENGTH 63
5757

58+
/* See mysql_write_frm function comment for explanations of these flags */
5859
#define WFRM_WRITE_SHADOW 1
5960
#define WFRM_INSTALL_SHADOW 2
6061
#define WFRM_KEEP_SHARE 4
6162
#define WFRM_WRITE_CONVERTED_TO 8
6263
#define WFRM_BACKUP_ORIGINAL 16
64+
#define WFRM_ALTER_INFO_PREPARED 32
6365

6466
/* Flags for conversion functions. */
6567
static const uint FN_FROM_IS_TMP= 1 << 0;

0 commit comments

Comments
 (0)