From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 2E32E27C63 for ; Thu, 2 Aug 2018 18:15:17 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id YXnJEkX_PWdh for ; Thu, 2 Aug 2018 18:15:17 -0400 (EDT) Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DC7BA27C45 for ; Thu, 2 Aug 2018 18:15:16 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 2/5] schema: add new system space for FK constraints References: <83369e84-6fba-adbf-8751-2141a3e256b1@tarantool.org> <24bb9776-b33d-a68f-8738-3967ffd013ea@tarantool.org> From: Vladislav Shpilevoy Message-ID: <30b2a1cc-3e6d-e5f6-bca5-6a5e5396bc14@tarantool.org> Date: Fri, 3 Aug 2018 01:15:14 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.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) > > [ STR, UINT, UINT, 1. Typo: contraint. > BOOL, STR, STR, > STR, ARRAY, > ARRAY] > > FK constraint is local to space, so every pair > 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"); > +} > +