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 3D479295C2 for ; Mon, 1 Apr 2019 15:45: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 bJMFMbP7ZLZb for ; Mon, 1 Apr 2019 15:45: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 81A5827FC7 for ; Mon, 1 Apr 2019 15:45:45 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2 5/9] schema: add new system space for CHECK constraints From: "n.pettik" In-Reply-To: <7ef2f5ed-831d-dd55-1a5e-9dfa0b33bd0f@tarantool.org> Date: Mon, 1 Apr 2019 22:45:42 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <4092AC4B-4CED-48E8-9540-EF348802DC17@tarantool.org> References: <186b109d1f6049b1413fe0cdded43e5fad29a895.1548838034.git.kshcherbatov@tarantool.org> <7ef2f5ed-831d-dd55-1a5e-9dfa0b33bd0f@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 >>> + ++schema_version; >>=20 >> 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. >=20 See no fixes to this comment. I=E2=80=99ve found way to make Tarantool hang: box.sql.execute("create table t(id int primary key check (id > 5))") drop_ck =3D function() = box.space._ck_constraint:delete({'CK_CONSTRAINT_1_T', box.space.T.id}) = end fiber =3D require('fiber') box.error.injection.set("ERRINJ_WAL_DELAY", true) f2 =3D 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. > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D >=20 > This patch introduces 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 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. > diff --git a/src/box/alter.cc b/src/box/alter.cc > index fb668aa4c..9aa5e3653 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc >=20 > @@ -1846,6 +1898,8 @@ on_replace_dd_space(struct trigger * /* trigger = */, void *event) > def->field_count); > (void) new CheckSpaceFormat(alter); > (void) new ModifySpace(alter, def); > + /* Add an op to rebuild check constraints. */ > + (void) new BuildCkConstraints(alter); > def_guard.is_active =3D false; > /* Create MoveIndex ops for all space indexes. */ > alter_space_move_indexes(alter, 0, = old_space->index_id_max + 1); > @@ -2088,6 +2142,7 @@ on_replace_dd_index(struct trigger * /* trigger = */, void *event) > * old space. > */ > 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? > /* Add an op to update schema_version on commit. */ > (void) new UpdateSchemaVersion(alter); > alter_space_do(txn, alter); > @@ -2155,7 +2210,7 @@ on_replace_dd_truncate(struct trigger * /* = trigger */, void *event) > struct index *old_index =3D old_space->index[i]; > (void) new TruncateIndex(alter, old_index->def->iid); > } > - > + (void) new BuildCkConstraints(alter); Looks like a nonsense: why do we need to rebuild ck constraints on = truncate? > @@ -4064,6 +4119,145 @@ on_replace_dd_fk_constraint(struct trigger * = /* trigger*/, void *event) > } > } >=20 > +/** > + * Create an instance of check constraint definition from tuple > + * on region. > + */ > +static struct ck_constraint_def * > +ck_constraint_def_decode(const struct tuple *tuple, struct region = *region) 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 > +{ > + 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; > + uint32_t sz =3D ck_constraint_def_sizeof(name_len, expr_str_len, > + &name_offset, = &expr_str_offset); > + struct ck_constraint_def *ck_constraint_def =3D > + (struct ck_constraint_def *)region_alloc_xc(region, sz); > + ck_constraint_def_create(ck_constraint_def, name, name_len, = expr_str, > + expr_str_len); For each call of ck_constraint_def_create() you invoke = ck_constraint_def_size() twice: first time to allocate space for struct ck_def and second one to = compose memory layout for def+impl. Why can=E2=80=99t you move memory allocation and ck_sizeof inside ck_constraint_def_create(), make it return pointer to a constructed = object? Combining this with allocation using malloc (see comment below), code would look much better IMHO. > + return ck_constraint_def; > +} > + > +/** Trigger invoked on rollback in the _ck_constraint space. */ > +static void > +on_replace_ck_constraint_rollback(struct trigger *trigger, void = *event) > +{ > + struct txn_stmt *stmt =3D txn_last_stmt((struct txn*) event); > + struct ck_constraint *ck_constraint =3D > + (struct ck_constraint *)trigger->data; > + struct space *space =3D NULL; > + if (ck_constraint !=3D NULL) > + space =3D space_by_id(ck_constraint->space_id); > + if (stmt->old_tuple !=3D NULL && stmt->new_tuple =3D=3D NULL) { > + /* Rollback DELETE check constraint. */ > + if (ck_constraint =3D=3D NULL) > + return; But in on_replace_dd_ck_constraint() you have next check: assert(old_ck_constraint !=3D NULL); =E2=80=A6 on_rollback->data =3D old_ck_constraint; How ck_constraint could be NULL? > + assert(space !=3D NULL); > + rlist_add_entry(&space->ck_constraint, ck_constraint, = link); > + } else if (stmt->new_tuple !=3D NULL && stmt->old_tuple =3D=3D = NULL) { > + /* Rollback INSERT check constraint. */ > + assert(space !=3D NULL); > + rlist_del_entry(ck_constraint, link); > + ck_constraint_delete(ck_constraint); > + } else { > + /* Rollback REPLACE check constraint. */ > + assert(space !=3D NULL); > + const char *space_name =3D ck_constraint->def->name; It=E2=80=99s constraint=E2=80=99s name. > + struct ck_constraint *new_ck_constraint =3D > + space_ck_constraint_by_name(space, space_name, > + strlen(space_name)); > + assert(new_ck_constraint !=3D NULL); > + rlist_del_entry(new_ck_constraint, link); > + rlist_add_entry(&space->ck_constraint, ck_constraint, = link); > + ck_constraint_delete(new_ck_constraint); > + } > +} >=20 > +/** A trigger invoked on replace in the _ck_constraint space. */ > +static void > +on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void = *event) > +{ > + struct txn *txn =3D (struct txn *) event; > + txn_check_singlestatement_xc(txn, "Space _ck_constraint"); > + struct txn_stmt *stmt =3D txn_current_stmt(txn); > + struct tuple *old_tuple =3D stmt->old_tuple; > + struct tuple *new_tuple =3D stmt->new_tuple; > + uint32_t space_id =3D > + tuple_field_u32_xc(old_tuple !=3D NULL ? old_tuple : = new_tuple, > + BOX_CK_CONSTRAINT_FIELD_SPACE_ID); > + struct space *space =3D space_cache_find_xc(space_id); > + access_check_ddl(space->def->name, space->def->id, = space->def->uid, > + SC_SPACE, PRIV_A); > + See comments to previous patch concerning privileges. > + struct trigger *on_rollback =3D > + txn_alter_trigger_new(on_replace_ck_constraint_rollback, = NULL); > + struct trigger *on_commit =3D > + txn_alter_trigger_new(on_replace_ck_constraint_commit, = NULL); > + > + if (new_tuple !=3D NULL) { > + /* Create or replace check constraint. */ > + struct ck_constraint_def *ck_constraint_def =3D > + ck_constraint_def_decode(new_tuple, = &fiber()->gc); > + struct ck_constraint *new_ck_constraint =3D > + ck_constraint_new(ck_constraint_def, = space->def); > + if (new_ck_constraint =3D=3D NULL) > + diag_raise(); > + const char *space_name =3D new_ck_constraint->def->name; But this is not space name, it=E2=80=99s name of constraint. > + struct ck_constraint *old_ck_constraint =3D > + space_ck_constraint_by_name(space, space_name, > + strlen(space_name)); > + if (old_ck_constraint !=3D NULL) > + rlist_del_entry(old_ck_constraint, link); > + rlist_add_entry(&space->ck_constraint, = new_ck_constraint, link); > + on_commit->data =3D old_ck_constraint; > + on_rollback->data =3D old_tuple =3D=3D NULL ? = new_ck_constraint : > + = old_ck_constraint; >=20 > diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c > new file mode 100644 > index 000000000..110098efc > --- /dev/null > +++ b/src/box/ck_constraint.c > +uint32_t > +ck_constraint_def_sizeof(uint32_t name_len, uint32_t expr_str_len, > + uint32_t *name_offset, uint32_t = *expr_str_offset) > +{ > + *name_offset =3D sizeof(struct ck_constraint_def); > + *expr_str_offset =3D *name_offset + (name_len !=3D 0 ? name_len = + 1 : 0); Why name_len can be 0? Isn=E2=80=99t name is mandatory field? The same question about expr_str_len. > + return *expr_str_offset + (expr_str_len !=3D 0 ? expr_str_len + = 1 : 0); > +} > + > +void > +ck_constraint_def_create(struct ck_constraint_def *ck_constraint_def, > + const char *name, uint32_t name_len, > + const char *expr_str, uint32_t expr_str_len) > +{ > + uint32_t name_offset, expr_str_offset; > + (void)ck_constraint_def_sizeof(name_len, expr_str_len, = &name_offset, > + &expr_str_offset); > + ck_constraint_def->name =3D (char *)ck_constraint_def + = name_offset; > + sprintf(ck_constraint_def->name, "%.*s", name_len, name); Why don=E2=80=99t you use memcpy? > + ck_constraint_def->expr_str =3D > + (char *)ck_constraint_def + expr_str_offset; > + sprintf(ck_constraint_def->expr_str, "%.*s", expr_str_len, = expr_str); > + rlist_create(&ck_constraint_def->link); > +} > + > +struct ck_constraint * > +ck_constraint_new(const struct ck_constraint_def *ck_constraint_def, > + struct space_def *space_def) > +{ > + uint32_t ck_constraint_name_len =3D = strlen(ck_constraint_def->name); > + uint32_t expr_str_len =3D strlen(ck_constraint_def->expr_str); > + uint32_t name_offset, expr_str_offset; > + uint32_t ck_constraint_def_sz =3D > + ck_constraint_def_sizeof(ck_constraint_name_len, = expr_str_len, > + &name_offset, = &expr_str_offset); > + uint32_t ck_constraint_sz =3D sizeof(struct ck_constraint) + > + ck_constraint_def_sz; > + struct ck_constraint *ck_constraint =3D calloc(1, = ck_constraint_sz); > + if (ck_constraint =3D=3D NULL) { > + diag_set(OutOfMemory, ck_constraint_sz, "malloc", > + "ck_constraint"); > + return NULL; > + } > + rlist_create(&ck_constraint->link); > + ck_constraint->space_id =3D space_def->id; > + ck_constraint->def =3D > + (struct ck_constraint_def *)((char *)ck_constraint + > + sizeof(struct = ck_constraint)); > + ck_constraint_def_create(ck_constraint->def, = ck_constraint_def->name, > + ck_constraint_name_len, > + ck_constraint_def->expr_str, = expr_str_len); 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. > + struct Expr *expr =3D > + sql_expr_compile(sql_get(), ck_constraint_def->expr_str, > + expr_str_len); > + if (expr =3D=3D NULL || > + ck_constraint_resolve_ref(expr, space_def) !=3D 0) { > + diag_set(ClientError, ER_CREATE_CK_CONSTRAINT, > + ck_constraint->def->name, > + box_error_message(box_error_last())); > + goto error; > + } > + ck_constraint->expr =3D expr; > + > + return ck_constraint; > +error: > + ck_constraint_delete(ck_constraint); > + return NULL; > +} >=20 > diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h > new file mode 100644 > index 000000000..02aa525ce > --- /dev/null > +++ b/src/box/ck_constraint.h >=20 > +/** > + * Definition of check constraint. > + * The memory of size calculated with ck_constraint_def_sizeof > + * must be allocated manually and must be initialized with routine > + * ck_constraint_def_create. > + */ > +struct ck_constraint_def { > + /** > + * The name of the check constraint is used for error > + * reporting. Must be unique for a given space. It is used not only for error reporting. Constraint names are *going to be* used to create and drop separate entities: ALTER TABLE t ADD CONSTRAINT ck1 CHECK(a > 5); ALTER TABLE t DROP CONSTRAINT ck1; > + */ > + char *name; > + /** > + * The string describing an check constraint expression. > + */ Expand comment with example: For instance: "field1 + field2 > 2 * 3" > + char *expr_str; > + /** > + * Organize check_def structs into linked list with > + * Parse::new_ck_constraint. > + */ > + struct rlist link; You can make link be first member of struct and place name at the end of struct (like in space_def or fk_def). > +}; > + > +/* Structure representing check constraint object. */ > +struct ck_constraint { > + /** > + * The check constraint definition. > + */ Please, save space using one-line comments. Or what is better, don=E2=80=99t add useless comments :) > diff --git a/src/box/errcode.h b/src/box/errcode.h > index 7764aa352..83f719225 100644 > --- a/src/box/errcode.h > +++ b/src/box/errcode.h > @@ -240,6 +240,9 @@ struct errcode_record { > /*185 */_(ER_SQL_UNKNOWN_TOKEN, "Syntax error: = unrecognized token: '%.*s'") \ > /*186 */_(ER_SQL_PARSER_GENERIC, "%s") \ > /*187 */_(ER_SQL_ANALYZE_ARGUMENT, "ANALYZE statement = argument %s is not a base table") \ > + /*188 */_(ER_CREATE_CK_CONSTRAINT, "Failed to create check = constraint '%s': %s") \ > + /*189 */_(ER_CK_CONSTRAINT_FAILED, "Check constraint = failed: %s") \ > + Nit: extra empty line. 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=E2=80=9D it=E2=80=99s quite complicated to understand which constraint failed. So, I suggest to add to this error string of failed condition. > diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua > index dc7328714..492834b28 100644 > --- a/src/box/lua/upgrade.lua > +++ b/src/box/lua/upgrade.lua >=20 > local function create_sysview(source_id, target_id) > @@ -627,7 +629,39 @@ local function upgrade_to_2_1_1() I suppose this should be part of upgrade to 2_2_0 script. >=20 > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 0c0655543..3197cde0c 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -661,31 +661,55 @@ primary_key_exit: > } >=20 > void > -sql_add_check_constraint(struct Parse *parser, struct ExprSpan *span) > +sql_add_ck_constraint(struct Parse *parser, struct ExprSpan *span) > { > + const char *ck_constraint_name =3D NULL; > + if (parser->constraintName.n !=3D 0) { > + ck_constraint_name =3D > + region_alloc(region, parser->constraintName.n + = 1); > + if (ck_constraint_name =3D=3D NULL) { > + diag_set(OutOfMemory, parser->constraintName.n + = 1, > + "region_alloc", "ck_constraint_name"); > + parser->is_aborted =3D true; > + goto out; > } > + sprintf((char *)ck_constraint_name, "%.*s", > + parser->constraintName.n, = parser->constraintName.z); Why not memcpy? > + rlist_add_entry(&parser->new_ck_constraint, ck_constraint_def, = link); > +out: > + sql_expr_delete(parser->db, expr, false); > + return; Nit: redundant return statement. > +/** > + * Generate opcodes to serialize check constraint definition into > + * MsgPack and insert produced tuple into _ck_constraint space. > + * @param parser Parsing context. > + * @param ck_constraint_def Check constraint definition to be > + * serialized. > + * @param reg_space_id The VDBE containing space id. > +*/ > +static void > +vdbe_emit_ck_constraint_create(struct Parse *parser, > + const struct ck_constraint_def = *ck_constraint_def, > + uint32_t reg_space_id) > +{ > + struct sql *db =3D parser->db; > + struct Vdbe *v =3D sqlGetVdbe(parser); > + assert(v !=3D NULL); > + int ck_constraint_reg =3D sqlGetTempRange(parser, 4); > + sqlVdbeAddOp4(v, OP_String8, 0, ck_constraint_reg, 0, > + sqlDbStrDup(db, ck_constraint_def->name), > + P4_DYNAMIC); A bit broken indentation. > + sqlVdbeAddOp2(v, OP_SCopy, reg_space_id, ck_constraint_reg + 1); > + sqlVdbeAddOp4(v, OP_String8, 0, ck_constraint_reg + 2, 0, > + sqlDbStrDup(db, ck_constraint_def->expr_str), > + P4_DYNAMIC); > + sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 3, > + ck_constraint_reg + 3); > + sqlVdbeAddOp3(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, 0, > + ck_constraint_reg + 3); > + save_record(parser, BOX_CK_CONSTRAINT_ID, ck_constraint_reg, 2, > + v->nOp - 1); > + sqlReleaseTempRange(parser, ck_constraint_reg, 4); > + return; Nit: redundant return statement. > +/** > + * Generate VDBE program to remove entry from _ck_constraint space. > + * > + * @param parser Parsing context. > + * @param ck_constraint_name Name of CK constraint to be dropped. > + * @param child_id Id of table which constraint belongs to. > + */ > +static void > +vdbe_emit_ck_constraint_drop(struct Parse *parser, > + const char *ck_constraint_name, uint32_t = space_id) > +{ > + struct Vdbe *v =3D sqlGetVdbe(parser); > + struct sql *db =3D v->db; > + assert(v !=3D NULL); > + int key_reg =3D sqlGetTempRange(parser, 3); > + sqlVdbeAddOp4(v, OP_String8, 0, key_reg, 0, > + sqlDbStrDup(db, ck_constraint_name), > + P4_DYNAMIC); A bit broken indentation. > + sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg + 1); > + const char *error_msg =3D > + tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), > + ck_constraint_name); > + if (vdbe_emit_halt_with_presence_test(parser, = BOX_CK_CONSTRAINT_ID, 0, > + key_reg, 2, = ER_NO_SUCH_CONSTRAINT, > + error_msg, false, > + OP_Found) !=3D 0) > + return; > + sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2); > + sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2); Please, add to byte code comment like in _fk_constraint_drop -=20 it makes bytecode easier to read. > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 6f7f02040..2fe74a027 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -919,34 +919,29 @@ vdbe_emit_constraint_checks(struct Parse = *parse_context, struct space *space, > if (on_conflict =3D=3D ON_CONFLICT_ACTION_DEFAULT) > on_conflict =3D ON_CONFLICT_ACTION_ABORT; > /* Test all CHECK constraints. */ > - struct ExprList *checks =3D def->opts.checks; > enum on_conflict_action on_conflict_check =3D on_conflict; > if (on_conflict =3D=3D ON_CONFLICT_ACTION_REPLACE) > on_conflict_check =3D ON_CONFLICT_ACTION_ABORT; > - if (checks !=3D NULL) { > + if (!rlist_empty(&space->ck_constraint)) > parse_context->ckBase =3D new_tuple_reg; > + struct ck_constraint *ck_constraint; > + rlist_foreach_entry(ck_constraint, &space->ck_constraint, link) = { > + struct Expr *expr =3D ck_constraint->expr; > + if (is_update && checkConstraintUnchanged(expr, = upd_cols) !=3D 0) > + continue; > + int all_ok =3D sqlVdbeMakeLabel(v); > + sqlExprIfTrue(parse_context, expr, all_ok, = SQL_JUMPIFNULL); > + if (on_conflict =3D=3D 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. > + } else { > + char *name =3D ck_constraint->def->name; > + sqlHaltConstraint(parse_context, > + SQL_CONSTRAINT_CHECK, > + on_conflict_check, name, > + P4_TRANSIENT, = P5_ConstraintCheck); > } > + sqlVdbeResolveLabel(v, all_ok); >=20 > @@ -1231,14 +1226,12 @@ 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 destination > + * table contains any check constraints. > + */ > + if (!rlist_empty(&dest->ck_constraint)) > return 0; Why did you change condition of optimisation? > diff --git a/src/box/sql/select.c b/src/box/sql/select.c > index 5195656af..6c031c2d8 100644 > --- a/src/box/sql/select.c > +++ b/src/box/sql/select.c > @@ -6422,7 +6422,5 @@ 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); > + parser->parsed_ast.expr =3D sqlExprDup(parser->db, = expr_list->a->pExpr, 0); Is this change related to patch-set? > } > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 8967ea3e0..3c58ac649 100644 >=20 > /** > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c > index ed7bf8870..cbccec95e 100644 > --- a/src/box/sql/vdbe.c > +++ b/src/box/sql/vdbe.c > @@ -1028,7 +1028,7 @@ case OP_HaltIfNull: { /* in3 */ > * 0: (no change) > * 1: NOT NULL contraint failed: P4 > * 2: UNIQUE constraint failed: P4 > - * 3: CHECK constraint failed: P4 > + * 3: Check constraint failed: P4 > * 4: FOREIGN KEY constraint failed: P4 This change looks inconsistent with other other names. > * > * If P5 is not zero and P4 is NULL, then everything after the > @@ -1077,8 +1077,8 @@ case OP_Halt: { > pOp->p4.z); > } > } else if (pOp->p5 !=3D 0) { > - static const char * const azType[] =3D { "NOT = NULL", "UNIQUE", "CHECK", > - "FOREIGN = KEY" }; > + static const char * const azType[] =3D > + {"NOT NULL", "UNIQUE", "Check", "FOREIGN = KEY" }; Do we really need this change? > diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua > index 0582bbb63..9a7e5faf4 100644 > --- a/test/sql/checks.test.lua > +++ b/test/sql/checks.test.lua > @@ -8,41 +8,42 @@ box.sql.execute('pragma = sql_default_engine=3D\''..engine..'\'') > -- gh-3272: Move SQL CHECK into server > -- >=20 > --- invalid expression > -opts =3D {checks =3D {{expr =3D 'X><5'}}} > -format =3D {{name =3D 'X', type =3D 'unsigned'}} > -t =3D {513, 1, 'test', 'memtx', 0, opts, format} > -s =3D box.space._space:insert(t) > - > +-- Legacy data in _space (insertion on bootrap) test. Nit: _bootstrap. Anyway, re-phrase comment pls: =E2=80=9CUntil Tarantool version 2.2 check constraints were stored in = space opts. Make sure that now this legacy option is ignored." > +box.space.test:create_index('pk') > + > +-- 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'}) > +-- Field type test. Typo: Failed. -> Pass integer instead of expression. > +box.space._ck_constraint:insert({'CK_CONSTRAINT_01', 550, 666}) > + > +-- Check constraints LUA creation test. > +box.space._ck_constraint:insert({'CK_CONSTRAINT_01', 513, 'X<5'}) > +box.space._ck_constraint:count({}) > + > +box.sql.execute("INSERT INTO \"test\" VALUES(5);") > +box.space._ck_constraint:replace({'CK_CONSTRAINT_01', 513, 'X<=3D5'}) > +box.sql.execute("INSERT INTO \"test\" VALUES(5);") > +box.sql.execute("INSERT INTO \"test\" VALUES(6);") > +-- Can't drop table with check constraints. > +box.space.test:delete({5}) > +box.space.test.index.pk:drop() > +box.space._space:delete({513}) > +box.space._ck_constraint:delete({'CK_CONSTRAINT_01', 513}) > +box.space.test:drop() But space:drop() drops constraints before space itself. > diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua > index d8833feb4..f5e78664c 100644 > --- a/test/sql/errinj.test.lua > +++ b/test/sql/errinj.test.lua > @@ -139,3 +139,23 @@ box.sql.execute("INSERT INTO t VALUES (2);") > box.sql.execute("UPDATE t SET id =3D 2;") > -- Finish drop space. > errinj.set("ERRINJ_WAL_DELAY", false) > + > +-- > +-- Tests which are aimed at verifying work of commit/rollback > +-- triggers on _ck_constraint space. > +-- > +errinj =3D box.error.injection > +s =3D box.schema.space.create('test', {format =3D {{name =3D 'X', = type =3D 'unsigned'}}}) > +pk =3D box.space.test:create_index('pk') > + > +errinj.set("ERRINJ_WAL_IO", true) > +_ =3D box.space._ck_constraint:insert({'CK_CONSTRAINT_01', s.id, = 'X<5'}) > +errinj.set("ERRINJ_WAL_IO", false) > +_ =3D box.space._ck_constraint:insert({'CK_CONSTRAINT_01', s.id, = 'X<5'}) > +box.sql.execute("INSERT INTO \"test\" VALUES(5);") > +errinj.set("ERRINJ_WAL_IO", true) > +_ =3D box.space._ck_constraint:replace({'CK_CONSTRAINT_01', s.id, = 'X<=3D5'}) > +errinj.set("ERRINJ_WAL_IO", false) > +_ =3D box.space._ck_constraint:replace({'CK_CONSTRAINT_01', s.id, = 'X<=3D5'}) > +box.sql.execute("INSERT INTO \"test\" VALUES(5);=E2=80=9D) You forgot to test drop rollback trigger.