From 7d23b339075889e59919158bc73b6e4a1493b989 Mon Sep 17 00:00:00 2001
From: Aleksandr Lyapunov <alyapunov@tarantool.org>
Date: Wed, 2 Aug 2023 15:59:07 +0300
Subject: [PATCH] box: forbid foreign keys for incompatible temp/local spaces
There must be a couple of rules:
* foreign key from non-temporary space to temporary space must be
forbidden since after restart all existing links will be broken.
* foreign key from non-local space to local space must be forbidden
on any replica all existing can be broken.
This patch implements the rules.
Closes #8936
NO_DOC=bugfix
---
...6-foreign-keys-with-temporary-and-local.md | 5 +
src/box/alter.cc | 50 ++++
src/box/tuple_constraint_fkey.c | 30 +++
..._8936_foreign_key_wrong_reference_test.lua | 217 ++++++++++++++++++
4 files changed, 302 insertions(+)
create mode 100644 changelogs/unreleased/gh-8936-foreign-keys-with-temporary-and-local.md
create mode 100644 test/engine-luatest/gh_8936_foreign_key_wrong_reference_test.lua
diff --git a/changelogs/unreleased/gh-8936-foreign-keys-with-temporary-and-local.md b/changelogs/unreleased/gh-8936-foreign-keys-with-temporary-and-local.md
new file mode 100644
index 0000000000..7ba758a069
--- /dev/null
+++ b/changelogs/unreleased/gh-8936-foreign-keys-with-temporary-and-local.md
@@ -0,0 +1,5 @@
+## bugfix/core
+
+* Now foreign keys from non-temporary to temporary and from non-local to local
+ spaces are prohibited since they can potentially break foreign key consistency
+ (gh-8936).
diff --git a/src/box/alter.cc b/src/box/alter.cc
index f8784da6ed..036bbdd1ad 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -1988,6 +1988,52 @@ space_check_truncate(struct space *space)
return 0;
}
+/**
+ * Check whether @a old_space holders prohibit alter to @a new_space_def.
+ * For example if the space becomes temporary, there can be foreign keys
+ * from non-temporary space, so this alter must not be allowed.
+ * Return 0 if allowed, or -1 if not allowed (diag is set).
+ */
+static int
+space_check_alter(struct space *old_space, struct space_def *new_space_def)
+{
+ /*
+ * group_id, which is currently used for defining local spaces, is
+ * now can't be changed; if it could, an additional check would be
+ * required below.
+ */
+ assert(old_space->def->opts.group_id == new_space_def->opts.group_id);
+ /* Only alter from non-temporary to temporary can cause problems. */
+ if (old_space->def->opts.is_temporary ||
+ !new_space_def->opts.is_temporary)
+ return 0;
+ /* Check for foreign keys that refers to this space. */
+ struct space_cache_holder *h;
+ rlist_foreach_entry(h, &old_space->space_cache_pin_list, link) {
+ if (h->selfpin)
+ continue;
+ if (h->type != SPACE_HOLDER_FOREIGN_KEY)
+ continue;
+ struct tuple_constraint *constr =
+ container_of(h, struct tuple_constraint,
+ space_cache_holder);
+ struct space *other_space = constr->space;
+ /*
+ * If the referring space is temporary too then the alter
+ * can't break foreign key consistency after restart.
+ */
+ if (other_space->def->opts.is_temporary)
+ continue;
+ diag_set(ClientError, ER_ALTER_SPACE,
+ space_name(old_space),
+ tt_sprintf("foreign key '%s' from non-temporary space"
+ " '%s' can't refer to temporary space",
+ constr->def.name, space_name(other_space)));
+ return -1;
+ }
+ return 0;
+}
+
/**
* A trigger which is invoked on replace in a data dictionary
* space _space.
@@ -2287,6 +2333,10 @@ on_replace_dd_space(struct trigger * /* trigger */, void *event)
"view");
return -1;
}
+
+ if (space_check_alter(old_space, def) != 0)
+ return -1;
+
/*
* Allow change of space properties, but do it
* in WAL-error-safe mode.
diff --git a/src/box/tuple_constraint_fkey.c b/src/box/tuple_constraint_fkey.c
index 03e20e4953..8d7f0d7d28 100644
--- a/src/box/tuple_constraint_fkey.c
+++ b/src/box/tuple_constraint_fkey.c
@@ -619,6 +619,33 @@ tuple_constraint_fkey_destroy(struct tuple_constraint *constr)
constr->space = NULL;
}
+/**
+ * Check that spaces @a space and @a foreign_space are compatible with
+ * the foreign key constraint @a constr.
+ * Return 0 on success, return -1 and set diag in case of error.
+ */
+static int
+tuple_constraint_fkey_check_spaces(struct tuple_constraint *constr,
+ struct space *space,
+ struct space *foreign_space)
+{
+ if (space_is_temporary(foreign_space) && !space_is_temporary(space)) {
+ diag_set(ClientError, ER_CREATE_FOREIGN_KEY,
+ constr->def.name, constr->space->def->name,
+ "foreign key from non-temporary space"
+ " can't refer to temporary space");
+ return -1;
+ }
+ if (space_is_local(foreign_space) && !space_is_local(space)) {
+ diag_set(ClientError, ER_CREATE_FOREIGN_KEY,
+ constr->def.name, constr->space->def->name,
+ "foreign key from non-local space"
+ " can't refer to local space");
+ return -1;
+ }
+ return 0;
+}
+
int
tuple_constraint_fkey_init(struct tuple_constraint *constr,
struct space *space, int32_t field_no)
@@ -637,6 +664,9 @@ tuple_constraint_fkey_init(struct tuple_constraint *constr,
enum space_cache_holder_type type = SPACE_HOLDER_FOREIGN_KEY;
if (foreign_space != NULL) {
/* Space was found, use it. */
+ if (tuple_constraint_fkey_check_spaces(constr, space,
+ foreign_space) != 0)
+ return -1;
space_cache_pin(foreign_space, &constr->space_cache_holder,
tuple_constraint_fkey_space_cache_on_replace,
type, fkey_same_space);
diff --git a/test/engine-luatest/gh_8936_foreign_key_wrong_reference_test.lua b/test/engine-luatest/gh_8936_foreign_key_wrong_reference_test.lua
new file mode 100644
index 0000000000..1f2a8bfc20
--- /dev/null
+++ b/test/engine-luatest/gh_8936_foreign_key_wrong_reference_test.lua
@@ -0,0 +1,217 @@
+-- https://github.com/tarantool/tarantool/issues/8936
+-- Test foreign keys to temporary and local spaces.
+local server = require('luatest.server')
+local t = require('luatest')
+
+-- The test creates two spaces - 'country' and 'city' which are linked with
+-- foreign key - city has country_id field that is linked to country space.
+local test_opts = t.helpers.matrix{
+ engine = {'memtx', 'vinyl'},
+ -- Value of country space option 'temporary' or 'is_local' (depending on
+ -- test case).
+ country_variant = {false, true},
+ -- Value of city space option 'temporary' or 'is_local' (depending on
+ -- test case).
+ city_variant = {false, true},
+}
+local g = t.group('gh-8936-foreign-key-wrong-reference-test', test_opts)
+
+g.before_all(function(cg)
+ cg.server = server:new({alias = 'master'})
+ cg.server:start()
+end)
+
+g.after_all(function(cg)
+ cg.server:stop()
+ cg.server = nil
+end)
+
+g.after_each(function(cg)
+ cg.server:exec(function()
+ if box.space.city then
+ box.space.city:drop()
+ end
+ if box.space.country then
+ box.space.country:drop()
+ end
+ end)
+end)
+
+-- Foreign key must not refer to temporary space from normal space.
+g.test_field_foreign_key_temporary = function(cg)
+ local engine = cg.params.engine
+ local country_is_temporary = cg.params.country_variant
+ local city_is_temporary = cg.params.city_variant
+ -- vinyl space can't be temporary.
+ t.skip_if(engine == 'vinyl')
+
+ cg.server:exec(function(engine, country_is_temporary, city_is_temporary)
+ -- foreign key must not point for non-temporary to temporary space.
+ local must_be_prohibited =
+ country_is_temporary and not city_is_temporary
+
+ local country_fmt = {
+ {name = 'id', type = 'unsigned'},
+ {name = 'name', type = 'string'},
+ }
+ local country_opts = {engine = engine, format = country_fmt,
+ temporary = country_is_temporary}
+
+ local city_fmt = {
+ {name = 'id', type = 'unsigned'},
+ {name = 'country_id', type = 'unsigned',
+ foreign_key = {space = 'country', field = 'id'}},
+ {name = 'name', type = 'string'},
+ }
+ local city_opts = {engine = engine, format = city_fmt,
+ temporary = city_is_temporary}
+
+ local country = box.schema.create_space('country', country_opts)
+ country:create_index('pk')
+
+ if must_be_prohibited then
+ t.assert_error_msg_content_equals(
+ "Failed to create foreign key 'country' in space 'city': " ..
+ "foreign key from non-temporary space " ..
+ "can't refer to temporary space",
+ box.schema.create_space, 'city', city_opts
+ )
+ return nil
+ end
+
+ local city = box.schema.create_space('city', city_opts)
+ city:create_index('pk')
+
+ -- Alter to wrong state must be prohibited while all other alters
+ -- must be allowed.
+ if country_is_temporary and city_is_temporary then
+ t.assert_error_msg_content_equals(
+ "Failed to create foreign key 'country' in space 'city': " ..
+ "foreign key from non-temporary space " ..
+ "can't refer to temporary space",
+ city.alter, city, {temporary = false}
+ )
+ country:alter{temporary = false}
+ country:alter{temporary = true}
+ end
+ if not country_is_temporary and not city_is_temporary then
+ t.assert_error_msg_content_equals(
+ "Can't modify space 'country': foreign key 'country' from " ..
+ "non-temporary space 'city' can't refer to temporary space",
+ country.alter, country, {temporary = true}
+ )
+ city:alter{temporary = true}
+ city:alter{temporary = false}
+ end
+ t.assert_equals(country_is_temporary, country.temporary)
+ t.assert_equals(city_is_temporary, city.temporary)
+
+ -- Check that foreign key still works as expected
+ t.assert_error_msg_content_equals(
+ "Foreign key constraint 'country' failed for field " ..
+ "'2 (country_id)': foreign tuple was not found",
+ city.replace, city, {1, 1, 'msk'}
+ )
+ country:replace{1, 'ru'}
+ city:replace{1, 1, 'msk'}
+ t.assert_error_msg_content_equals(
+ "Foreign key 'country' integrity check failed: index was not found",
+ country.delete, country, {1}
+ )
+ city:create_index('sk', {parts = {{'country_id'}}, unique = false})
+ t.assert_error_msg_content_equals(
+ "Foreign key 'country' integrity check failed: tuple is referenced",
+ country.delete, country, {1}
+ )
+ t.assert_error_msg_content_equals(
+ "Can't modify space 'country': space is referenced by foreign key",
+ country.truncate, country
+ )
+ t.assert_error_msg_content_equals(
+ "Can't modify space 'country': space is referenced by foreign key",
+ country.drop, country
+ )
+
+ end, {engine, country_is_temporary, city_is_temporary})
+end
+
+-- Foreign key must not refer to local space from non-local space.
+g.test_field_foreign_key_local = function(cg)
+ local engine = cg.params.engine
+ local country_is_local = cg.params.country_variant
+ local city_is_local = cg.params.city_variant
+
+ cg.server:exec(function(engine, country_is_local, city_is_local)
+ -- foreign key must not point for non-temporary to temporary space.
+ local must_be_prohibited =
+ country_is_local and not city_is_local
+
+ local country_fmt = {
+ {name = 'id', type = 'unsigned'},
+ {name = 'name', type = 'string'},
+ }
+ local country_opts = {engine = engine, format = country_fmt,
+ is_local = country_is_local}
+
+ local city_fmt = {
+ {name = 'id', type = 'unsigned'},
+ {name = 'country_id', type = 'unsigned',
+ foreign_key = {space = 'country', field = 'id'}},
+ {name = 'name', type = 'string'},
+ }
+ local city_opts = {engine = engine, format = city_fmt,
+ is_local = city_is_local}
+
+ local country = box.schema.create_space('country', country_opts)
+ country:create_index('pk')
+
+ if must_be_prohibited then
+ t.assert_error_msg_content_equals(
+ "Failed to create foreign key 'country' in space 'city': " ..
+ "foreign key from non-local space can't refer to local space",
+ box.schema.create_space, 'city', city_opts
+ )
+ return nil
+ end
+
+ local city = box.schema.create_space('city', city_opts)
+ city:create_index('pk')
+
+ -- Alter of 'is_local' should be prohibited in any case.
+ t.assert_error_msg_content_equals(
+ "Illegal parameters, unexpected option 'is_local'",
+ country.alter, country, {is_local = not country_is_local}
+ )
+ t.assert_error_msg_content_equals(
+ "Illegal parameters, unexpected option 'is_local'",
+ city.alter, city, {is_local = not city_is_local}
+ )
+
+ -- Check that foreign key still works as expected
+ t.assert_error_msg_content_equals(
+ "Foreign key constraint 'country' failed for field " ..
+ "'2 (country_id)': foreign tuple was not found",
+ city.replace, city, {1, 1, 'msk'}
+ )
+ country:replace{1, 'ru'}
+ city:replace{1, 1, 'msk'}
+ t.assert_error_msg_content_equals(
+ "Foreign key 'country' integrity check failed: index was not found",
+ country.delete, country, {1}
+ )
+ city:create_index('sk', {parts = {{'country_id'}}, unique = false})
+ t.assert_error_msg_content_equals(
+ "Foreign key 'country' integrity check failed: tuple is referenced",
+ country.delete, country, {1}
+ )
+ t.assert_error_msg_content_equals(
+ "Can't modify space 'country': space is referenced by foreign key",
+ country.truncate, country
+ )
+ t.assert_error_msg_content_equals(
+ "Can't modify space 'country': space is referenced by foreign key",
+ country.drop, country
+ )
+
+ end, {engine, country_is_local, city_is_local})
+end
--
GitLab