[tarantool-patches] Re: [PATCH v2 5/9] schema: add new system space for CHECK constraints
n.pettik
korablev at tarantool.org
Mon Apr 1 22:45:42 MSK 2019
>>> + ++schema_version;
>>
>> 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.
>
See no fixes to this comment.
I’ve found way to make Tarantool hang:
box.sql.execute("create table t(id int primary key check (id > 5))")
drop_ck = function() box.space._ck_constraint:delete({'CK_CONSTRAINT_1_T', box.space.T.id}) end
fiber = require('fiber')
box.error.injection.set("ERRINJ_WAL_DELAY", true)
f2 = 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.
> ============================================================
>
> This patch introduces new system space to persist check
> constraints. Format of the space:
>
> _ck_constraint (space id = 357)
> [<constraint name> STR, <space id> UINT, <expression string>STR]
>
> CK constraint is local to space, so every pair <CK name, space id>
> is unique (and it is PK in _ck_constraint space).
>
> 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
>
> @@ -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 = 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 = 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)
> }
> }
>
> +/**
> + * 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 =
> + 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 =
> + tuple_field_str_xc(tuple, BOX_CK_CONSTRAINT_FIELD_EXPR_STR,
> + &expr_str_len);
> + uint32_t name_offset, expr_str_offset;
> + uint32_t sz = ck_constraint_def_sizeof(name_len, expr_str_len,
> + &name_offset, &expr_str_offset);
> + struct ck_constraint_def *ck_constraint_def =
> + (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’t 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 = txn_last_stmt((struct txn*) event);
> + struct ck_constraint *ck_constraint =
> + (struct ck_constraint *)trigger->data;
> + struct space *space = NULL;
> + if (ck_constraint != NULL)
> + space = space_by_id(ck_constraint->space_id);
> + if (stmt->old_tuple != NULL && stmt->new_tuple == NULL) {
> + /* Rollback DELETE check constraint. */
> + if (ck_constraint == NULL)
> + return;
But in on_replace_dd_ck_constraint() you have next check:
assert(old_ck_constraint != NULL);
…
on_rollback->data = old_ck_constraint;
How ck_constraint could be NULL?
> + assert(space != NULL);
> + rlist_add_entry(&space->ck_constraint, ck_constraint, link);
> + } else if (stmt->new_tuple != NULL && stmt->old_tuple == NULL) {
> + /* Rollback INSERT check constraint. */
> + assert(space != NULL);
> + rlist_del_entry(ck_constraint, link);
> + ck_constraint_delete(ck_constraint);
> + } else {
> + /* Rollback REPLACE check constraint. */
> + assert(space != NULL);
> + const char *space_name = ck_constraint->def->name;
It’s constraint’s name.
> + struct ck_constraint *new_ck_constraint =
> + space_ck_constraint_by_name(space, space_name,
> + strlen(space_name));
> + assert(new_ck_constraint != NULL);
> + rlist_del_entry(new_ck_constraint, link);
> + rlist_add_entry(&space->ck_constraint, ck_constraint, link);
> + ck_constraint_delete(new_ck_constraint);
> + }
> +}
>
> +/** 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 = (struct txn *) event;
> + txn_check_singlestatement_xc(txn, "Space _ck_constraint");
> + struct txn_stmt *stmt = txn_current_stmt(txn);
> + struct tuple *old_tuple = stmt->old_tuple;
> + struct tuple *new_tuple = stmt->new_tuple;
> + uint32_t space_id =
> + tuple_field_u32_xc(old_tuple != NULL ? old_tuple : new_tuple,
> + BOX_CK_CONSTRAINT_FIELD_SPACE_ID);
> + struct space *space = 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 =
> + txn_alter_trigger_new(on_replace_ck_constraint_rollback, NULL);
> + struct trigger *on_commit =
> + txn_alter_trigger_new(on_replace_ck_constraint_commit, NULL);
> +
> + if (new_tuple != NULL) {
> + /* Create or replace check constraint. */
> + struct ck_constraint_def *ck_constraint_def =
> + ck_constraint_def_decode(new_tuple, &fiber()->gc);
> + struct ck_constraint *new_ck_constraint =
> + ck_constraint_new(ck_constraint_def, space->def);
> + if (new_ck_constraint == NULL)
> + diag_raise();
> + const char *space_name = new_ck_constraint->def->name;
But this is not space name, it’s name of constraint.
> + struct ck_constraint *old_ck_constraint =
> + space_ck_constraint_by_name(space, space_name,
> + strlen(space_name));
> + if (old_ck_constraint != NULL)
> + rlist_del_entry(old_ck_constraint, link);
> + rlist_add_entry(&space->ck_constraint, new_ck_constraint, link);
> + on_commit->data = old_ck_constraint;
> + on_rollback->data = old_tuple == NULL ? new_ck_constraint :
> + old_ck_constraint;
>
> 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 = sizeof(struct ck_constraint_def);
> + *expr_str_offset = *name_offset + (name_len != 0 ? name_len + 1 : 0);
Why name_len can be 0? Isn’t name is mandatory field?
The same question about expr_str_len.
> + return *expr_str_offset + (expr_str_len != 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 = (char *)ck_constraint_def + name_offset;
> + sprintf(ck_constraint_def->name, "%.*s", name_len, name);
Why don’t you use memcpy?
> + ck_constraint_def->expr_str =
> + (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 = strlen(ck_constraint_def->name);
> + uint32_t expr_str_len = strlen(ck_constraint_def->expr_str);
> + uint32_t name_offset, expr_str_offset;
> + uint32_t ck_constraint_def_sz =
> + ck_constraint_def_sizeof(ck_constraint_name_len, expr_str_len,
> + &name_offset, &expr_str_offset);
> + uint32_t ck_constraint_sz = sizeof(struct ck_constraint) +
> + ck_constraint_def_sz;
> + struct ck_constraint *ck_constraint = calloc(1, ck_constraint_sz);
> + if (ck_constraint == NULL) {
> + diag_set(OutOfMemory, ck_constraint_sz, "malloc",
> + "ck_constraint");
> + return NULL;
> + }
> + rlist_create(&ck_constraint->link);
> + ck_constraint->space_id = space_def->id;
> + ck_constraint->def =
> + (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 =
> + sql_expr_compile(sql_get(), ck_constraint_def->expr_str,
> + expr_str_len);
> + if (expr == NULL ||
> + ck_constraint_resolve_ref(expr, space_def) != 0) {
> + diag_set(ClientError, ER_CREATE_CK_CONSTRAINT,
> + ck_constraint->def->name,
> + box_error_message(box_error_last()));
> + goto error;
> + }
> + ck_constraint->expr = expr;
> +
> + return ck_constraint;
> +error:
> + ck_constraint_delete(ck_constraint);
> + return NULL;
> +}
>
> 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
>
> +/**
> + * 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’t 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”
it’s 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
>
> 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.
>
> 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:
> }
>
> 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 = NULL;
> + if (parser->constraintName.n != 0) {
> + ck_constraint_name =
> + region_alloc(region, parser->constraintName.n + 1);
> + if (ck_constraint_name == NULL) {
> + diag_set(OutOfMemory, parser->constraintName.n + 1,
> + "region_alloc", "ck_constraint_name");
> + parser->is_aborted = 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 = parser->db;
> + struct Vdbe *v = sqlGetVdbe(parser);
> + assert(v != NULL);
> + int ck_constraint_reg = 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 = sqlGetVdbe(parser);
> + struct sql *db = v->db;
> + assert(v != NULL);
> + int key_reg = 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 =
> + 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) != 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 -
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 == ON_CONFLICT_ACTION_DEFAULT)
> on_conflict = ON_CONFLICT_ACTION_ABORT;
> /* Test all CHECK constraints. */
> - struct ExprList *checks = def->opts.checks;
> enum on_conflict_action on_conflict_check = on_conflict;
> if (on_conflict == ON_CONFLICT_ACTION_REPLACE)
> on_conflict_check = ON_CONFLICT_ACTION_ABORT;
> - if (checks != NULL) {
> + if (!rlist_empty(&space->ck_constraint))
> parse_context->ckBase = new_tuple_reg;
> + struct ck_constraint *ck_constraint;
> + rlist_foreach_entry(ck_constraint, &space->ck_constraint, link) {
> + struct Expr *expr = ck_constraint->expr;
> + if (is_update && checkConstraintUnchanged(expr, upd_cols) != 0)
> + continue;
> + int all_ok = sqlVdbeMakeLabel(v);
> + sqlExprIfTrue(parse_context, expr, all_ok, SQL_JUMPIFNULL);
> + if (on_conflict == 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 = ck_constraint->def->name;
> + sqlHaltConstraint(parse_context,
> + SQL_CONSTRAINT_CHECK,
> + on_conflict_check, name,
> + P4_TRANSIENT, P5_ConstraintCheck);
> }
> + sqlVdbeResolveLabel(v, all_ok);
>
> @@ -1231,14 +1226,12 @@ xferOptimization(Parse * pParse, /* Parser context */
> if (pSrcIdx == NULL)
> return 0;
> }
> - /* Get server checks. */
> - ExprList *pCheck_src = src->def->opts.checks;
> - ExprList *pCheck_dest = dest->def->opts.checks;
> - if (pCheck_dest != NULL &&
> - sqlExprListCompare(pCheck_src, pCheck_dest, -1) != 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 = select->pEList;
> assert(expr_list->nExpr == 1);
> parser->parsed_ast_type = AST_TYPE_EXPR;
> - parser->parsed_ast.expr = sqlExprDup(parser->db,
> - expr_list->a->pExpr,
> - EXPRDUP_REDUCE);
> + parser->parsed_ast.expr = 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
>
> /**
> 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 != 0) {
> - static const char * const azType[] = { "NOT NULL", "UNIQUE", "CHECK",
> - "FOREIGN KEY" };
> + static const char * const azType[] =
> + {"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=\''..engine..'\'')
> -- gh-3272: Move SQL CHECK into server
> --
>
> --- invalid expression
> -opts = {checks = {{expr = 'X><5'}}}
> -format = {{name = 'X', type = 'unsigned'}}
> -t = {513, 1, 'test', 'memtx', 0, opts, format}
> -s = box.space._space:insert(t)
> -
> +-- Legacy data in _space (insertion on bootrap) test.
Nit: _bootstrap.
Anyway, re-phrase comment pls:
“Until 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<=5'})
> +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 = 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 = box.error.injection
> +s = box.schema.space.create('test', {format = {{name = 'X', type = 'unsigned'}}})
> +pk = box.space.test:create_index('pk')
> +
> +errinj.set("ERRINJ_WAL_IO", true)
> +_ = box.space._ck_constraint:insert({'CK_CONSTRAINT_01', s.id, 'X<5'})
> +errinj.set("ERRINJ_WAL_IO", false)
> +_ = 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)
> +_ = box.space._ck_constraint:replace({'CK_CONSTRAINT_01', s.id, 'X<=5'})
> +errinj.set("ERRINJ_WAL_IO", false)
> +_ = box.space._ck_constraint:replace({'CK_CONSTRAINT_01', s.id, 'X<=5'})
> +box.sql.execute("INSERT INTO \"test\" VALUES(5);”)
You forgot to test drop rollback trigger.
More information about the Tarantool-patches
mailing list