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 899202796B for ; Sun, 5 Aug 2018 20:28:07 -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 NevH-sJ79mVz for ; Sun, 5 Aug 2018 20:28:07 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 DA1C22759B for ; Sun, 5 Aug 2018 20:28:06 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 2/5] schema: add new system space for FK constraints From: "n.pettik" In-Reply-To: <30b2a1cc-3e6d-e5f6-bca5-6a5e5396bc14@tarantool.org> Date: Mon, 6 Aug 2018 03:28:02 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <0DFF30FE-9BAF-4149-910A-A552D2740080@tarantool.org> References: <83369e84-6fba-adbf-8751-2141a3e256b1@tarantool.org> <24bb9776-b33d-a68f-8738-3967ffd013ea@tarantool.org> <30b2a1cc-3e6d-e5f6-bca5-6a5e5396bc14@tarantool.org> 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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy Firstly, I don=E2=80=99t understand some of your fixes: @@ -3855,11 +3853,9 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) memset(fkey, 0, sizeof(*fkey)); fkey->def =3D fk_def; fkey->index_id =3D fk_index->def->iid; + rlist_add_entry(&child_space->child_fkey, fkey, = child_link); + rlist_add_entry(&parent_space->parent_fkey, fkey, = parent_link); if (old_tuple =3D=3D NULL) { - rlist_add_entry(&child_space->child_fkey, fkey, - child_link); - rlist_add_entry(&parent_space->parent_fkey, = fkey, - parent_link); struct trigger *on_rollback =3D = txn_alter_trigger_new(on_create_fkey_rollback, fkey); @@ -3868,10 +3864,6 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) struct fkey *old_fk =3D = fkey_grab_by_name(&child_space->child_fkey, fk_def->name); - rlist_add_entry(&child_space->child_fkey, fkey, - child_link); - rlist_add_entry(&parent_space->parent_fkey, = fkey, - parent_link); In case of replace we must firstly remove entry and only then insert new = one. It is easy to check that the way you suggest doesn=E2=80=99t work (just = modify test you provided): box.sql.execute("CREATE TABLE t3 (id PRIMARY KEY, a REFERENCES t3, b INT = UNIQUE);") t =3D box.space._fk_constraint:select{}[1]:totable() errinj =3D box.error.injection errinj.set("ERRINJ_WAL_IO", true) -- Make constraint reference B field instead of id. t[9] =3D {2} box.space._fk_constraint:replace(t) box.sql.execute("INSERT INTO t3 VALUES (1, 2, 2)=E2=80=9D) - No error is raised. If I discard your diff it works fine (i.e. "FOREIGN KEY constraint = failed=E2=80=9D error is raised). >> 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 =3D 358) >> [ STR, UINT, UINT, >=20 > 1. Typo: contraint. Fixed. >> +/** 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 =3D (struct fkey *)trigger->data; >> + struct space *parent =3D space_by_id(fk->def->parent_id); >> + struct space *child =3D space_by_id(fk->def->child_id); >> + struct fkey *old_fkey =3D 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); >=20 >=20 > 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). >=20 > 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 =3D=3D fkey (I = tested, it is). >=20 > 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. Thx for catch. Fixed: +++ b/src/box/alter.cc @@ -3678,14 +3678,14 @@ static void on_replace_fkey_rollback(struct trigger *trigger, void *event) { (void) event; - struct fkey *fk =3D (struct fkey *)trigger->data; - struct space *parent =3D space_by_id(fk->def->parent_id); - struct space *child =3D space_by_id(fk->def->child_id); - struct fkey *old_fkey =3D 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); + struct fkey *old_fk =3D (struct fkey *)trigger->data; + struct space *parent =3D space_by_id(old_fk->def->parent_id); + struct space *child =3D space_by_id(old_fk->def->child_id); + struct fkey *new_fkey =3D fkey_grab_by_name(&child->child_fkey, + old_fk->def->name); + fkey_delete(new_fkey); + rlist_add_entry(&child->child_fkey, old_fk, child_link); + rlist_add_entry(&parent->parent_fkey, old_fk, parent_link); } =20 /** On rollback of drop simply return back FK to DD. */ @@ -3866,7 +3866,7 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) fk_def->name); struct trigger *on_rollback =3D = txn_alter_trigger_new(on_replace_fkey_rollback, - fkey); + old_fk); >=20 > The test for this bug is below: >=20 > box.sql.execute("CREATE TABLE t1 (id PRIMARY KEY, a REFERENCES = t1, b INT);") > t =3D box.space._fk_constraint:select{}[1]:totable() > errinj =3D box.error.injection > errinj.set("ERRINJ_WAL_IO", true) > box.space._fk_constraint:replace(t) Added to the next patch: +++ b/test/sql/foreign-keys.test.lua @@ -160,5 +160,31 @@ box.space._fk_constraint:select{} box.sql.execute('DROP TABLE tc') box.sql.execute('DROP TABLE tp') =20 +-- Tests which are aimed at verifying work of commit/rollback +-- triggers on _fk_constraint space. +-- +box.sql.execute("CREATE TABLE t3 (id PRIMARY KEY, a REFERENCES t3, b = INT UNIQUE);") +t =3D box.space._fk_constraint:select{}[1]:totable() +errinj =3D box.error.injection +errinj.set("ERRINJ_WAL_IO", true) +-- Make constraint reference B field instead of id. +t[9] =3D {2} +box.space._fk_constraint:replace(t) +errinj.set("ERRINJ_WAL_IO", false) +box.sql.execute("INSERT INTO t3 VALUES (1, 2, 2);") +errinj.set("ERRINJ_WAL_IO", true) +box.sql.execute("ALTER TABLE t3 ADD CONSTRAINT fk1 FOREIGN KEY (b) = REFERENCES t3;") +errinj.set("ERRINJ_WAL_IO", false) +box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);") +box.sql.execute("DELETE FROM t3;") +box.snapshot() +box.sql.execute("ALTER TABLE t3 ADD CONSTRAINT fk1 FOREIGN KEY (b) = REFERENCES t3;") +box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);") +errinj.set("ERRINJ_WAL_IO", true) +box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;") +box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);") +errinj.set("ERRINJ_WAL_IO", false) +box.sql.execute("DROP TABLE t3;") + +++ b/test/sql/foreign-keys.result @@ -356,5 +356,85 @@ box.sql.execute('DROP TABLE tc') box.sql.execute('DROP TABLE tp') --- ... +-- Tests which are aimed at verifying work of commit/rollback +-- triggers on _fk_constraint space. +-- +box.sql.execute("CREATE TABLE t3 (id PRIMARY KEY, a REFERENCES t3, b = INT UNIQUE);") +--- +... +t =3D box.space._fk_constraint:select{}[1]:totable() +--- +... +errinj =3D box.error.injection +--- +... +errinj.set("ERRINJ_WAL_IO", true) +--- +- ok +... +-- Make constraint reference B field instead of id. +t[9] =3D {2} +--- +... +box.space._fk_constraint:replace(t) +--- +- error: Failed to write to disk +... +errinj.set("ERRINJ_WAL_IO", false) +--- +- ok +... +box.sql.execute("INSERT INTO t3 VALUES (1, 2, 2);") +--- +- error: FOREIGN KEY constraint failed +... +errinj.set("ERRINJ_WAL_IO", true) +--- +- ok +... +box.sql.execute("ALTER TABLE t3 ADD CONSTRAINT fk1 FOREIGN KEY (b) = REFERENCES t3;") +--- +- error: Failed to write to disk +... +errinj.set("ERRINJ_WAL_IO", false) +--- +- ok +... +box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);") +--- +... +box.sql.execute("DELETE FROM t3;") +--- +... +box.snapshot() +--- +- ok +... +box.sql.execute("ALTER TABLE t3 ADD CONSTRAINT fk1 FOREIGN KEY (b) = REFERENCES t3;") +--- +... +box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);") +--- +- error: FOREIGN KEY constraint failed +... +errinj.set("ERRINJ_WAL_IO", true) +--- +- ok +... +box.sql.execute("ALTER TABLE t3 DROP CONSTRAINT fk1;") +--- +- error: Failed to write to disk +... +box.sql.execute("INSERT INTO t3 VALUES(1, 1, 3);") +--- +- error: FOREIGN KEY constraint failed +... +errinj.set("ERRINJ_WAL_IO", false) +--- +- ok +... +box.sql.execute("DROP TABLE t3;") +--- +... > But it crashes on the next commit. On the current one this function is > unreachable. >=20 > Process 51167 stopped > * thread #1, queue =3D 'com.apple.main-thread', stop reason =3D = EXC_BAD_ACCESS (code=3DEXC_I386_GPFLT) >=20 >> +/** >> + * 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 =3D 0; >> + for (uint32_t i =3D 0; i < fk_def->field_count; ++i) { >> + uint32_t parent_field =3D 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); >=20 > 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. Yep, thx for fix.