[tarantool-patches] Re: [PATCH 2/5] schema: add new system space for FK constraints

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 18 comments below and a patch on
the branch.

On 13/07/2018 05:04, Nikita Pettik wrote:
> This patch introduces new system space to persist foreign keys

1. Typos: contraints, chils.

> contraints. Format of the space:
> 
> _fk_constraint (space id = 350)
> 
> [<contraint name> STR, <parent id> UINT, <child id> UINT,>   <is deferred> BOOL, <match> STR, <on delete action> STR,
>   <on update action> STR, <field links> ARRAY<MAP< UINT : UINT >>]
> 
> FK constraint is local to space, so every pair <FK name, child id>
> is unique (and it is PK in _fk_constraint space).
> 
> After insertion into this space, new instance describing FK constraint
> is created. FK constraints are held in data-dictionary as two lists
> (for child and parent constraints) in struct space.
> 
> There is a list of FK restrictions:
>   - At the time of FK creation parent and chils spaces must exist;
>   - VIEWs can't be involved into FK processing;
>   - Child space must be empty;
>   - Types of referencing and referenced fields must match;

2. How about not match, but fit? See field_type1_contains_type2() and
its usages.

>   - Collations of referencing and referenced fields must match;
>   - Referenced fields must compose unique index;
> 
> Until space (child) features FK constraints it isn't allowed to be
> dropped. Implicitly referenced index also can't be dropped
> (and that is why parent space can't be dropped). But :drop() method
> of child space firstly deletes all FK constraint (the same as SQL
> triggers, indexes etc) and then removes entry from _space.
> 
> Part of #3271
> ---
>   src/box/CMakeLists.txt         |   1 +
>   src/box/alter.cc               | 432 ++++++++++++++++++++++++++++++++++++++++-
>   src/box/alter.h                |   1 +
>   src/box/bootstrap.snap         | Bin 1704 -> 1798 bytes
>   src/box/errcode.h              |   4 +
>   src/box/fkey.c                 |  69 +++++++
>   src/box/fkey.h                 | 163 ++++++++++++++++
>   src/box/lua/schema.lua         |   6 +
>   src/box/lua/space.cc           |   2 +
>   src/box/lua/upgrade.lua        |  16 ++
>   src/box/schema.cc              |  16 ++
>   src/box/schema_def.h           |  14 ++
>   src/box/space.c                |   2 +
>   src/box/space.h                |   3 +
>   src/box/sql.c                  |   8 +
>   src/box/sql/fkey.c             |  32 +--
>   src/box/sql/tarantoolInt.h     |   1 +
>   test/box/access_misc.result    |   5 +
>   test/box/access_sysview.result |   6 +-
>   test/box/alter.result          |   5 +-
>   test/box/misc.result           |   2 +
>   test/engine/iterator.result    |   2 +-
>   test/sql/foreign-keys.result   | 316 ++++++++++++++++++++++++++++++
>   test/sql/foreign-keys.test.lua | 144 ++++++++++++++
>   24 files changed, 1214 insertions(+), 36 deletions(-)
>   create mode 100644 src/box/fkey.c
>   create mode 100644 src/box/fkey.h
>   create mode 100644 test/sql/foreign-keys.result
>   create mode 100644 test/sql/foreign-keys.test.lua
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 89b11dcd3..aaa56bd21 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1889,6 +1916,22 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
>   			  "can not add a secondary key before primary");
>   	}
>   
> +	/*
> +	 * Can't drop index if foreign key constraints references
> +	 * this index.
> +	 */
> +	if (new_tuple == NULL) {

3. But it can be non-drop, but alter, for example. Drop case is processed
below:

     /* Case 1: drop the index, if it is dropped. */
     if (old_index != NULL && new_tuple == NULL) {

What if I want to rename an index? Also see index_def_change_requires_rebuild.

> +		struct fkey *fk = old_space->parent_fkey;
> +		while (fk != NULL) {
> +			if (old_space->parent_fkey->index_id == iid) {
> +				tnt_raise(ClientError, ER_ALTER_SPACE,
> +					  space_name(old_space),
> +					  "can not drop referenced index");
> +			}
> +			fk = fk->fkey_parent_next;
> +		}
> +	}
> +
> @@ -3404,6 +3447,387 @@ on_replace_dd_trigger
>   	txn_on_commit(txn, on_commit);
>   }
>   
> +/**
> + * Decode MsgPack array of links. It consists from maps:
> + * {parent_id (UINT) : child_id (UINT)}.
> + *
> + * @param data MsgPack array of links.
> + * @param[out] out_count Count of links.
> + * @param constraint_name Constraint name to use in error
> + *			  messages.
> + * @param constraint_len Length of constraint name.
> + * @param errcode Errcode for client errors.
> + * @retval Array of links.
> + */
> +static struct field_link *
> +fkey_links_decode(const char *data, uint32_t *out_count,
> +		  const char *constraint_name, uint32_t constraint_len,
> +		  uint32_t errcode)
> +{
> +	assert(mp_typeof(*data) == MP_ARRAY);
> +	uint32_t count = mp_decode_array(&data);
> +	if (count == 0) {
> +		tnt_raise(ClientError, errcode,
> +			  tt_cstr(constraint_name, constraint_len),
> +			  "at least one link must be specified");
> +	}
> +	*out_count = count;
> +	size_t size = count * sizeof(struct field_link);
> +	struct field_link *region_links =
> +		(struct field_link *) region_alloc_xc(&fiber()->gc, size);
> +	memset(region_links, 0, size);
> +	const char **map = &data;
> +	for (uint32_t i = 0; i < count; ++i) {
> +		uint32_t map_sz = mp_decode_map(map);
> +		if (map_sz != 2) {
> +			tnt_raise(ClientError, errcode,
> +				  tt_cstr(constraint_name, constraint_len),
> +				  tt_sprintf("link must be map with 2 fields"));
> +		}
> +		if (mp_typeof(**map) != MP_STR) {
> +			tnt_raise(ClientError, errcode,
> +				  tt_cstr(constraint_name, constraint_len),
> +				  tt_sprintf("link %d is not map "\
> +					     "with string keys", i));
> +		}

4. What about a second key? mp_typeof(**map) right after decoding its
header returns type of the first key. But second can be of another type.

And why do we need this {child = , parent = }, ... sequence? Lets just
use {<uint>, <uint>}, {<uint>, <uint>}, {<uint>, <uint>} ... It is more
compact and simpler to parse. Or even just two arrays: first child and
second parent field numbers. It is the most canonical way similar to
SQL, it is not? When you specify two column ranges.

But in the internal format I vote to keep field_link. In the next
patches I see, that common usage case is to iterate over the pairs,
so for cache it is better to keep them.

> +		for (uint8_t j = 0; j < map_sz; ++j) {
> +			uint32_t key_len;
> +			const char *key = mp_decode_str(map, &key_len);
> +			if (key_len == 6 &&
> +			    memcmp(key, "parent", key_len) == 0) {
> +				region_links[i].parent_field =
> +					mp_decode_uint(map);
> +			} else if (key_len == 5 &&
> +				   memcmp(key, "child", key_len) == 0) {
> +				region_links[i].child_field =
> +					mp_decode_uint(map);
> +			} else {
> +				char *errmsg = tt_static_buf();
> +				snprintf(errmsg, TT_STATIC_BUF_LEN,
> +					 "unexpected key of link %d '%.*s'", i,
> +					 key_len, key);
> +				tnt_raise(ClientError, errcode,
> +					  tt_cstr(constraint_name,
> +						  constraint_len), errmsg);
> +			}
> +		}
> +	}
> +	return region_links;
> +}
> +
> +/**
> + * Replace entry in child's and parent's lists of
> + * FK constraints.
> + *
> + * @param child Child space of FK constraint.
> + * @param parent Parent space of FK constraint.
> + * @param new_fkey Constraint to be added to child and parent.
> + * @param[out] old_fkey Constraint to be found and replaced.
> + */
> +static void
> +fkey_list_replace(struct space *child, struct space *parent, const char *name,
> +		  struct fkey *new_fkey, struct fkey **old_fkey)
> +{

5. I see that for fkey you actually have invented rlist. Please, see rlist.h
and use it for fkeys. It already has rlist_foreach_entry, rlist_add,
rlist_add_tail, rlist_del etc. The struct foreign key will have two
rlist links: in_child and in_parent.

> +	*old_fkey = NULL;
> +	struct fkey **fk = &parent->parent_fkey;
> +	while (*fk != NULL && !(strcmp((*fk)->def->name, name) == 0 &&
> +			        (*fk)->def->child_id == child->def->id))
> +		fk = &((*fk)->fkey_parent_next);
> +	if (*fk != NULL) {
> +		*old_fkey = *fk;
> +		*fk = (*fk)->fkey_parent_next;
> +	}
> +	if (new_fkey != NULL) {
> +		new_fkey->fkey_parent_next = parent->parent_fkey;
> +		parent->parent_fkey = new_fkey;
> +	}
> +	fk = &child->child_fkey;
> +	/* In child's list all constraints are unique by name. */
> +	while (*fk != NULL && strcmp((*fk)->def->name, name) != 0)
> +		fk = &((*fk)->fkey_child_next);
> +	if (*fk != NULL) {
> +		assert(*old_fkey == *fk);
> +		*fk = (*fk)->fkey_child_next;
> +	}
> +	if (new_fkey != NULL) {
> +		new_fkey->fkey_child_next = child->child_fkey;
> +		child->child_fkey = new_fkey;
> +	}
> +}
> +
> +/**
> + * On rollback of creation we remove FK constraint from DD, i.e.
> + * from parent's and child's lists of constraints and
> + * release memory.
> + */
> +static void
> +on_create_fkey_rollback(struct trigger *trigger, void *event)
> +{
> +	(void) event;
> +	struct fkey *fk = (struct fkey *)trigger->data;
> +	struct space *parent = space_by_id(fk->def->parent_id);
> +	struct space *child = space_by_id(fk->def->child_id);
> +	struct fkey *fkey = NULL;
> +	fkey_list_replace(child, parent, fk->def->name, NULL, &fkey);
> +	fkey_delete(fkey);

6. After you fixed the previous remark, this function would
collapse into two rlist_del, it is not? The same about the
next function. Btw, these functions are identical. Can you
keep only one?

Same about on_drop_fkey_commit and on_replace_fkey_commit.

> +}
> +
> +/** Return old FK and release memory for the new one. */
> +static void
> +on_replace_fkey_rollback(struct trigger *trigger, void *event)
> +{
> +	(void) event;
> +	struct fkey *fk = (struct fkey *)trigger->data;
> +	struct space *parent = space_by_id(fk->def->parent_id);
> +	struct space *child = space_by_id(fk->def->child_id);
> +	struct fkey *old_fkey = NULL;
> +	fkey_list_replace(child, parent, fk->def->name, fk, &old_fkey);
> +	fkey_delete(old_fkey);
> +}
> +
> +/** Release memory for old foreign key. */
> +static void
> +on_replace_fkey_commit(struct trigger *trigger, void *event)
> +{
> +	(void) event;
> +	struct fkey *fk = (struct fkey *)trigger->data;
> +	fkey_delete(fk);
> +}
> +
> +/** On rollback of drop simply return back FK to DD. */
> +static void
> +on_drop_fkey_rollback(struct trigger *trigger, void *event)
> +{
> +	(void) event;
> +	struct fkey *fk_to_restore = (struct fkey *)trigger->data;
> +	struct space *parent = space_by_id(fk_to_restore->def->parent_id);
> +	struct space *child = space_by_id(fk_to_restore->def->child_id);
> +	struct fkey *old_fk;
> +	fkey_list_replace(child, parent, fk_to_restore->def->name, fk_to_restore,
> +			  &old_fk);
> +	assert(old_fk == NULL);
> +}
> +
> +/**
> + * On commit of drop we have already deleted foreign key from
> + * both (parent's and child's) lists, so just release memory.
> + */
> +static void
> +on_drop_fkey_commit(struct trigger *trigger, void *event)
> +{
> +	(void) event;
> +	struct fkey *fk = (struct fkey *)trigger->data;
> +	fkey_delete(fk);
> +}
> +
> +/** A trigger invoked on replace in the _fk_constraint space. */
> +static void
> +on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
> +{
> +	struct txn *txn = (struct txn *) event;
> +	txn_check_singlestatement_xc(txn, "Space _fk_constraint");
> +	struct txn_stmt *stmt = txn_current_stmt(txn);
> +	struct tuple *old_tuple = stmt->old_tuple;
> +	struct tuple *new_tuple = stmt->new_tuple;
> +	if (new_tuple != NULL) {
> +		/* Create or replace foreign key. */
> +		struct fkey_def *fk_def =
> +			fkey_def_new_from_tuple(new_tuple,
> +						ER_CREATE_FK_CONSTRAINT);
> +		auto fkey_def_guard = make_scoped_guard([=] { free(fk_def); });
> +		struct space *child_space =
> +			space_cache_find_xc(fk_def->child_id);
> +		if (child_space->def->opts.is_view) {
> +			tnt_raise(ClientError, ER_CREATE_FK_CONSTRAINT,
> +				  fk_def->name,
> +				  "referencing space can't be VIEW");
> +		}
> +		struct space *parent_space =
> +			space_cache_find_xc(fk_def->parent_id);
> +		if (parent_space->def->opts.is_view) {
> +			tnt_raise(ClientError, ER_CREATE_FK_CONSTRAINT,
> +				  fk_def->name,
> +				  "referenced space can't be VIEW");
> +		}
> +		/*
> +		 * FIXME: until SQL triggers are completely
> +		 * integrated into server (i.e. we are able to
> +		 * invoke triggers even if DML occurred via Lua
> +		 * interface), it makes no sense to provide any
> +		 * checks on existing data in space.
> +		 */
> +		struct index *pk = space_index(child_space, 0);
> +		if (index_count(pk, ITER_ALL, NULL, 0) > 0) {

7. Lets better use index_size. Index_count on Vinyl takes O(N) time
scanning disk. I think, it is ok to forbid new fk on a Vinyl space,
that logically is empty, but actually still contains some non-compacted
garbage. Anyway user now is able to force the compaction and try to
create fk again.

> +			tnt_raise(ClientError, ER_CREATE_FK_CONSTRAINT,
> +				  fk_def->name,
> +				  "referencing space must be empty");
> +		}
> +		/* Check types of referenced fields. */
> +		for (uint32_t i = 0; i < fk_def->field_count; ++i) {
> +			uint32_t child_fieldno = fk_def->links[i].child_field;
> +			uint32_t parent_fieldno = fk_def->links[i].parent_field;
> +			if (child_fieldno >= child_space->def->field_count ||
> +			    parent_fieldno >= parent_space->def->field_count) {
> +				tnt_raise(ClientError, ER_CREATE_FK_CONSTRAINT,
> +					  fk_def->name, "foreign key refers to "
> +						        "nonexistent field");
> +			}
> +			if (child_space->def->fields[child_fieldno].type !=
> +			    parent_space->def->fields[parent_fieldno].type) {
> +				tnt_raise(ClientError, ER_CREATE_FK_CONSTRAINT,
> +					  fk_def->name, "field type mismatch");
> +			}
> +			if (child_space->def->fields[child_fieldno].coll_id !=
> +			    parent_space->def->fields[parent_fieldno].coll_id) {
> +				tnt_raise(ClientError, ER_CREATE_FK_CONSTRAINT,
> +					  fk_def->name,
> +					  "field collation mismatch");
> +			}
> +		}
> +		/*
> +		 * Search for suitable index in parent space:
> +		 * it must be unique and consist exactly from
> +		 * referenced columns (but order may be different).

8. Should this index has the same collations? For a column we can define
multiple collations in different indexes. And in one index the value may
be unique, but in another may be not.

> +		 */
> +		struct index *fk_index = NULL;
> +		for (uint32_t i = 0; i < parent_space->index_count; ++i) {
> +			struct index *idx = space_index(parent_space, i);
> +			if (!idx->def->opts.is_unique)
> +				continue;
> +			if (idx->def->key_def->part_count !=
> +			    fk_def->field_count)
> +				continue;
> +			uint32_t j;
> +			for (j = 0; j < fk_def->field_count; ++j) {
> +				if (idx->def->key_def->parts[j].fieldno !=
> +				    fk_def->links[j].parent_field)
> +					break;
> +			}
> +			if (j != fk_def->field_count)
> +				continue;

9. See key_def_find.

10. Above you have said that order may be different, but here I see
that once you have found a unique index, you merely check that it
has sequentially the same field numbers as the fk.

> +			fk_index = idx;
> +			break;
> +		}
> +		if (fk_index == NULL) {
> +			tnt_raise(ClientError, ER_CREATE_FK_CONSTRAINT,
> +				  fk_def->name, "referenced fields don't "
> +					      "compose unique index");
> +		}
> +		struct fkey *fkey = (struct fkey *) malloc(sizeof(*fkey));

11. I know how you like memory mapping. How about merge fkey_def into
fkey memory? I see, that it is relatively simple and linear thing.

> +		if (fkey == NULL)
> +			tnt_raise(OutOfMemory, sizeof(*fkey), "malloc", "fkey");
> +		auto fkey_guard = make_scoped_guard([=] { free(fkey); });
> +		memset(fkey, 0, sizeof(*fkey));
> +		fkey->def = fk_def;
> +		fkey->index_id = fk_index->def->iid;
> +		struct fkey *old_fk;
> +		fkey_list_replace(child_space, parent_space, fk_def->name,
> +				  fkey, &old_fk);
> +		if (old_tuple == NULL) {
> +			struct trigger *on_rollback =
> +				txn_alter_trigger_new(on_create_fkey_rollback,
> +						      fkey);
> +			txn_on_rollback(txn, on_rollback);
> +			assert(old_fk == NULL);
> +		} else {
> +			struct trigger *on_rollback =
> +				txn_alter_trigger_new(on_replace_fkey_rollback,
> +						      fkey);
> +			txn_on_rollback(txn, on_rollback);
> +			struct trigger *on_commit =
> +				txn_alter_trigger_new(on_replace_fkey_commit,
> +						      old_fk);
> +			txn_on_commit(txn, on_commit);
> +		}
> +		fkey_def_guard.is_active = false;
> +		fkey_guard.is_active = false;
> +	} else if (new_tuple == NULL && old_tuple != NULL) {
> +		/* Drop foreign key. */
> +		struct fkey_def *fk_def =
> +			fkey_def_new_from_tuple(old_tuple,
> +						ER_DROP_FK_CONSTRAINT);
> +		auto fkey_guard = make_scoped_guard([=] { free(fk_def); });
> +		struct space *parent_space =
> +			space_cache_find_xc(fk_def->parent_id);
> +		struct space *child_space =
> +			space_cache_find_xc(fk_def->child_id);
> +		struct fkey *old_fkey = NULL;
> +		fkey_list_replace(child_space, parent_space, fk_def->name, NULL,
> +				  &old_fkey);
> +		struct trigger *on_commit =
> +			txn_alter_trigger_new(on_drop_fkey_commit, old_fkey);
> +		txn_on_commit(txn, on_commit);
> +		struct trigger *on_rollback =
> +			txn_alter_trigger_new(on_drop_fkey_rollback, old_fkey);
> +		txn_on_rollback(txn, on_rollback);
> +	}
> +}
> +
> diff --git a/src/box/fkey.c b/src/box/fkey.c
> new file mode 100644
> index 000000000..e45889a0d
> --- /dev/null
> +++ b/src/box/fkey.c
> +void
> +fkey_trigger_delete(struct sqlite3 *db, struct sql_trigger *p)
> +{

12. Why do you need this function when sql_trigger_delete exists?

> +	if (p != NULL) {
> +		struct TriggerStep *step = p->step_list;
> +		sql_expr_delete(db, step->pWhere, false);
> +		sql_expr_list_delete(db, step->pExprList);
> +		sql_select_delete(db, step->pSelect);
> +		sql_expr_delete(db, p->pWhen, false);
> +		sqlite3DbFree(db, p);
> +	}
> +}
> +
> diff --git a/src/box/fkey.h b/src/box/fkey.h
> new file mode 100644
> index 000000000..1b6ea71d9
> --- /dev/null
> +++ b/src/box/fkey.h
> @@ -0,0 +1,163 @@
> +#ifndef TARANTOOL_BOX_FKEY_H_INCLUDED
> +#define TARANTOOL_BOX_FKEY_H_INCLUDED
> +/*
> + * Copyright 2010-2018, Tarantool AUTHORS, please see AUTHORS file.
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + * 1. Redistributions of source code must retain the above
> + *    copyright notice, this list of conditions and the
> + *    following disclaimer.
> + *
> + * 2. Redistributions in binary form must reproduce the above
> + *    copyright notice, this list of conditions and the following
> + *    disclaimer in the documentation and/or other materials
> + *    provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY <COPYRIGHT HOLDER> ``AS IS'' AND
> + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED
> + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL
> + * <COPYRIGHT HOLDER> OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
> + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF
> + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
> + * SUCH DAMAGE.
> + */
> +#include <stdbool.h>
> +#include <stdint.h>
> +
> +#include "space.h"
> +
> +#if defined(__cplusplus)
> +extern "C" {
> +#endif /* defined(__cplusplus) */
> +
> +struct sqlite3;

13. Is it possible to drop this announcement after
you replaced fkey_trigger_delete with sql_trigger_delete?

> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index 87c79bdde..30d8b0081 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua
> @@ -506,6 +506,7 @@ box.schema.space.drop = function(space_id, space_name, opts)
>       local _vindex = box.space[box.schema.VINDEX_ID]
>       local _truncate = box.space[box.schema.TRUNCATE_ID]
>       local _space_sequence = box.space[box.schema.SPACE_SEQUENCE_ID]
> +    local _fk_constraint = box.space[box.schema.FK_CONSTRAINT_ID]
>       local sequence_tuple = _space_sequence:delete{space_id}
>       if sequence_tuple ~= nil and sequence_tuple[3] == true then
>           -- Delete automatically generated sequence.
> @@ -519,6 +520,11 @@ box.schema.space.drop = function(space_id, space_name, opts)
>           local v = keys[i]
>           _index:delete{v[1], v[2]}
>       end
> +    for _, t in _fk_constraint.index.primary:pairs() do
> +        if t.child_id == space_id then

14. Fullscan here looks bad. Can we create a secondary index on
_fk_constraint by child_id? Or create a Lua C function, that takes
space and returns names of its child fks? If we use the index, then
we are able to optimize VDBE deletion of the FKs in the main patch
(sql: introduce ADD CONSTRAINT statement), where you delete by
the composite key {name, child_id}, but actually child_id is enough.

> +            _fk_constraint:delete{t.name, t.child_id}
> +        end
> +    end
> diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
> index f112a93ae..772f55cb2 100644
> --- a/src/box/lua/upgrade.lua
> +++ b/src/box/lua/upgrade.lua
> @@ -509,6 +509,22 @@ local function upgrade_to_2_1_0()
>                     {unique = true}, {{0, 'string'}, {1, 'string'},
>                                       {5, 'scalar'}}}
>   
> +    local fk_constr_ft = {{name='name', type='string'},
> +                          {name='child_id', type='unsigned'},
> +                          {name='parent_id', type='unsigned'},
> +                          {name='deferred', type='boolean'},

15. Flag, so 'is_deferred', it is not?

> +                          {name='match', type='string'},
> +                          {name='on_delete', type='string'},
> +                          {name='on_update', type='string'},
> +                          {name='links', type='array'}}
> +    log.info("create space _fk_constraint")
> +    _space:insert{box.schema.FK_CONSTRAINT_ID, ADMIN, '_fk_constraint', 'memtx',
> +                  0, setmap({}), fk_constr_ft}
> +
> +    log.info("create index primary on _fk_constraint")
> +    _index:insert{box.schema.FK_CONSTRAINT_ID, 0, 'primary', 'tree',
> +                  {unique = true}, {{0, 'string'}, {1, 'unsigned'}}}
> +
>       -- Nullability wasn't skipable. This was fixed in 1-7.
>       -- Now, abscent field means NULL, so we can safely set second
>       -- field in format, marking it nullable.
> diff --git a/src/box/space.h b/src/box/space.h
> index 7da2ee51f..fc5e8046f 100644
> --- a/src/box/space.h
> +++ b/src/box/space.h
> @@ -183,6 +183,9 @@ struct space {
>   	 * of index id.
>   	 */
>   	struct index **index;
> +	/** Foreign key constraints. */

16. It would be good to have here an explanation about who is
child, who is parent.

> +	struct fkey *parent_fkey;
> +	struct fkey *child_fkey;
>   };
>   
> diff --git a/test/box/access_misc.result b/test/box/access_misc.result
> index 5a2563d55..62c92ca03 100644
> --- a/test/box/access_misc.result
> +++ b/test/box/access_misc.result
> @@ -809,6 +809,11 @@ box.space._space:select()
>     - [349, 1, '_sql_stat4', 'memtx', 0, {}, [{'name': 'tbl', 'type': 'string'}, {'name': 'idx',
>           'type': 'string'}, {'name': 'neq', 'type': 'string'}, {'name': 'nlt', 'type': 'string'},
>         {'name': 'ndlt', 'type': 'string'}, {'name': 'sample', 'type': 'scalar'}]]
> +  - [350, 1, '_fk_constraint', 'memtx', 0, {}, [{'name': 'name', 'type': 'string'},
> +      {'name': 'child_id', 'type': 'unsigned'}, {'name': 'parent_id', 'type': 'unsigned'},
> +      {'name': 'deferred', 'type': 'boolean'}, {'name': 'match', 'type': 'string'},
> +      {'name': 'on_delete', 'type': 'string'}, {'name': 'on_update', 'type': 'string'},
> +      {'name': 'links', 'type': 'array'}]]

17. Please, keep spare room after sql_stat4 in 8 identifiers for future needs. So as
_fk_constraint was 356 (sql_stat1 348 + 8).

>   ...
>   box.space._func:select()
>   ---
> diff --git a/test/engine/iterator.result b/test/engine/iterator.result
> index a36761df8..ba9b0545a 100644
> --- a/test/engine/iterator.result
> +++ b/test/engine/iterator.result
> @@ -4211,7 +4211,7 @@ s:replace{35}
>   ...
>   state, value = gen(param,state)
>   ---
> -- error: 'builtin/box/schema.lua:1049: usage: next(param, state)'
> +- error: 'builtin/box/schema.lua:1055: usage: next(param, state)'

18. This test fails on each schema change. Lets remove this file:line
from the error message alongside the patch.