[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 3 comments below and a separate
commit on the branch.

>     schema: add new system space for FK constraints
>     
>     This patch introduces new system space to persist foreign keys
>     constraints. Format of the space:
>     
>     _fk_constraint (space id = 358)
>     
>     [<contraint name> STR, <parent id> UINT, <child id> UINT,

1. Typo: contraint.

>      <is deferred> BOOL, <match> STR, <on delete action> STR,
>      <on update action> STR, <child cols> ARRAY<UINT>,
>      <parent cols> ARRAY<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 child spaces must exist;
>      - VIEWs can't be involved into FK processing;
>      - Child space must be empty;
>      - Types of referencing and referenced fields must be comparable;
>      - Collations of referencing and referenced fields must match;
>      - Referenced fields must compose unique index;
>      - Referenced fields can not contain duplicates.
>     
>     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> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 7b6bd1a5a..5b55bfd7a 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -3459,6 +3515,395 @@ on_replace_dd_trigger(struct trigger * /* trigger */, void *event)> +
> +/**
> + * 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;
> +	rlist_del_entry(fk, parent_link);
> +	rlist_del_entry(fk, child_link);
> +	fkey_delete(fk);
> +}
> +
> +/** 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 = fkey_grab_by_name(&child->child_fkey,
> +						  fk->def->name);
> +	fkey_delete(old_fkey);
> +	rlist_add_entry(&child->child_fkey, fk, child_link);
> +	rlist_add_entry(&parent->parent_fkey, fk, parent_link);


2. In the comment you said that this function restores the old fk and
deletes the new one. But on the code we see the line:
fkey_delete(old_fkey).

Secondly, you got old_fkey from child->child_fkey, but earlier, in
on_replace trigger, you had removed old_fkey from child_fkey list
and put here new fkey. So here actually old_fkey == fkey (I tested, it is).

I think, it can be fixed quite simple: you have ability to fetch the new
fkey from child_list and have no the latter for the old one since it is
unlinked already. So lets pass here old_fkey as trigger->data and fetch
the new fkey from the list.

The test for this bug is below:

	box.sql.execute("CREATE TABLE t1 (id PRIMARY KEY, a REFERENCES t1, b INT);")
	t = box.space._fk_constraint:select{}[1]:totable()
	errinj = box.error.injection
	errinj.set("ERRINJ_WAL_IO", true)
	box.space._fk_constraint:replace(t)

But it crashes on the next commit. On the current one this function is
unreachable.

	Process 51167 stopped
	* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)

> +/**
> + * ANSI SQL doesn't allow list of referenced fields to contain
> + * duplicates. Firstly, we try to follow the easiest way:
> + * if all referenced fields numbers are less than 63, we can
> + * use bit mask. Otherwise, fall through slow check where we
> + * use O(field_cont^2) simple nested cycle iterations.
> + */
> +static void
> +fkey_links_check_duplicates(struct fkey_def *fk_def)
> +{
> +	uint64_t field_mask = 0;
> +	for (uint32_t i = 0; i < fk_def->field_count; ++i) {
> +		uint32_t parent_field = fk_def->links[i].parent_field;
> +		if (parent_field > 63)
> +			goto slow_check;
> +		if (field_mask & (((uint64_t) 1) << parent_field))
> +			goto error;
> +		column_mask_set_fieldno(&field_mask, parent_field);

3. Looks like column_mask API is not applicable here since you
anyway still use raw bit setting. I have removed column_mask API
from there on the branch.

> +	}
> +	return;
> +slow_check:
> +	for (uint32_t i = 0; i < fk_def->field_count; ++i) {
> +		uint32_t parent_field = fk_def->links[i].parent_field;
> +		for (uint32_t j = i + 1; j < fk_def->field_count; ++j) {
> +			if (parent_field == fk_def->links[j].parent_field)
> +				goto error;
> +		}
> +	}
> +	return;
> +error:
> +	tnt_raise(ClientError, ER_CREATE_FK_CONSTRAINT, fk_def->name,
> +		  "referenced fields can not contain duplicates");
> +}
> +