[tarantool-patches] Re: [PATCH 2/5] schema: add new system space for FK constraints
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Fri Aug 3 01:15:14 MSK 2018
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");
> +}
> +
More information about the Tarantool-patches
mailing list