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 0000000000000000000000000000000000000000..7ba758a069657582eef402f715ccb40aee006014 --- /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 f8784da6ed7b71a37b32507cacc8914cc5bb57ab..036bbdd1ada594043f06522ca7df052232b52c77 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 03e20e4953731b857a6bdd2e36576f45cedbe1fa..8d7f0d7d2840550aa54d1ade544e4451932c4902 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 0000000000000000000000000000000000000000..1f2a8bfc2032e0ad47cda616b3430d198531d193 --- /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