midenok, this task will now be moved to 10.6 as it, and the task MDEV-16417, cannot be finished for 10.5 anymore. This has been agreed with Sergei Golubchik

Please review bb-10.5-midenok-MDEV-16417

The branch is several hundred commits behind the latest 10.5, and an attempt to merge will flag several conflicted files:

	both modified:   sql/sql_string.h

	both modified:   sql/sql_table.cc

	both modified:   sql/table.cc

	both modified:   sql/unireg.cc

	both modified:   storage/innobase/handler/ha_innodb.cc

I will merge 10.5 to 10.6, so that you can rebase the work to an up-to-date 10.6.

Updated branch is bb-10.6-midenok-MDEV-16417

Sorry, this review was stalled for a long time. I see that there will be conflicts with the latest 10.6. Please try to rebase this to an up-to-date version soon. I think that proper testing of this will be blocked by MDEV-17567.

If serg agrees that the functional change of disallowing self-references is acceptable, I do not mind if we disallow that. But I will not authorize that change myself.

There is an error in btr_node_ptr_max_size(): if (!comp && table_name_is_SYS_FOREIGN) was wrongly replaced with if (!comp). If we declared that on upgrade, all tables will lose their FOREIGN KEY constraints, then the correct course of action would have been to remove the entire if statement and its body.

Because we intend to support ugprades (MDEV-21652), that statement must be retained in its entirety for now. Until all tables have been upgraded to the new FOREIGN KEY metadata storage, the old metadata tables SYS_FOREIGN and SYS_FOREIGN_COLS may be accessed, without reintroducing MDEV-14637. Only in some point of future (MariaDB 10.8? 10.9?) when we are ready to remove the support for upgrade, all references to the tables can be removed.

Loading the foreign key metadata should be controlled by the SQL layer and not the storage engine. Until now, the reason why InnoDB was loading the FOREIGN KEY metadata was that it was responsible for the persistent storage. Now that the metadata is managed by the SQL layer, the SQL layer should be responsible for loading it, instead of the storage engine.

Yes, until MDEV-22361, InnoDB must cache the foreign key metadata. To facilitate that, I think that we will need a handler API call to register any discovered foreign key metadata with the storage engine. In MDEV-22361 that API would be removed, along with any FOREIGN KEY processing code in InnoDB.

With this, I am saying that a replacement of the function dict_load_foreigns() must be introduced in the SQL layer, and it must be called from the SQL layer, not any storage engine. To avoid losing FOREIGN KEY metadata on upgrade, we will need a handlerton function to let InnoDB load metadata in case no metadata exists in the SQL layer and the .frm file is old enough to not determine whether foreign keys exist.

Currently, the rewritten function dict_load_foreigns() is repeatedly calling current_thd, which is not acceptable. Why does it not take THD* as a parameter? The function is formatted in a style that follows neither the common server formatting rules nor the old InnoDB style that we should not use for any new code. Many lines exceed 79 characters. The function seems to blindly assume that tables are not partitioned.

Other than this, the commit is mostly removing code from InnoDB, which is fine.

Mandatory changes from Marko's review:

1. There is an error in btr_node_ptr_max_size(): if (!comp && table_name_is_SYS_FOREIGN) was wrongly replaced with if (!comp). If we declared that on upgrade, all tables will lose their FOREIGN KEY constraints, then the correct course of action would have been to remove the entire if statement and its body. Fixed

2. Currently, the rewritten function dict_load_foreigns() is repeatedly calling current_thd, which is not acceptable. Why does it not take THD* as a parameter? Fixed

3. The function is formatted in a style that follows neither the common server formatting rules nor the old InnoDB style that we should not use for any new code. Many lines exceed 79 characters. Fixed

4. The function seems to blindly assume that tables are not partitioned. Fixed

marko Please complement this list if I missed something.

The test innodb.innodb_bug60049 is failing massively. Please, also fix the compiler warnings before requesting another review:

/buildbot/amd64-ubuntu-1804/build/storage/innobase/btr/btr0cur.cc:1122:5: warning: this statement may fall through [-Wimplicit-fallthrough=]

and another type of warnings that occurs pretty frequently (CMAKE_BUILD_TYPE=RelWithDebInfo using clang++-11):

sql/sql_table.cc:12256:7: warning: expression result unused [-Wunused-value]

  if (ERROR_INJECT("fail_fk_alter_3", "crash_fk_alter_3"))

      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

sql/sql_base.h:664:57: note: expanded from macro 'ERROR_INJECT'

  DBUG_EVALUATE_IF(code_crash, (DBUG_SUICIDE(), false), false), \

                                                        ^~~~~

I think that the following code (which I understand is supposed to exist only for test purposes in special debug builds) should be disabled by default:

OPTION(WITH_INNODB_LEGACY_FOREIGN_STORAGE "Create SYS_FOREIGN, SYS_FOREIGN_COLS tables" ON)

IF (WITH_INNODB_LEGACY_FOREIGN_STORAGE)

  ADD_DEFINITIONS(-DWITH_INNODB_LEGACY_FOREIGN_STORAGE)

ENDIF()

Enabling that code by default undermines the purpose of the task, which is to phase out the InnoDB internal metadata storage for FOREIGN KEY constraints.

I would like to see a test case that exercises the upgrade logic. The following failed for me when I disabled the above:

--source include/have_innodb.inc

CREATE TABLE parent(a INT PRIMARY KEY) ENGINE=InnoDB;

