[tarantool-patches] Re: [PATCH v2 5/9] schema: add new system space for CHECK constraints

["n.pettik" <korablev@tarantool.org>]

>>> +	++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.