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 EA4DF2DBD1 for ; Sun, 12 May 2019 09:45:33 -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 PUrFL9ARQlnW for ; Sun, 12 May 2019 09:45:33 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 20A6A2DBB6 for ; Sun, 12 May 2019 09:45:33 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH v3 1/3] schema: add new system space for CHECK constraints From: "n.pettik" In-Reply-To: <0cd8bbf7-adfa-f59e-b726-313700aa2b39@tarantool.org> Date: Sun, 12 May 2019 16:45:30 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <0cd8bbf7-adfa-f59e-b726-313700aa2b39@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: Kirill Shcherbatov >> Nit: ck_constraint_def_sizeof() return uint32_t >>=20 >> Also why do we not use vdbe_emit_halt_with_presence_test() >> during creation of check constraints? >>=20 >> tarantool> create table t2(id int primary key, constraint ck1 = check(id > 0), constraint ck1 check(id < 0)) >> --- >> - error: Duplicate key exists in unique index 'primary' in space = '_ck_constraint' >> ... >> tarantool> create table t2(id int primary key, constraint fk1 foreign = key(id) references t2, constraint fk1 foreign key(id) references t2) >> --- >> - error: Constraint FK1 already exists >> ... >>=20 >=20 > And this is a bug in FK > https://github.com/tarantool/tarantool/issues/4183 Still, we have an option to fix it: display proper error message and process clean-up. Please, add a mention that in scope of issue we should also use vdbe_emit_halt_with_presence_test for CK constraints >>> + if (rlist_next(dst_ck_link) !=3D &dest->ck_constraint) >>> + return 0; >>=20 >> After next patch this check could be removed: VDBE program for = checking >> consistency of check constraints would be generated automatically. >> In SQLite xFer allows to avoid generation of this program. On the = other hand, >> if you added way to temporarily disable check constraints, we would = able >> to leave this check and re-enable them after query is executed. So, = consider >> way of disabling/enabling check constraints - it might be useful for = other >> cases as well. >=20 > At first, there is no way to control this state as you propose. Ok, then we should come up with machinery which will allow us to disable CK constraints, but not other NoSQL triggers. > Next, disabling/enabling triggers is required for upgrade() = functionality > and is not a part of public API. =46rom user=E2=80=99s point of view checks are constraints. AFAIR = Konstantin asked for a handle to disable check and foreign key constraints. > As in further patches ck constraint are rely > on trigger machinery, we don't need a separate controller. We shouldn=E2=80=99t disable all NoSQL triggers, only check constraints. > So, I've reject xfer optimization when source or destination space has > ck constraints. This optimisation doesn=E2=80=99t seem to be vital. On the other hand, = if we can use it, why not to do so? > This patch introduces a new system space to persist check > constraints. Format of the space: >=20 > _ck_constraint (space id =3D 357) > [ STR, UINT, STR] Nit: you forgot about is_deferred field. > Because space alter, index alter and space truncate operations > cause space recreation, introduced RebuildCkConstrains object > that compile -> compiles > new ck constraint objects, replace and remove -> replaces and removes > existent instances atomically(when some compilation fails, > nothing changed). -> is 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 -> are going to > The main motivation for these changes is the ability to support > ADD CHECK CONSTRAINT operation in the future. CK constraints are > are easier to manage as self-sustained objects: we mustn=E2=80=99t What to do? > the tuple describing target space to do it(unlike the current > architecture). Can=E2=80=99t parse this sentence. Re-phrase it please. > Disabled xfer optimization when some space have ck constraints > because in the following patches this xfer optimisation becomes > impossible. No reason to rewrite this code. >=20 > Needed for #3691 > =E2=80=94 Please, don=E2=80=99t send the whole patch again, I expect only diff between versions. It takes a while to review such huge patch again.