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 334EF2C165 for ; Thu, 16 May 2019 09:51:09 -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 8P3LHDjH1RG8 for ; Thu, 16 May 2019 09:51:09 -0400 (EDT) Received: from smtp33.i.mail.ru (smtp33.i.mail.ru [94.100.177.93]) (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 972DC29189 for ; Thu, 16 May 2019 09:51:08 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v3 0/3] box: run checks on insertions in LUA spaces References: From: Kirill Shcherbatov Message-ID: <18bfb586-1cc8-7bf1-4282-567bea8fbbdb@tarantool.org> Date: Thu, 16 May 2019 16:51:05 +0300 MIME-Version: 1.0 In-Reply-To: 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, Nikita Pettik Cc: Kostya Osipov , Vladislav Shpilevoy Hello everybody, The discussion has grown in many threads. I've gathered together everything that we have agreed and what has been done. Looks like we have a conception with this patch series with @korablev now. I'll resend whole patch a bit later. (careful: now it is based on kshch/gh-3691-sql-flags-in-parser branch) 1. @korablev [tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter 14.05.2019, 19:49: > > One unpleasant “feature” that I’ve noticed: > > > > create table t4 (id int primary key check(id > id*id)) > > insert into t4 values(0.5) > > - error: 'Failed to execute SQL statement: Check constraint failed ''CK_CONSTRAINT_1_T4'': > > id > id*id’ > > Please, document this “feature”. At first, we've discussed this problem with @kyukhin, @kostya, @pgulutzan again [dev] Implicit casting from FP -> INT 15.05.2019, 11:44 Re: [dev] cast 15.05.2019, 21:44 #4216 and decided that it's OK and we should live with it. I've included the following description in the last commit: Note: this patch changes the CK constraints execution order for SQL. Previously check of CK constraints integrity was fired before tuple is formed; meanwhile now they are implemented as NoSQL before replace triggers, which are fired right before tuple insertion. In turn, type casts are performed earlier than msgpack serialization. You should be careful with functions that use field types in your check constrains (like typeof()). Consider following situation: ``` box.execute("CREATE TABLE t2(id INT primary key, x INTEGER CHECK (x > 1));") box.execute("INSERT INTO t2 VALUES(3, 1.1)") ``` the last operation would fail because 1.1 is silently casted to integer 1 which is not greater than 1. 2. @kostya [tarantool-patches] Re: [PATCH v3 1/3] schema: add new system space for CHECK constraints 14.05.2019, 21:29 > Please make sure at least the space definition contains all the > fields necessary for information_schema, even if this patch > doesn't fill in some of the fields yet. > > Does it? What's in information_schema.check_constraints? What do > other vendors store in this system space (like deferrable, > initially deferred). https://docs.microsoft.com/en-us/sql/relational-databases/system-information-schema-views/check-constraints-transact-sql?view=sql-server-2017 https://dev.mysql.com/doc/refman/8.0/en/check-constraints-table.html https://mariadb.com/kb/en/library/information-schema-check_constraints-table/ https://www.postgresql.org/docs/9.1/infoschema-check-constraints.html Have really similar structure: | constraint_catalog - Name of the database containing the constraint (always the current database) | constraint_schema - Name of the schema containing the constraint | constraint_name - Name of the constraint | check_clause - The check expression of the check constraint Looks like current format of _ck_constraint comprises all of these fields. 3. @kostya [tarantool-patches] Re: [PATCH v3 3/3] box: user-friendly interface to manage ck constraints 14.05.2019, 23:09 > Imagine you have stored Lua functions. Don't you want to be able > to define Lua check constraints from Lua? > > Which begs a question, should _ck_constraint space contain > 'language' field just in case you'd want to add this inthe future? > > Looks a bit weird that you use Lua to define an SQL constraint, > doesn't it? > > But overall this is pretty cool already. Ok, then we are going to extend space format to make it hold language of expression. 4. @kostya [tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter 07.05.2019, 19:39 > > + /* Compile VDBE with default sql parameters. */ > > + struct session *user_session = current_session(); > > + uint32_t sql_flags = user_session->sql_flags; > > + user_session->sql_flags = default_flags; > > This is a time bomb from technical debt point of view. Please pass > the flags to sql_parser_create instead, which will then pass them > to vdbe. I agree with your position, so that I implemented this change in a separate commit series. Now ck constraint(s) branch is based on it. 5. @kostya [tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter 07.05.2019, 19:39 > This looks like a pessimization to me. Depending on the code flow, > some of the tuple fields may not be accessed at all. Is it really > necessary to decode them so agressibvely here? Especially since > you encode *all* space fields. @kshcherbatov [tarantool-patches] Re: [tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter 07.05.2019, 20:47 > I'll try to walk though the AST tree and prepare the map of fields that > are involved in expression (on check compile operation); Here there > would be binding of **used** fields. > What do you think? @korablev [tarantool-patches] Re: [PATCH v3 0/3] box: run checks on insertions in LUA spaces 14.05.2019, 21:22 > Add to ck_constraint array of used field numbers - that’s done > during ck_constraint_program_compile() while we have struct > Expr by traversing AST. Then, we emit OP_Variable ck_field_count > times, where ck_field_count is length of array of used field > numbers. I've solved this problem using smart column mask for AST-required fields. It is a separate patch now. It's good enough, I think. Regards, Kirill Shcherbatov