MDEV-19191 Partial support of foreign keys in partitioned tables #3641

midenok · 2024-11-19T16:46:33Z

The patch adds the ability to run foreign keys in partitioned tables with
limitations.

Example:

  create or replace table t1 (id int primary key) engine innodb;
  create or replace table t2 (fk int references t1(id)) engine innodb
  partition by hash (fk) partitions 2;

Limitations:

  1. Foreign keys cannot refer partitioned table even SYSTEM_TIME-partitioned
  still. create_foreign_keys() at InnoDB layer receives Foreign_key about
  referenced table T, but no such table exists in InnoDB layer, only partition
  tables T#P#p0, T#P#p1, ..., T#P#pn. Finding out it is SYSTEM_TIME partitioning
  and current partition at that point is impossible without modification of
  SYS_TABLES or opening TABLE object of referenced table. Both should be avoided
  as this is superseded by MDEV-12483.

  2. CASCADE and SET NULL actions are disabled in partitioned foreign table as
  these actions update row data and this is the subject of row placement into
  another partition but it cannot be done in InnoDB layer. DELETE CASCADE for
  SYSTEM_TIME partitioning requires the row to be moved from current to history
  partition.

The task is basically divided into 3 parts:

  1. Remove prohibiting code, allow FKs to be created for partitions;
  2. Allow partitioned FKs at SQL layer for such functions as SHOW CREATE;
  3. Implement correct handling of FKs when partitioning configuration changes.

1. Remove prohibiting code, allow FKs to be created for partitions

  In SYS_FOREIGN table foreign key records are unique by ID which was taken from
  constraint name or automatically generated. Normally foreign ID and constraint
  name are identical, but that's not the case for partitioned table as InnoDB
  holds foreign keys per dict_table_t object and each partition is a different
  table for InnoDB. So for each foreign key in SQL layer there is a set of foreign
  keys in InnoDB layer per each partition (except SYSTEM_TIME partitioning where
  we keep foreign keys only for current partition). To constitute unique foreign
  ID at InnoDB layer we concatenate constraint name with partition suffix, the one
  what is added to partition table name beginning with #P# and optionally
  containing #SP# for subpartitions. Constraint name and partitioning suffix are
  separated by \xFF character which is the safe character code non-clashing with
  identifier character set.

  When we return back foreign ID to SQL layer this partitioning suffix is stripped
  off the constraint name and SQL output receives the name similar to
  non-partitioned table.

  User may see a bit more truthful version of foreign ID in
  INFORMATION_SCHEMA.INNODB_SYS_FOREIGN with \xFF replaced by ':' and #P# or
  everything starting from #P# and ending by #SP# chopped out. So he may see
  corresponding partition name or subpartition name in ID of INNODB_SYS_FOREIGN.

2. Allow partitioned FKs at SQL layer for such functions as SHOW CREATE

  Through standard handler interface get_foreign_key_list() foreign keys are
  returned to SQL layer. For SYSTEM_TIME partitioning from current partition, for
  any other partitioning from first read-marked partition.

3. Implement correct handling of FKs when partitioning configuration changes

  ALTER operations such as ADD PARTITION, DROP PARTITION, REMOVE PARTITIONING,
  etc. are reflected into correct configuration of foreign keys in InnoDB.
  Handling of foreign key ID for temporary tables in ALTER was done based on
  MDEV-28933.

CLAassistant · 2024-11-19T16:46:39Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

dr-m

Here are some initial review comments.

Something needs to be done about MDEV-33489, because the test atomic.alter_table is timing out on several builders for this pull request.

What does the change 07e23fc have to do with this? It should be submitted as a separate pull request, with as separate MDEV number.

There are InnoDB changes in several commits, with messages that fail to specify why something was changed, or even to describe what was changed. An example is dbdade6. Each change needs to be covered with test cases.

dr-m · 2024-11-20T05:45:28Z

storage/mroonga/ha_mroonga.hpp

@@ -604,7 +604,6 @@ class ha_mroonga: public handler
  int index_last(uchar *buf) mrn_override;
 #endif
  void change_table_ptr(TABLE *table_arg, TABLE_SHARE *share_arg) mrn_override;
