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 96EF22A20A for ; Wed, 19 Sep 2018 15:19:41 -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 gf5pbyFdKKiN for ; Wed, 19 Sep 2018 15:19:41 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 D71E02A206 for ; Wed, 19 Sep 2018 15:19:40 -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 v1 1/1] sql: make sql checks on server side From: "n.pettik" In-Reply-To: <65293bc495744d8c8b6ce6fbb9e810a4901238dd.1537355900.git.kshcherbatov@tarantool.org> Date: Wed, 19 Sep 2018 22:19:38 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <65293bc495744d8c8b6ce6fbb9e810a4901238dd.1537355900.git.kshcherbatov@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 Firstly, sql-tap/selectG.test.lua fails on your branch. > To make SQL CHECKS on space insertion in LUA we have to > generate a temporal VDBE and execute it with a tuple > mapped in VDBE memory with new opcode OP_FieldMapMemory as > an argument. These actions are performed by a new special trigger > set for the space on server side. > The CHECKS are carried out twice for SQL requests because of > context and affinity changes to which the inserting tuple is > subjected. > At first, user could specify IGNORE keyword for SQL UPDATE that > is a part of PARSER CONTEXT and couldn't be handled on > server-side for now. If you mean =E2=80=98pragma ignore_checks=E2=80=99, then we are going to remove this option at all. So this shouldn=E2=80=99t be a problem. https://github.com/tarantool/tarantool/issues/3696 > The other point is that tuple represented as msgpack on > server-side may have already changed field data type. >=20 > E.g. > CREATE TABLE t2(id primary key, > z TEXT CONSTRAINT three > CHECK(typeof(coalesce(z,'')) =3D=3D 'text')); > INSERT INTO t2 VALUES(1, 3.14159); >=20 > Before the tuple for insertion into the server-side space was > generated, it's z field was of type REAL, which did not pass > the CHECK test. But then new compatible affinity BLOB is applied, > so making check on server-side no problems would be triggered > and tuple would be inserted successfully. This will be fixed when static types hit the trunk. Currently, patch is = under review. Hence, this shouldn=E2=80=99t be issue as well. To be honest, I think that triggers, foreign keys and checks should work in the same way. So you need kind of interface (or framework) to = run them. Personally mine point of view is following: we should compile check constraints once, and reuse the same VDBE = program - it would significantly speed up execution time. The same for triggers = and foreign keys. Eventually, it should look like (IMHO): vdbe_run(get_compiled_triggers(space)); vdbe_run(get_compiled_checks(space)); vdbe_run(get_compiled_fk(space)); Check can=E2=80=99t be changed now in any way (at least until we = introduce ALTER TABLE ADD/DROP CONSTRAINT CHECK), so this program can=E2=80=99t change. The same for FK: until constraint exists, it = can=E2=80=99t be changed, ergo VDBE program implementing FK checks can=E2=80=99t be modified as well. As for triggers - now we can refer to inexistent spaces in trigger=E2=80=99= s body, but I guess we may ban this opportunity without any regrets. To sump up, I suggest you to completely remove code generation for check constraints from SQL parser and move it to tnt trigger. Moreover, = investigate how we can store compiled program and then run it instead of parsing each time the same expression. Your approach works when it comes for checks - the easiest part of = integrity constraints. But I have doubts that it would work the same way for = triggers and foreign keys. One more general nit - comment your code, please. > Closes #3691. > --- > Branch: = https://github.com/tarantool/tarantool/tree/kshch/gh-3691-checks-on-server= -side > Issue: https://github.com/tarantool/tarantool/issues/3691 >=20 > src/box/alter.cc | 37 +++++++++++++++++++++ > src/box/sql.c | 67 +++++++++++++++++++++++++++++++++++++ > src/box/sql.h | 15 +++++++++ > src/box/sql/build.c | 2 +- > src/box/sql/insert.c | 86 = +++++++++++++++++++++++++++--------------------- > src/box/sql/sqliteInt.h | 27 ++++++++++++--- > src/box/sql/vdbe.c | 20 ++++++++++- > test/sql/checks.result | 47 ++++++++++++++++++++++++++ > test/sql/checks.test.lua | 17 ++++++++++ > 9 files changed, 275 insertions(+), 43 deletions(-) >=20 > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 1757332..4684fc6 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -39,6 +39,7 @@ > #include "coll_id_def.h" > #include "txn.h" > #include "tuple.h" > +#include "trigger.h" > #include "fiber.h" /* for gc_pool */ > #include "scoped_guard.h" > #include "third_party/base64.h" > @@ -1425,6 +1426,27 @@ on_drop_space_rollback(struct trigger *trigger, = void *event) > } >=20 > /** > + * SQL-specific actions for space. > + */ Too poor comment. it doesn=E2=80=99t even look like comment, sorry. I didn=E2=80=99t review the rest of code since it may change = significantly after reworking patch. > +static void > +on_replace_dd_space_sql(struct trigger *trigger, void *event) > +{