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 AE65026654 for ; Thu, 25 Apr 2019 16:38:46 -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 jAx95mA05V4S for ; Thu, 25 Apr 2019 16:38:46 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 D15DD2CBD2 for ; Thu, 25 Apr 2019 16:38:45 -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: Date: Thu, 25 Apr 2019 23:38:42 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: 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 > On 16 Apr 2019, at 16:51, Kirill Shcherbatov = wrote: >=20 > 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] >=20 > CK constraint is local to space, so every pair > is unique (and it is PK in _ck_constraint space). >=20 > After insertion into this space, a new instance describing check > constraint is created. Check constraint holds Expr AST tree. Nit: AST includes =E2=80=99tree=E2=80=99 word :) -> =E2=80=98holds AST = of expression representing it=E2=80=99. > 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. >=20 > Because space alter, index alter and space truncate operations > case -> cause, I guess. > space recreation, introduced RebuildCkConstrains object > that compile new ck constraint objects, replace and remove > existent instances atomically(when some compilation fails, > nothing changed). Still don=E2=80=99t understand necessity of re-creating check = constraints in case of truncation or index alteration. Truncate can turn out to be quite expensive in terms of performance since we will have to rebuild and recompile all check, foreign key constraints and SQL triggers. > 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. >=20 Why can=E2=80=99t we implement this statement without separate space? > --- >=20 > /* }}} */ > @@ -4035,6 +4098,161 @@ on_replace_dd_fk_constraint(struct trigger * = /* trigger*/, void *event) > } > } >=20 > +/** Create an instance of check constraint definition by tuple. */ > +static struct ck_constraint_def * > +ck_constraint_def_new_from_tuple(struct tuple *tuple) > +{ > + uint32_t name_len; > + const char *name =3D > + tuple_field_str_xc(tuple, BOX_CK_CONSTRAINT_FIELD_NAME, > + &name_len); > + if (name_len > BOX_NAME_MAX) { > + tnt_raise(ClientError, ER_CREATE_CK_CONSTRAINT, > + tt_cstr(name, BOX_INVALID_NAME_MAX), > + "check constraint name is too long"); > + } > + identifier_check_xc(name, name_len); > + uint32_t expr_str_len; > + const char *expr_str =3D > + tuple_field_str_xc(tuple, = BOX_CK_CONSTRAINT_FIELD_EXPR_STR, > + &expr_str_len); > + > + uint32_t name_offset, expr_str_offset; > + size_t ck_def_sz =3D > + ck_constraint_def_sizeof(name_len, expr_str_len, = &name_offset, > + &expr_str_offset); Nit: function returns uint_32. Please, make types be consistent. > diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c > new file mode 100644 > index 000000000..daeebb78b > --- /dev/null > +++ b/src/box/ck_constraint.c > @@ -0,0 +1,115 @@ >=20 > +#include "ck_constraint.h" > +#include "errcode.h" > +#include "small/rlist.h=E2=80=9D This header is already included in ck_constraint.h > +#include "sql.h" > +#include "sql/sqlInt.h=E2=80=9D It=E2=80=99s awful that we keep on including this header... > + > +/** > + * Resolve space_def references for check constraint via AST > + * tree traversal. > + * @param ck_constraint Check constraint object to update. > + * @param space_def Space definition to use. > + * @retval 0 on success. > + * @retval -1 on error. > + */ > +static int > +ck_constraint_resolve_space_def(struct Expr *expr, > + struct space_def *space_def) > +{ I would call this method like =E2=80=9Cck_constraint_resolve_names=E2=80=9D= or =E2=80=9Cck_constraint_resolve_field_names=E2=80=9D. IMHO we can=E2=80=99t= resolve space definition and it may sound confusing... > diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h > new file mode 100644 > index 000000000..615612605 > --- /dev/null > +++ b/src/box/ck_constraint.h > @@ -0,0 +1,163 @@ >=20 > +/** > + * Find check constraint object in space by given name and > + * name_len. > + * @param space The space to lookup check constraint. > + * @param name The check constraint name. > + * @param name_len The length of the name. > + * @retval not NULL Check constrain if exists, NULL otherwise. > + */ > +struct ck_constraint * This function is used only in alter.cc, mb it is worth moving it to = alter.cc and make it static? > +space_ck_constraint_by_name(struct space *space, const char *name, > + uint32_t name_len); > + >=20 > diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua > index f31cf7f2c..e01f500e6 100644 > --- a/src/box/lua/schema.lua > +++ b/src/box/lua/schema.lua >=20 > local function create_sysview(source_id, target_id) > @@ -735,6 +737,43 @@ local function upgrade_to_2_1_3() > id =3D id + 1 > end > end > + This upgrade should be part of 2.2.0, I guess. > + -- In previous Tarantool releases check constraints were > + -- stored in space opts. Now we use separate space > + -- _ck_constraint for this purpose. Perform legacy data > + -- migration. > diff --git a/src/box/schema_def.h b/src/box/schema_def.h > +/** _ck_constraint fields. */ > +enum { > + BOX_CK_CONSTRAINT_FIELD_NAME =3D 0, > + BOX_CK_CONSTRAINT_FIELD_SPACE_ID =3D 1, > + BOX_CK_CONSTRAINT_FIELD_EXPR_STR =3D 2, In addition to these properties mb it is worth adding field: ANSI says that every constraint can be created with this option. I suggest to do it now to avoid changing system spaces in future. Note that _fk_constraints already has this attribute. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index b1ed64a01..e55bbe43b 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -47,6 +47,7 @@ > #include "vdbeInt.h" > #include "tarantoolInt.h" > #include "box/box.h" > +#include "box/ck_constraint.h" > #include "box/fk_constraint.h" > #include "box/sequence.h" > #include "box/session.h" > @@ -643,37 +644,74 @@ primary_key_exit: > void > sql_add_check_constraint(struct Parse *parser) Please, make name of this function consistent with sql_create_foreign_key(). i.e. rename _add_check_...() to sql_create_check_contraint() or vice versa. > { > - struct create_ck_def *ck_def =3D &parser->create_ck_def; > + struct create_ck_def *create_ck_def =3D &parser->create_ck_def; > + struct ExprSpan *expr_span =3D create_ck_def->expr; > + sql_expr_delete(parser->db, expr_span->pExpr, false); Looks inefficient: we delete expr right after its creation.. Could we come up with workaround to avoid this? I mean cut string containing expression without allocating/releasing memory for it and without filling in expr tree? > + uint32_t name_len =3D strlen(name); Nit: strlen() return size_t type. > + > + uint32_t expr_str_len =3D (uint32_t)(create_ck_def->expr->zEnd - > + create_ck_def->expr->zStart); > + const char *expr_str =3D create_ck_def->expr->zStart; > + > + /* > + * Allocate memory for ck constraint parse structure and > + * ck constraint definition as a single memory chunk on > + * region: > + * > + * [ck_parse][ck_def[name][expr_str]] > + * |_____^ |___^ ^ > + * |_________| > + */ > + uint32_t name_offset, expr_str_offset; > + size_t ck_def_sz =3D > + ck_constraint_def_sizeof(name_len, expr_str_len, = &name_offset, > + &expr_str_offset); Nit: ck_constraint_def_sizeof() return uint32_t Also why do we not use vdbe_emit_halt_with_presence_test() during creation of check constraints? 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 ... > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index c2aac553f..a075d4dc3 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c >=20 > @@ -1245,14 +1239,26 @@ xferOptimization(Parse * pParse, /* = Parser context */ > if (pSrcIdx =3D=3D NULL) > return 0; > } > - /* Get server checks. */ > - ExprList *pCheck_src =3D src->def->opts.checks; > - ExprList *pCheck_dest =3D dest->def->opts.checks; > - if (pCheck_dest !=3D NULL && > - sqlExprListCompare(pCheck_src, pCheck_dest, -1) !=3D 0) { > - /* Tables have different CHECK constraints. Ticket = #2252 */ > + /* > + * Dissallow the transfer optimization if the check > + * constraints are differ. > + */ > + if (rlist_empty(&dest->ck_constraint) !=3D > + rlist_empty(&src->ck_constraint)) > return 0; > + struct rlist *dst_ck_link =3D &dest->ck_constraint; > + struct ck_constraint *src_ck; > + rlist_foreach_entry(src_ck, &src->ck_constraint, link) { > + dst_ck_link =3D rlist_next(dst_ck_link); > + if (dst_ck_link =3D=3D &dest->ck_constraint) > + return 0; > + struct ck_constraint *dest_ck =3D > + rlist_entry(dst_ck_link, struct ck_constraint, = link); > + if (sqlExprCompare(src_ck->expr, dest_ck->expr, -1) !=3D = 0) > + return 0; > } > + if (rlist_next(dst_ck_link) !=3D &dest->ck_constraint) > + return 0; 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. > diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h > index a1af2bacd..ec434420d 100644 > --- a/src/box/sql/parse_def.h > +++ b/src/box/sql/parse_def.h > @@ -123,6 +123,20 @@ struct fk_constraint_parse { > struct rlist link; > }; >=20 > +/** > + * Structure representing check constraint appeared within > + * CREATE TABLE statement. Used only during parsing. > + */ > +struct ck_constraint_parse { > + /** > + * Check constraint declared in > + * statement. Must be coded after space creation. Nit: Definition of check constraints=E2=80=A6Mention that they don=E2=80=99t need any cleanups, since they are allocated using region. > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index b1ec8c758..531e29ed5 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -6445,7 +6445,12 @@ sql_expr_extract_select(struct Parse *parser, = struct Select *select) > struct ExprList *expr_list =3D select->pEList; > assert(expr_list->nExpr =3D=3D 1); > parser->parsed_ast_type =3D AST_TYPE_EXPR; > - parser->parsed_ast.expr =3D sqlExprDup(parser->db, > - expr_list->a->pExpr, > - EXPRDUP_REDUCE); > + /* > + * Extract a copy of parsed expression. > + * We cannot use EXPRDUP_REDUCE flag in sqlExprDup call > + * because some compiled Expr (like Checks expressions) > + * may require further resolve with sqlResolveExprNames. Agh, still can=E2=80=99t point out what does this flag mean. Could you = please briefly explain? > + */ > + parser->parsed_ast.expr =3D > + sqlExprDup(parser->db, expr_list->a->pExpr, 0); > } >=20 > /** > diff --git a/test/app-tap/tarantoolctl.test.lua = b/test/app-tap/tarantoolctl.test.lua > index 62a78d6bf..9ac0b5c69 100755 > --- a/test/app-tap/tarantoolctl.test.lua > +++ b/test/app-tap/tarantoolctl.test.lua > @@ -352,7 +352,7 @@ do >=20 > local filler_code =3D [[ > box.cfg{memtx_memory =3D 104857600, background=3Dfalse} > - local space =3D box.schema.create_space("test") > + space =3D box.schema.create_space("test=E2=80=9D) Why this diff is needed? > diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua > index b01afca7c..0dda3fac8 100755 > --- a/test/sql-tap/check.test.lua > +++ b/test/sql-tap/check.test.lua > @@ -319,7 +319,7 @@ test:do_catchsql_test( > ); > ]], { > -- > - 1, "Failed to create space 'T3': Subqueries are prohibited in = a CHECK constraint definition" > + 1, "Failed to create check constraint 'CK_CONSTRAINT_1_T3': = Subqueries are prohibited in a check constraint definition=E2=80=9D In some error messages "check=E2=80=9D word is uppercased, in the rest - = isn=E2=80=99t. Please, make this rule be consistent.=20 > diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua > index 5bfcf12f8..0cdfde2e9 100644 > --- a/test/sql/checks.test.lua > +++ b/test/sql/checks.test.lua > @@ -8,41 +8,90 @@ box.execute('pragma = sql_default_engine=3D\''..engine..'\'') > -- gh-3272: Move SQL CHECK into server > -- > -opts =3D {checks =3D {{expr =3D 'X>5', name =3D 'ONE'}}} > -format =3D {{name =3D 'X', type =3D 'unsigned'}} > -t =3D {513, 1, 'test', 'memtx', 0, opts, format} > -s =3D box.space._space:insert(t) > -box.space._space:delete(513) > +-- Invalid expression test. > +box.space._ck_constraint:insert({'CK_CONSTRAINT_01', 513, 'X><5'}) > +-- Unexistent space test. > +box.space._ck_constraint:insert({'CK_CONSTRAINT_01', 550, 'X<5'}) > +-- Pass integer instead of expression. > +box.space._ck_constraint:insert({'CK_CONSTRAINT_01', 550, 666}) Please, pass valid space id in this particular test (to void confusion). > +-- Test xferOptimization for space that have CK constraints. > +box.execute("CREATE TABLE first (id FLOAT PRIMARY KEY CHECK(id < 5), = a INT CONSTRAINT ONE CHECK(a < 5));") > +box.execute("CREATE TABLE second (id FLOAT PRIMARY KEY CHECK(id < 5), = a INT CONSTRAINT ONE CHECK(a < 5));") > +box.execute("CREATE TABLE third (id FLOAT PRIMARY KEY, a INT = CONSTRAINT ONE CHECK(a < 5));") > +box.execute("INSERT INTO first VALUES(1, 1);") > +box.execute("INSERT INTO first VALUES(2, 2);") > +box.execute("INSERT INTO first VALUES(3, 3);") > +box.execute("INSERT OR IGNORE INTO second SELECT * FROM first;") > +box.execute("INSERT OR IGNORE INTO third SELECT * FROM first;") > +box.execute("DELETE FROM second;") > +box.execute("INSERT OR IGNORE INTO second SELECT * FROM third;=E2=80=9D= ) How does this check that xFer was really fired? > +-- Test space creation rollback on spell error in ck constraint. > +box.execute("CREATE TABLE first (id FLOAT PRIMARY KEY CHECK(id < 5), = a INT CONSTRAINT ONE CHECK(a >< 5));") > +box.space.FIRST =3D=3D nil > +box.space._ck_constraint:count() =3D=3D 0 > +box.space.FIRST:drop() Isn=E2=80=99t this redundant check? I mean box.space.FIRST =3D=3D nil = already checks that space hasn=E2=80=99t been created. > +-- Ck constraints are disallowed for spaces having no format. > +s =3D box.schema.create_space('test') > +_ =3D s:create_index('pk') > +_ =3D box.space._ck_constraint:insert({'physics', s.id, 'X +s:format({{name=3D'X', type=3D'integer'}, {name=3D'Y', = type=3D'integer'}}) > +_ =3D box.space._ck_constraint:insert({'physics', s.id, 'X +box.execute("INSERT INTO \"test\" VALUES(2, 1);") > +s:format({{name=3D'Y', type=3D'integer'}, {name=3D'X', = type=3D'integer'}}) > +box.execute("INSERT INTO \"test\" VALUES(1, 2);") > +box.execute("INSERT INTO \"test\" VALUES(2, 1);") > +s:truncate() > +box.execute("INSERT INTO \"test\" VALUES(1, 2);") > +s:format({}) Good test scenario. Also, it would be nice to see a few complex expression. For instance, involving CAST, built-in functions and other allowed in check exprs constructions. > diff --git a/test/sql/upgrade.test.lua b/test/sql/upgrade.test.lua > index b76a8f373..cfda74a08 100644 > --- a/test/sql/upgrade.test.lua > +++ b/test/sql/upgrade.test.lua > @@ -62,6 +62,11 @@ s ~=3D nil > i =3D box.space._index:select(s.id) > i ~=3D nil > i[1].opts.sql =3D=3D nil > +box.space._space:get(s.id).flags.checks =3D=3D nil > +check =3D box.space._ck_constraint:select()[1] > +check ~=3D nil > +check.name > +check.expr_str Am I right that last time snapshot to be upgraded was changed, it was filled with space featuring check constraint? I see that you = didn=E2=80=99t change snapshot, but space has check constraint after upgrade.