-  bool is_fk_defined_on_table_or_index(uint index) mrn_override;


The commit message 7d6cb82 fails to explain why this was removed. Were the mroonga tests run at all after changing this? Can you point us to a build linked from [the grid view of this pull request|https://buildbot.mariadb.org/#/grid?branch=refs%2Fpull%2F3641%2Fmerge] that covers both the Mroonga storage and wrapper tests?

Sorry, I have no idea where Mroonga is tested in buildbot. I tested it by hand:

Completed: All 665 tests were successful.

mtr.log

The interface is removed because it is removed from ha_partition.

dr-m · 2024-11-20T06:01:29Z

mysql-test/main/partition.test

--error ER_FEATURE_NOT_SUPPORTED_WITH_PARTITIONING
 CREATE TABLE t1 (a INT, FOREIGN KEY (a) REFERENCES t0 (a))
 ENGINE=MyISAM
 PARTITION BY HASH (a);
+show create table t1;
+drop table t1;


It is not sufficient to demonstrate that the table can be created. Some functional testing is needed as well, to demonstrate that the foreign keys actually work.

Given that this test uses ENGINE=MyISAM and FOREIGN KEY constraints are currently implemented only in ENGINE=InnoDB, this test seem to demonstrate that things are changing for the worse. We’d allow a DDL operation that was previously rejected because it was known not to work. I don’t think that this change is acceptable.

MyISAM silently ignores foreign keys, that is demonstrated by main.foreign_keys. It was there always. Making FK to work correctly with MyISAM is not the topic of this task.

The functional testing is done in parts.foreign, it was reviewed by @sanja-byelkin

dr-m · 2024-11-20T06:03:21Z

mysql-test/main/partition_innodb.test

--error ER_FEATURE_NOT_SUPPORTED_WITH_PARTITIONING
 ALTER TABLE t1 ADD CONSTRAINT test_ibfk_1 FOREIGN KEY (parent_id) REFERENCES t2 (id);

 ALTER TABLE t1 PARTITION BY HASH (id) PARTITIONS 2;

--error ER_FEATURE_NOT_SUPPORTED_WITH_PARTITIONING
+--error ER_CANT_CREATE_TABLE
 ALTER TABLE t1 ADD CONSTRAINT test_ibfk_1 FOREIGN KEY (parent_id) REFERENCES t2 (id);

 DROP TABLE t1, t2;


Some functional testing with DML statements is needed here to demonstrate that the FOREIGN KEY constraint works after each of these ALTER TABLE operation, including the rejected one.

Please also add a comment here that there are further tests about this in parts.foreign. Initially, I was going to ask for this to be extended to cover ON (UPDATE|DELETE) (CASCADE|SET NULL), but those are covered in that test.

The functional testing is done in parts.foreign, it was reviewed by @sanja-byelkin

This test is for Bug#50104: Partitioned table with just 1 partion works with fk, no point in commenting some bugs test cases, at least for this topic.

dr-m · 2024-11-20T06:05:27Z

mysql-test/main/partition_innodb.result

 ALTER TABLE t1 ADD CONSTRAINT test_ibfk_1 FOREIGN KEY (parent_id) REFERENCES t2 (id);
-ERROR HY000: Partitioned tables do not support FOREIGN KEY
 ALTER TABLE t1 PARTITION BY HASH (id) PARTITIONS 2;
 ALTER TABLE t1 ADD CONSTRAINT test_ibfk_1 FOREIGN KEY (parent_id) REFERENCES t2 (id);
-ERROR HY000: Partitioned tables do not support FOREIGN KEY
+ERROR HY000: Can't create table `test`.`t1` (errno: 150 "Foreign key constraint is incorrectly formed")


The error message looks incorrect here. Shouldn’t this fail because a duplicate constraint name test_ibfk_1? Would it work with a different constraint name?

That is how it works without partitions:

--source include/have_innodb.inc create table t2 ( id int, primary key (id) ) engine=innodb ; create table t1 ( id int not null auto_increment, parent_id int default null, primary key (id), key parent_id (parent_id) ) engine=innodb; alter table t1 add constraint test_ibfk_1 foreign key (parent_id) references t2 (id); alter table t1 add constraint a foreign key (parent_id) references t2 (id); --error ER_CANT_CREATE_TABLE alter table t1 add constraint test_ibfk_1 foreign key (parent_id) references t2 (id); --error ER_CANT_CREATE_TABLE alter table t1 add constraint a foreign key (parent_id) references t2 (id); drop tables t1, t2;

The failure is the same with different name.

Vanilla error message:

ERROR HY000: Can't create table `test`.`t1` (errno: 121 "Duplicate key on write or update")

Patched error message:

ERROR HY000: Can't create table `test`.`t1` (errno: 150 "Foreign key constraint is incorrectly formed")

The correct error message is done by MDEV-16417.

dr-m · 2024-11-20T06:05:50Z

mysql-test/suite/innodb/r/foreign_null,COPY.rdiff

-+++ foreign_null,COPY.result
+++ foreign_null,COPY.reject


Why was this line changed?

It was changed by the script of preparing new rdiff.

dr-m · 2024-11-20T06:06:28Z

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

 alter table t3 add constraint dc foreign key (a) references t1(a);
-ERROR HY000: Can't create table `test`.`t3` (errno: 121 "Duplicate key on write or update")
+ERROR HY000: Duplicate constraint name


The error message fails to include the duplicate constraint name dc.

Yes, like the original one. It is fixed by MDEV-16417. This was reviewed by @sanja-byelkin

dr-m · 2024-11-20T06:07:06Z

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

+select * from information_schema.innodb_sys_foreign_cols;
+show create table u;
+use test;
+eval drop database `$d`;


This is missing an --echo # End of 11.x tests marker.

janlindstrom · 2024-11-20T08:51:51Z

@midenok Unfortunately, I do not see any replication tests or Galera tests that demonstrates this new feature does work. Functional testing seems to focus can you create foreign key on portioned table but fails to test does actual foreign key work and RESTRICT action work.

midenok · 2024-11-29T12:06:45Z

@midenok Unfortunately, I do not see any replication tests or Galera tests that demonstrates this new feature does work.

I'm not sure what can I do about it. DDL is replicated as statement, statement is SQL layer, SQL layer is tested. As far as DML is concerned nothing changed. Since there is nothing new functional in replication the test cases is the subject of bugtesting. I'm going to add some basic demonstration of the feature though.

Functional testing seems to focus can you create foreign key on portioned table but fails to test does actual foreign key work and RESTRICT action work.

This is not true. You may find functional testing in parts.foreign.

When there are GCC-incompatible compiler flags dtrace fails like this: gcc: error: unrecognized command-line option ‘-fno-limit-debug-info’ gcc: error: unrecognized command-line option ‘-mbranches-within-32B-boundaries’ "gcc .dtrace-temp.3fd6bacf.c" failed Usage /usr/bin/dtrace [--help] [-h | -G] [-C [-I<Path>]] -s File.d [-o <File>]

Return true for temporary partitions.

Based on Marko Makela's patch 4a591d4: dict_get_referenced_table(): Let the caller convert names when needed. row_rename_table_for_mysql(): Specify the treatment of FOREIGN KEY constraints in a 4-valued enum parameter. In cases where FOREIGN KEY constraints cannot exist (partitioned tables, or internal tables of FULLTEXT INDEX), we can use the mode RENAME_IGNORE_FK. The mod RENAME_REBUILD is for any DDL operation that rebuilds the table inside InnoDB, such as TRUNCATE and native ALTER TABLE (or OPTIMIZE TABLE). The mode RENAME_ALTER_COPY is used solely during non-native ALTER TABLE in ha_innobase::rename_table(). Normal ha_innobase::rename_table() will use the mode RENAME_FK. CREATE OR REPLACE will rename the old table (if one exists) along with its FOREIGN KEY constraints into a temporary name. The replacement table will be initially created with another temporary name. Unlike in ALTER TABLE, all FOREIGN KEY constraints must be renamed and not inherited as part of these operations, using the mode RENAME_FK. dict_get_referenced_table(): Let the callers convert names when needed. create_table_info_t::create_foreign_keys(): CREATE OR REPLACE creates the replacement table with a temporary name table, so for self-references foreign->referenced_table will be a table with temporary name and charset conversion must be skipped for it.

All InnoDB internal SQL functions should be moved to sql_funcs.h

Use temporary constraint names for temporary tables. The constraints are not added to cache (skipped in dict_table_rename_in_cache()). The scheme for temporary constraint names is as follows: for old table: db_name/\xFFconstraint_name for new table: db_name/\xFF\xFFconstraint_name normalize_table_name_c_low(): wrong comparison "less than FN_REFLEN - 1". Somewhere array of FN_REFLEN includes the trailing 0, somewhere array of FN_REFLEN + 1 includes trailing 0, but nowhere array of FN_REFLEN - 1 must include trailing 0. Change from original MDEV-28933 fix: As temporary FK is now required for any ALTER operation (especially partitioning operations) row_rename_table_for_mysql() does FK rename on both RENAME_FK and RENAME_ALTER_COPY (see do_rename_fk).

Unused code, assertions, comments, etc

The patch adds the ability to run foreign keys in partitioned tables with limitations. Example: create or replace table t1 (id int primary key) engine innodb; create or replace table t2 (fk int references t1(id)) engine innodb partition by hash (fk) partitions 2; Limitations: 1. Foreign keys cannot refer partitioned table even SYSTEM_TIME-partitioned still. create_foreign_keys() at InnoDB layer receives Foreign_key about referenced table T, but no such table exists in InnoDB layer, only partition tables T#P#p0, T#P#p1, ..., T#P#pn. Finding out it is SYSTEM_TIME partitioning and current partition at that point is impossible without modification of SYS_TABLES or opening TABLE object of referenced table. Both should be avoided as this is superseded by MDEV-12483. 2. CASCADE and SET NULL actions are disabled in partitioned foreign table as these actions update row data and this is the subject of row placement into another partition but it cannot be done in InnoDB layer. DELETE CASCADE for SYSTEM_TIME partitioning requires the row to be moved from current to history partition. The task is basically divided into 3 parts: 1. Remove prohibiting code, allow FKs to be created for partitions; 2. Allow partitioned FKs at SQL layer for such functions as SHOW CREATE; 3. Implement correct handling of FKs when partitioning configuration changes. 1. Remove prohibiting code, allow FKs to be created for partitions In SYS_FOREIGN table foreign key records are unique by ID which was taken from constraint name or automatically generated. Normally foreign ID and constraint name are identical, but that's not the case for partitioned table as InnoDB holds foreign keys per dict_table_t object and each partition is a different table for InnoDB. So for each foreign key in SQL layer there is a set of foreign keys in InnoDB layer per each partition (except SYSTEM_TIME partitioning where we keep foreign keys only for current partition). To constitute unique foreign ID at InnoDB layer we concatenate constraint name with partition suffix, the one what is added to partition table name beginning with #P# and optionally containing #SP# for subpartitions. Constraint name and partitioning suffix are separated by \xFF character which is the safe character code non-clashing with identifier character set. When we return back foreign ID to SQL layer this partitioning suffix is stripped off the constraint name and SQL output receives the name similar to non-partitioned table. User may see a bit more truthful version of foreign ID in INFORMATION_SCHEMA.INNODB_SYS_FOREIGN with \xFF replaced by ':' and #P# or everything starting from #P# and ending by #SP# chopped out. So he may see corresponding partition name or subpartition name in ID of INNODB_SYS_FOREIGN. 2. Allow partitioned FKs at SQL layer for such functions as SHOW CREATE Through standard handler interface get_foreign_key_list() foreign keys are returned to SQL layer. For SYSTEM_TIME partitioning from current partition, for any other partitioning from first read-marked partition. 3. Implement correct handling of FKs when partitioning configuration changes ALTER operations such as ADD PARTITION, DROP PARTITION, REMOVE PARTITIONING, etc. are reflected into correct configuration of foreign keys in InnoDB. Handling of foreign key ID for temporary tables in ALTER was done based on MDEV-28933.

This commit is not intended to be pushed into main branch. Its mission is to simplify the review process. The comments here are in different style: // and without indentation. The reviewer may ask for some of them to be included into the task code. In that case they will be reformatted and moved to the task commit.

Related to FIXME here. Now check_bulk_buffer() returns false. 3389 if (auto t= trx->check_bulk_buffer(index->table)) { 3390 /* MDEV-25036 FIXME: 3391 row_ins_check_foreign_constraint() check 3392 should be done before buffering the insert 3393 operation. */ 3394 ut_ad(index->table->skip_alter_undo 3395 || !trx->check_foreigns); 3396 return t->bulk_insert_buffered(*entry, *index, trx); 3397 }

midenok · 2024-11-29T12:27:40Z

Something needs to be done about MDEV-33489, because the test atomic.alter_table is timing out on several builders for this pull request.

Why are you requesting MDEV-33489 here?

What does the change 07e23fc have to do with this? It should be submitted as a separate pull request, with as separate MDEV number.

This is too short to be the separate ticket. @sanja-byelkin approved it.

There are InnoDB changes in several commits, with messages that fail to specify why something was changed, or even to describe what was changed. An example is dbdade6. Each change needs to be covered with test cases.

Pure refactoring doesn't change test cases. You may propose your version of comments if you find something unclear.

midenok requested a review from dr-m November 19, 2024 16:46

midenok requested a review from sanja-byelkin November 19, 2024 16:48

dr-m mentioned this pull request Nov 20, 2024

MDEV-19191 Partial support of foreign keys in partitioned tables #3513

Closed

dr-m requested changes Nov 20, 2024

View reviewed changes

cvicentiu added the MariaDB Corporation label Nov 26, 2024

midenok added 11 commits November 29, 2024 15:20

MDEV-12483 create_foreign_key() split

5389d62

is_partition(), is_temporary_name() fix

0313748

Return true for temporary partitions.

MDEV-28933 Moved RENAME_CONSTRAINT_IDS to include/sql_funcs.h

96edaa5

All InnoDB internal SQL functions should be moved to sql_funcs.h

MDEV-19191 Code cleanups

20aa0f8

Unused code, assertions, comments, etc

LIST PARTITIONING: fix wrong value in error message

4214369

midenok force-pushed the bb-main-midenok-MDEV-19191 branch from 7d42eee to b84d55b Compare November 29, 2024 12:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MDEV-19191 Partial support of foreign keys in partitioned tables #3641

MDEV-19191 Partial support of foreign keys in partitioned tables #3641

midenok commented Nov 19, 2024

CLAassistant commented Nov 19, 2024

dr-m left a comment

dr-m Nov 20, 2024

midenok Nov 29, 2024

dr-m Nov 20, 2024

midenok Nov 29, 2024

dr-m Nov 20, 2024

midenok Nov 29, 2024

dr-m Nov 20, 2024

midenok Nov 29, 2024 •

edited

Loading

dr-m Nov 20, 2024

midenok Nov 29, 2024

dr-m Nov 20, 2024

midenok Nov 29, 2024

dr-m Nov 20, 2024

midenok Nov 29, 2024

janlindstrom commented Nov 20, 2024

midenok commented Nov 29, 2024

midenok commented Nov 29, 2024

MDEV-19191 Partial support of foreign keys in partitioned tables #3641

Are you sure you want to change the base?

MDEV-19191 Partial support of foreign keys in partitioned tables #3641

Conversation

midenok commented Nov 19, 2024

CLAassistant commented Nov 19, 2024

dr-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

midenok Nov 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janlindstrom commented Nov 20, 2024

midenok commented Nov 29, 2024

midenok commented Nov 29, 2024

midenok Nov 29, 2024 •

edited

Loading