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 5989A27563 for ; Mon, 6 Aug 2018 14:25:00 -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 IZRv0vcNi-9G for ; Mon, 6 Aug 2018 14:25:00 -0400 (EDT) Received: from smtp1.mail.ru (smtp1.mail.ru [94.100.179.111]) (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 A9C392219E for ; Mon, 6 Aug 2018 14:24:59 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/5] sql: introduce ADD CONSTRAINT statement References: <092e07d5399d662cd38b755a0403b11cc4dde2a1.1531443603.git.korablev@tarantool.org> <647dcc4b-a2c4-5cc8-c81c-2dbb66ce18ed@tarantool.org> <9B5CA4F9-A6DB-46FF-BE86-CC3900D22D97@tarantool.org> From: Vladislav Shpilevoy Message-ID: <17680404-70f3-93bb-1f7c-024ff45a3530@tarantool.org> Date: Mon, 6 Aug 2018 21:24:56 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8"; format="flowed" Content-Language: en-US Content-Transfer-Encoding: 8bit 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! The whole patchset LGTM except two comments below about the tests. On 06/08/2018 03:28, n.pettik wrote: > I squashed your fixes. However, personally I don’t understand why you > decided to refactor endTable() routine, for instance. Ofc refactoring > is always appreciated but I would prefer separating functional part of patch and > cosmetic/codestyle changes. Initial version of this patch AFAIR included 1k +- lines. > Now (due to codestyle fixes) it is about 1.8k + and 1.6k -.. It would be quite > complicated to find smth in such giant diff. But new code amount was reduced actually. Many of the new code (added during the review) is the tests on the bugs found during the review. > >>> diff --git a/src/box/alter.cc b/src/box/alter.cc >>> index 5b55bfd7a..6b9e29470 100644 >>> --- a/src/box/alter.cc >>> +++ b/src/box/alter.cc >>> @@ -3660,6 +3660,50 @@ fkey_grab_by_name(struct rlist *list, const char *fkey_name) >>> return NULL; >>> } >>> +static void >> >> 1. No comment. > > Comment for static inline two-lines function? Seriously? > Ok, I will add it, but I think we should consider making an exception > in our developing guide for instance for static functions less than > 10 lines or sort of. You can propose it in a chat. >>> + /* Delete all child FK constraints. */ >>> + struct fkey *child_fk; >>> + rlist_foreach_entry (child_fk, &space->child_fkey, child_link) { >>> + const char *fk_name_dup = sqlite3DbStrDup(v->db, >>> + child_fk->def->name); >>> + if (fk_name_dup == NULL) >>> + return; >>> + vdbe_emit_fkey_drop(parse_context, fk_name_dup, space_id); >> >> 5. Can we use P4_STATIC and do not duplicate? > > No, unfortunately we can’t. P4_STATIC attribute is usable only when the same > memory is used twice during VDBE execution. STATIC attribute prevents from > memory releasing (even at the end of execution AFAIK). If we released memory > right after generating opcode (or at the end of parsing, it doesn’t matter), we will > probably get corrupted memory. How can child_fk->def->name be freed before VDBE execution? It is drop table code generation, so here the child_fk exists out of the parser context in a space from the space cache. > >> I want to say, that if we >> do not duplicate the memory here, then only 2 situations are possible: >> 1) Until the VDBE is executed nothing is happened, the memory is ok and >> can be used. > > Can be used, but wouldn’t be freed (or I misunderstand what you initially meant). Yes, it will not be freed because it will not be duplicated. Free of the original name together with the child_fk will occur inside alter.cc. Vdbe after drop of the space will not touch this freed memory. But do not mind. I see, that vdbe_emit_fkey_drop is used both to malloced normalized names from the parser and for struct fkey names. So it is not worth to refactor this function to use both static and dynamic memory. We need a more comprehensive way to compact these numerous mallocs during parsing, later. > diff --git a/test/sql-tap/fkey2.test.lua b/test/sql-tap/fkey2.test.lua > index 062597e9b..7cb315fcc 100755 > --- a/test/sql-tap/fkey2.test.lua > +++ b/test/sql-tap/fkey2.test.lua > @@ -755,10 +753,13 @@ test:do_catchsql_test( > DROP TABLE IF EXISTS p; > CREATE TABLE p(a, b, PRIMARY KEY(a, b)); > CREATE TABLE c(x PRIMARY KEY REFERENCES p); > +<<<<<<< HEAD > INSERT INTO c DEFAULT VALUES; > +======= > +>>>>>>> 323e96ee2... sql: introduce ADD CONSTRAINT statement 1. ??? > ]], { > -- > - 1, "foreign key mismatch - \"C\" referencing \"P\"" > + 1, "Failed to create foreign key constraint 'FK_CONSTRAINT_1_C': number of columns in foreign key does not match the number of columns in the primary index of referenced table" > -- > }) > > diff --git a/test/sql/foreign-keys.result b/test/sql/foreign-keys.result > index c2ec429c3..2580221d3 100644 > --- a/test/sql/foreign-keys.result > +++ b/test/sql/foreign-keys.result > @@ -332,5 +332,109 @@ box.space.CHILD:drop() > box.space.PARENT:drop() > --- > ... > --- Clean-up SQL DD hash. > -test_run:cmd('restart server default with cleanup=1') > +-- Check that parser correctly handles MATCH, ON DELETE and > +-- ON UPDATE clauses. > +-- > +box.sql.execute('CREATE TABLE tp (id INT PRIMARY KEY, a INT UNIQUE)') > +--- > +... > +box.sql.execute('CREATE TABLE tc (id INT PRIMARY KEY, a INT REFERENCES tp(a) ON DELETE SET NULL MATCH FULL)') > +--- > +... > +box.sql.execute('ALTER TABLE tc ADD CONSTRAINT fk1 FOREIGN KEY (id) REFERENCES tp(id) MATCH PARTIAL ON DELETE CASCADE ON UPDATE SET NULL') > +--- > +... > +box.space._fk_constraint:select{} > +--- > +- - ['FK1', 518, 517, false, 'partial', 'cascade', 'set_null', [0], [0]] > + - ['FK_CONSTRAINT_1_TC', 518, 517, false, 'full', 'set_null', 'no_action', [1], > + [1]] > +... > +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 = box.space._fk_constraint:select{}[1]:totable() > +--- > +... > +errinj = box.error.injection 2. Please, do not use errinj in regular tests. Only in release-disabled. Errinj structures and code are defined to nothing in C when NDEBUG is set, so they do not work.