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 AF52229B66 for ; Tue, 16 Apr 2019 09:51:14 -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 GiFYZEE7QwNy for ; Tue, 16 Apr 2019 09:51:14 -0400 (EDT) Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (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 E2CEE29AE6 for ; Tue, 16 Apr 2019 09:51:13 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 5/9] schema: add new system space for CHECK constraints References: <186b109d1f6049b1413fe0cdded43e5fad29a895.1548838034.git.kshcherbatov@tarantool.org> <7ef2f5ed-831d-dd55-1a5e-9dfa0b33bd0f@tarantool.org> <4092AC4B-4CED-48E8-9540-EF348802DC17@tarantool.org> From: Kirill Shcherbatov Message-ID: <5db849e3-5e46-0704-ddcf-c7af47a892e6@tarantool.org> Date: Tue, 16 Apr 2019 16:51:11 +0300 MIME-Version: 1.0 In-Reply-To: <4092AC4B-4CED-48E8-9540-EF348802DC17@tarantool.org> Content-Type: text/plain; charset="utf-8" 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: tarantool-patches@freelists.org, "n.pettik" Hi! Thank you for review. I didn't quote all your comments because the code is differ now and many of them are outdated or trivial(that are fixed). But I've answered all questions below. >>>> + ++schema_version; >>> >>> 7. We have UpdateSchemaVersion for that. It can be touched >>> direcrtly only by _space triggers. >> I don't know how make it for now. I don't use alter object in on_dd_...replace_trigger >> for _ck_constraint space; maybe it worth to introduce it. Do it later with review >> on whole patch. >> > > See no fixes to this comment. I've discussed it with Vlad. We don't need to update epoch here at all. > > I’ve found way to make Tarantool hang: > > box.sql.execute("create table t(id int primary key check (id > 5))") > drop_ck = function() box.space._ck_constraint:delete({'CK_CONSTRAINT_1_T', box.space.T.id}) end > fiber = require('fiber') > box.error.injection.set("ERRINJ_WAL_DELAY", true) > f2 = fiber.create(drop_ck) > box.sql.execute("INSERT INTO t VALUES(1)") > box.error.injection.set("ERRINJ_WAL_DELAY", false) > > Please, find the reason of that and add several tests like this: > insert/update + create/drop CK + delayed wal injection. I've created a bug reproduced without CK constraints. But it is a feature =) https://github.com/tarantool/tarantool/issues/4100#issuecomment-479846703 >> After insertion into this space, a new instance describing check >> constraint is created. Check constraint hold Expr tree. >> While space features check constraints, it isn't allowed to >> be dropped. The :drop() space method firstly deletes all check >> constraints and then removes entry from _space. > > It would be nice to see motivation for this change. Believe my new commit message a little better: schema: add new system space for CHECK constraints This patch introduces a new system space to persist check constraints. Format of the space: _ck_constraint (space id = 357) [ STR, UINT, STR] CK constraint is local to space, so every pair is unique (and it is PK in _ck_constraint space). After insertion into this space, a new instance describing check constraint is created. Check constraint holds Expr AST tree. While space features check constraints, it isn't allowed to be dropped. The :drop() space method firstly deletes all check constraints and then removes entry from _space. Because space alter, index alter and space truncate operations case space recreation, introduced RebuildCkConstrains object that compile new ck constraint objects, replace and remove existent instances atomically(when some compilation fails, nothing changed). In fact, in scope of this patch we don't really need to recreate ck_constraint object in such situations (patch space_def pointer in AST like we did it before is enough now, but we would recompile VDBE that represents ck constraint in future that can fail). The main motivation for these changes is the ability to support ADD CHECK CONSTRAINT operation in the future. Needed for #3691 >> alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1); >> + (void) new BuildCkConstraints(alter); > > Why do we need to rebuild check constraints on index alter? > >> + (void) new BuildCkConstraints(alter); > > Looks like a nonsense: why do we need to rebuild ck constraints on truncate? Consider my new comment: /** * As ck_constraint object depends on space_def we must rebuild * all ck constraints on space alter. * * To perform it transactionally, we create a list of a new ck * constraints objects in ::prepare method that is fault-tolerant. * Finally in ::alter or ::rollback methods we only swap thouse * lists securely. */ class RebuildCkConstraints: public AlterSpaceOp We must rebuild CK constraints when space definition is recreated. Factually, we must use RebuildCkConstraints on each alter_space_do() call: on_replace_dd_truncate, on_replace_dd_index, on_replace_dd_space. But I don't see any way to make it a logical part of alter_space_do. > Rename it to ck_constraint_def_new_from_tuple() to make name be > consistent with the rest of similar functions: > - space_def_new_from_tuple > - index_def_new_from_tuple > - func_def_new_from_tuple > ... > etc Ok. >> + if (ck_constraint == NULL) >> + return; > > But in on_replace_dd_ck_constraint() you have next check: > > assert(old_ck_constraint != NULL); > … > on_rollback->data = old_ck_constraint; > > How ck_constraint could be NULL? You are right. >> + access_check_ddl(space->def->name, space->def->id, space->def->uid, >> + SC_SPACE, PRIV_A); >> + > > See comments to previous patch concerning privileges. I've drop this code. We don't need to check privileges. Everything with _fk_constraint and _trigger spaces is ok. > Why do you need to place implementation and definition of CK in > the same memory chunk? Yep, it simplifies deallocation a bit, but > instead you have to copy def and call def_sizeof() 4 times. I suggest > to allocate def using malloc right during tuple parsing. I've reworked ck constraints structures. Now ck_constraint_def is simultaneous allocation. > You can make link be first member of struct and place > name at the end of struct (like in space_def or fk_def). But I have two strings here. Don't see any reason to distinguish them. Moreover, now this structure is slightly different. > For constraints with auto-generated names it would be helpful > to see failed condition IMHO. Otherwise, using only error message like > > "Check constraint failed: CK_CONSTRAINT_1_T1” > > it’s quite complicated to understand which constraint failed. > So, I suggest to add to this error string of failed condition. Done. > I suppose this should be part of upgrade to 2_2_0 script. I've implemented it as a part of upgrade_to_2_1_3 script. Feature #4007 that has 2.2.0 milestone is already there. >> + int all_ok = sqlVdbeMakeLabel(v); >> + sqlExprIfTrue(parse_context, expr, all_ok, SQL_JUMPIFNULL); >> + if (on_conflict == ON_CONFLICT_ACTION_IGNORE) { >> + sqlVdbeGoto(v, ignore_label); >> sqlVdbeResolveLabel(v, all_ok); > > Are you sure you need to resolve label here? > I see in code below one more same label resolution. You are right. It is redundant. > + /* >> + * Dissallow the transfer optimization if the destination >> + * table contains any check constraints. >> + */ >> + if (!rlist_empty(&dest->ck_constraint)) >> return 0; > > Why did you change condition of optimisation? It was easier to implement.. =) Ok; now we have equivalent code. And tests. >> + parser->parsed_ast.expr = sqlExprDup(parser->db, expr_list->a->pExpr, 0); > > Is this change related to patch-set? Described in comment. >> + static const char * const azType[] = >> + {"NOT NULL", "UNIQUE", "Check", "FOREIGN KEY" }; > > Do we really need this change? Now it is useless