CREATE TABLE child(a INT PRIMARY KEY REFERENCES parent(a)) ENGINE=InnoDB;

CREATE TABLE father(a INT PRIMARY KEY) ENGINE=InnoDB;

CREATE TABLE son(a INT PRIMARY KEY) ENGINE=InnoDB;

--source include/restart_mysqld.inc

RENAME TABLE parent TO mother, child to daughter;

RENAME TABLE father TO parent, son to child;

SHOW CREATE TABLE daughter;

SHOW CREATE TABLE child;

DROP TABLE daughter,mother,child,parent;

I started the 10.6 server using the following:

./mtr --manual-gdb innodb.fk_upgrade

Then, in the middle of the execution, I switched the executable:

[Inferior 1 (process 1681520) exited normally]

(gdb) file /dev/shm/10.6fk/sql/mariadbd

Load new symbol table from "/dev/shm/10.6fk/sql/mariadbd"? (y or n) y

Reading symbols from /dev/shm/10.6fk/sql/mariadbd...

(gdb) run

First, the test would fail with the following result difference:

@@ -12,6 +12,8 @@

   PRIMARY KEY (`a`),

   CONSTRAINT `daughter_ibfk_1` FOREIGN KEY (`a`) REFERENCES `mother` (`a`)

 ) ENGINE=InnoDB DEFAULT CHARSET=latin1

+Warnings:

+Note	1459	Upgrade required. Please do "REPAIR TABLE `daughter`" or dump/reload to fix it!

 SHOW CREATE TABLE child;

 Table	Create Table

 child	CREATE TABLE `child` (

With a rebuild after setting WITH_INNODB_LEGACY_FOREIGN_STORAGE:BOOL=OFF I got the following result difference (after removing innodb_sys_foreign and innodb_sys_foreign_cols from the startup parameters, to allow the server to start):

@@ -9,8 +9,7 @@

 Table	Create Table

 daughter	CREATE TABLE `daughter` (

   `a` int(11) NOT NULL,

-  PRIMARY KEY (`a`),

-  CONSTRAINT `daughter_ibfk_1` FOREIGN KEY (`a`) REFERENCES `mother` (`a`)

+  PRIMARY KEY (`a`)

 ) ENGINE=InnoDB DEFAULT CHARSET=latin1

 SHOW CREATE TABLE child;

 Table	Create Table

This proves what I suspected based on a quick read of the code: Because the code fails to delete old metadata on DDL operations, bogus FOREIGN KEY relations may be discovered.

I think that the following code (which I understand is supposed to exist only for test purposes in special debug builds) should be disabled by default.

No, this understanding is wrong. This is not for debugging-only, its purpose is described in commit message. Renamed WITH_INNODB_LEGACY_FOREIGN_STORAGE into more friendly WITH_INNODB_FOREIGN_UPGRADE.

I removed the test innodb.innodb_bug60049 because it was no longer testing what it was supposed to test. We have a much better test innodb.purge_secondary for that purpose.

midenok, I am confused. In the other ticket MDEV-21652, which was filed for the upgrade aspect of this, I see the following claims:

1. Drop SYS_FOREIGN[_COLS] when empty; Fixed
2. debug assertion had been commented out; Fixed, I guess

But, even though the branch that I reviewed appears to contain the MDEV-21652 changes, we are always creating the tables SYS_FOREIGN and SYS_FOREIGN_COLS.

Edit: Sorry, I misunderstood; we are checking if those tables exist and making some adjustments when they do exist. The current upgrade almost does what it should. I will post further comments about the upgrade in MDEV-21652. I think that the upgrade must work in a satisfactory way before the rest can be addressed.

The commit message as well as the Description must explain the InnoDB changes in detail. It now is unclear why dict_foreign_add_to_cache() was changed in the way it was.

The function dict_load_table_one() could now probably be merged to dict_load_table().

In ha_innobase::open(), we now invoke dict_load_foreigns(). It looks like we will prevent the eviction of connected tables from dict_sys as long as foreign key constraints exist. So, why do we have to invoke the function on every ha_innobase::open()? And what actually is protecting the dict_table_t in dict_load_foreigns()? It does not look like we are holding dict_sys.mutex or dict_table_t::lock_mutex there.

Any TODO or FIXME comments should be mentioned in the commit message with a plan how to address them.

The commit message as well as the Description must explain the InnoDB changes in detail. It now is unclear why dict_foreign_add_to_cache() was changed in the way it was.

Added an explanation:

The semantics of dict_foreign_add_to_cache() has been changed. Foreigns must be synchronized on every table open and cache must be updated. The old semantics was to keep old foreign while it is in cache.

The function dict_load_table_one() could now probably be merged to dict_load_table().

In ha_innobase::open(), we now invoke dict_load_foreigns(). It looks like we will prevent the eviction of connected tables from dict_sys as long as foreign key constraints exist. So, why do we have to invoke the function on every ha_innobase::open()? And what actually is protecting the dict_table_t in dict_load_foreigns()? It does not look like we are holding dict_sys.mutex or dict_table_t::lock_mutex there.

Actually it is protected by a dict_sys mutex. Added assertion into dict_load_foreigns() and debug test case into innodb.foreign-keys.

Any TODO or FIXME comments should be mentioned in the commit message with a plan how to address them.

I agree on that partially. All FIXMEs must be fixed or planned. Not all TODOs must be planned, only important ones.

Before a review can be conducted, these changes will need to be rebased to the 10.7 branch, once that branch has been properly created so that it includes the changes of the 10.6.3 release.