Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 1/4] box: add an ability to disable CK constraints

Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 1/4] box: add an ability to disable CK constraints

[Nikita Pettik]

On 08 Oct 16:02, Kirill Shcherbatov wrote:
> Now it is possible to disable and enable ck constraints in LUA.
> This option is persistent. All ck constraints are constructed
> in enabled state when Tarantool is configured. This ability
> may be useful when all information processed is (by some reason)

-> when processed data is verified and constraints validation
is not required. For instance, during casual recovery process there's
no need to provide any checks since data is assumed to be consistent.

Also explain why we have to store this option.

> trusted and ck constraints validations are not required.
> 
> Part of #4244
> ---
>  src/box/alter.cc             |  31 ++++++--
>  src/box/bootstrap.snap       | Bin 5934 -> 5944 bytes
>  src/box/ck_constraint.c      |   7 +-
>  src/box/ck_constraint.h      |   8 +-
>  src/box/lua/schema.lua       |  16 +++-
>  src/box/lua/space.cc         |   3 +
>  src/box/lua/upgrade.lua      |  13 ++++
>  src/box/schema_def.h         |   1 +
>  src/box/sql/build.c          |  11 +--
>  test/box-py/bootstrap.result |   3 +-
>  test/box/access_misc.result  | 137 ++++++++++++++++++-----------------
>  test/sql/checks.result       | 108 ++++++++++++++++++++++-----
>  test/sql/checks.test.lua     |  57 ++++++++++-----
>  test/sql/errinj.result       |  12 +--
>  test/sql/errinj.test.lua     |  12 +--
>  15 files changed, 286 insertions(+), 133 deletions(-)

To be honest, I doubt that 'is_enable' option should be persisted..
At least, we can assume that it is always 'on' by default, so that
save user from having to specify this option each time.

> diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
> index 4c9aea7f50f8ac86be32ca9f126fea9a3d2d182f..59f6cc16bb48841fa99b4e13590b8e0677433b35 100644
> GIT binary patch
> delta 4861
> 
> diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
> index 1cde27022..fafa7be12 100644
> --- a/src/box/ck_constraint.c
> +++ b/src/box/ck_constraint.c
> @@ -223,6 +225,7 @@ ck_constraint_new(struct ck_constraint_def *ck_constraint_def,
>  	}
>  	ck_constraint->def = NULL;
>  	ck_constraint->stmt = NULL;
> +

Extra diff.

Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 2/4] sql: add an ability to disable CK constraints

[Nikita Pettik]

On 08 Oct 16:02, Kirill Shcherbatov wrote:
> Closes #4244
> 
> @TarantoolBot document
> Title: an ability to disable CK constraints
> 
> Now it is possible to disable and enable ck constraints.
> This option is not persistent.

Outdated comment.

> All ck constraints are enabled
> by default when Tarantool is configured. Ck constraints checks
> are not performed during standard recovery, but performed during
> force_recovery - all conflicting tuples are skipped in case of
> ck_constraint conflict.
> 
> To change CK constraint "is_enabled" state, call
> -- in LUA
> ck_obj:enable(new_state in {true, false})
> -- in SQL
> ALTER TABLE {TABLE_NAME} {EN, DIS}ABLE CHECK CONSTRAINT {CK_NAME};
> 
> Example:
> box.space.T6.ck_constraint.ck_unnamed_T6_1:enable(false)
> box.space.T6.ck_constraint.ck_unnamed_T6_1
> - space_id: 512
>   is_enabled: false
>   name: ck_unnamed_T6_1
>   expr: a < 10
> box.space.T6:insert({11})
> -- passed
> box.execute("ALTER TABLE t6 ENABLE CHECK CONSTRAINT \"ck_unnamed_T6_1\"")
> box.space.T6:insert({12})
> - error: 'Check constraint failed ''ck_unnamed_T6_1'': a < 10'
> ---
> diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
> index 765600186..500cabc5f 100644
> --- a/src/box/sql/alter.c
> +++ b/src/box/sql/alter.c
> @@ -79,6 +79,71 @@ tnt_error:
>  	goto exit_rename_table;
>  }
>  
> +void
> +sql_alter_ck_constraint_enable(struct Parse *parse)
> +{
> +	struct enable_entity_def *enable_def = &parse->enable_entity_def;
> +	struct SrcList *src_tab = enable_def->base.entity_name;
> +	assert(enable_def->base.entity_type == ENTITY_TYPE_CK);
> +	assert(enable_def->base.alter_action == ALTER_ACTION_ENABLE);
> +	assert(src_tab->nSrc == 1);
> +	struct sql *db = parse->db;
> +
> +	char *constraint_name = NULL;
> +	const char *tbl_name = src_tab->a[0].zName;
> +	struct space *space = space_by_name(tbl_name);
> +	if (space == NULL) {
> +		diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name);
> +		parse->is_aborted = true;
> +		goto exit_alter_ck_constraint;
> +	}
> +
> +	constraint_name = sql_name_from_token(db, &enable_def->name);
> +	if (constraint_name == NULL) {
> +		parse->is_aborted = true;
> +		goto exit_alter_ck_constraint;
> +	}
> +
> +	struct Vdbe *v = sqlGetVdbe(parse);
> +	if (v == NULL)
> +		goto exit_alter_ck_constraint;
> +
> +	struct space *ck_space = space_by_id(BOX_CK_CONSTRAINT_ID);
> +	assert(ck_space != NULL);
> +	int cursor = parse->nTab++;
> +	vdbe_emit_open_cursor(parse, cursor, 0, ck_space);
> +	sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
> +
> +	int key_reg = sqlGetTempRange(parse, 2);
> +	sqlVdbeAddOp2(v, OP_Integer, space->def->id, key_reg);
> +	sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
> +		      sqlDbStrDup(db, constraint_name), P4_DYNAMIC);
> +	int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 2);
> +	sqlVdbeAddOp4(v, OP_SetDiag, ER_NO_SUCH_CONSTRAINT, 0, 0,
> +		      sqlMPrintf(db, tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
> +				 constraint_name), P4_DYNAMIC);
> +	sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
> +	sqlVdbeJumpHere(v, addr);
> +
> +	const int field_count = 6;
> +	int tuple_reg = sqlGetTempRange(parse, field_count + 1);
> +	for (int i = 0; i < field_count - 1; ++i)
> +		sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i);
> +	sqlVdbeAddOp1(v, OP_Close, cursor);
> +	sqlVdbeAddOp2(v, OP_Bool, enable_def->is_enabled,
> +		      tuple_reg + field_count - 1);
> +	sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, field_count,
> +		      tuple_reg + field_count);
> +	sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + field_count, 0, 0,
> +		      (char *)ck_space, P4_SPACEPTR);
> +
> +
> +

Too many new lines.

> +exit_alter_ck_constraint:
> +	sqlDbFree(db, constraint_name);
> +	sqlSrcListDelete(db, src_tab);
> +}
> +
>  /* This function is used to implement the ALTER TABLE command.
>   * The table name in the CREATE TRIGGER statement is replaced with the third
>   * argument and the result returned. This is analagous to rename_table()

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 2/4] sql: add an ability to disable CK constraints

[Kirill Shcherbatov]

>> Now it is possible to disable and enable ck constraints.
>> This option is not persistent.
> 
> Outdated comment.

Fixed.

> Too many new lines.

Fixed.

========================================================

Closes #4244

@TarantoolBot document
Title: an ability to disable CK constraints

Now it is possible to disable and enable ck constraints.
All ck constraints are enabled by default when Tarantool is
configured. Ck constraints checks are not performed during
standard recovery, but performed during force_recovery -
all conflicting tuples are skipped in case of ck_constraint
conflict.

To change CK constraint "is_enabled" state, call
-- in LUA
ck_obj:enable(new_state in {true, false})
-- in SQL
ALTER TABLE {TABLE_NAME} {EN, DIS}ABLE CHECK CONSTRAINT {CK_NAME};

Example:
box.space.T6.ck_constraint.ck_unnamed_T6_1:enable(false)
box.space.T6.ck_constraint.ck_unnamed_T6_1
- space_id: 512
  is_enabled: false
  name: ck_unnamed_T6_1
  expr: a < 10
box.space.T6:insert({11})
-- passed
box.execute("ALTER TABLE t6 ENABLE CHECK CONSTRAINT \"ck_unnamed_T6_1\"")
box.space.T6:insert({12})
- error: 'Check constraint failed ''ck_unnamed_T6_1'': a < 10'
---
 extra/mkkeywordhash.c          |  2 ++
 src/box/sql/alter.c            | 62 ++++++++++++++++++++++++++++++++++
 src/box/sql/parse.y            | 12 ++++++-
 src/box/sql/parse_def.h        | 22 +++++++++++-
 src/box/sql/sqlInt.h           | 10 ++++++
 test/sql-tap/keyword1.test.lua |  4 ++-
 test/sql/checks.result         | 49 +++++++++++++++++++++++++++
 test/sql/checks.test.lua       | 15 ++++++++
 8 files changed, 173 insertions(+), 3 deletions(-)

diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c
index 2a59d6a61..3c3de4cca 100644
--- a/extra/mkkeywordhash.c
+++ b/extra/mkkeywordhash.c
@@ -89,6 +89,7 @@ static Keyword aKeywordTable[] = {
   { "DEFERRED",               "TK_DEFERRED",    false },
   { "DEFERRABLE",             "TK_DEFERRABLE",  false },
   { "DELETE",                 "TK_DELETE",      true  },
+  { "DISABLE",                "TK_DISABLE",     false },
   { "DESC",                   "TK_DESC",        true  },
   { "DISTINCT",               "TK_DISTINCT",    true  },
   { "DROP",                   "TK_DROP",        true  },
@@ -203,6 +204,7 @@ static Keyword aKeywordTable[] = {
   { "DETERMINISTIC",          "TK_STANDARD",    true  },
   { "DOUBLE",                 "TK_STANDARD",    true  },
   { "ELSEIF",                 "TK_STANDARD",    true  },
+  { "ENABLE",                 "TK_ENABLE",      false },
   { "FETCH",                  "TK_STANDARD",    true  },
   { "FLOAT",                  "TK_STANDARD",    true  },
   { "FUNCTION",               "TK_STANDARD",    true  },
diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c
index 765600186..973b420cf 100644
--- a/src/box/sql/alter.c
+++ b/src/box/sql/alter.c
@@ -79,6 +79,68 @@ tnt_error:
 	goto exit_rename_table;
 }
 
+void
+sql_alter_ck_constraint_enable(struct Parse *parse)
+{
+	struct enable_entity_def *enable_def = &parse->enable_entity_def;
+	struct SrcList *src_tab = enable_def->base.entity_name;
+	assert(enable_def->base.entity_type == ENTITY_TYPE_CK);
+	assert(enable_def->base.alter_action == ALTER_ACTION_ENABLE);
+	assert(src_tab->nSrc == 1);
+	struct sql *db = parse->db;
+
+	char *constraint_name = NULL;
+	const char *tbl_name = src_tab->a[0].zName;
+	struct space *space = space_by_name(tbl_name);
+	if (space == NULL) {
+		diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name);
+		parse->is_aborted = true;
+		goto exit_alter_ck_constraint;
+	}
+
+	constraint_name = sql_name_from_token(db, &enable_def->name);
+	if (constraint_name == NULL) {
+		parse->is_aborted = true;
+		goto exit_alter_ck_constraint;
+	}
+
+	struct Vdbe *v = sqlGetVdbe(parse);
+	if (v == NULL)
+		goto exit_alter_ck_constraint;
+
+	struct space *ck_space = space_by_id(BOX_CK_CONSTRAINT_ID);
+	assert(ck_space != NULL);
+	int cursor = parse->nTab++;
+	vdbe_emit_open_cursor(parse, cursor, 0, ck_space);
+	sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP);
+
+	int key_reg = sqlGetTempRange(parse, 2);
+	sqlVdbeAddOp2(v, OP_Integer, space->def->id, key_reg);
+	sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0,
+		      sqlDbStrDup(db, constraint_name), P4_DYNAMIC);
+	int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 2);
+	sqlVdbeAddOp4(v, OP_SetDiag, ER_NO_SUCH_CONSTRAINT, 0, 0,
+		      sqlMPrintf(db, tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT),
+				 constraint_name), P4_DYNAMIC);
+	sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT);
+	sqlVdbeJumpHere(v, addr);
+
+	const int field_count = 6;
+	int tuple_reg = sqlGetTempRange(parse, field_count + 1);
+	for (int i = 0; i < field_count - 1; ++i)
+		sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i);
+	sqlVdbeAddOp1(v, OP_Close, cursor);
+	sqlVdbeAddOp2(v, OP_Bool, enable_def->is_enabled,
+		      tuple_reg + field_count - 1);
+	sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, field_count,
+		      tuple_reg + field_count);
+	sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + field_count, 0, 0,
+		      (char *)ck_space, P4_SPACEPTR);
+exit_alter_ck_constraint:
+	sqlDbFree(db, constraint_name);
+	sqlSrcListDelete(db, src_tab);
+}
+
 /* This function is used to implement the ALTER TABLE command.
  * The table name in the CREATE TRIGGER statement is replaced with the third
  * argument and the result returned. This is analagous to rename_table()
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ed59a875a..1d0c95fac 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -260,7 +260,7 @@ columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);}
   CONFLICT DEFERRED END ENGINE FAIL
   IGNORE INITIALLY INSTEAD NO MATCH PLAN
   QUERY KEY OFFSET RAISE RELEASE REPLACE RESTRICT
-  RENAME CTIME_KW IF
+  RENAME CTIME_KW IF ENABLE DISABLE
   .
 %wildcard ANY.
 
@@ -798,6 +798,10 @@ col_list_with_autoinc(A) ::= expr(Y) autoinc(I). {
   A = sql_expr_list_append(pParse->db, NULL, Y.pExpr);
 }
 
+%type enable {bool}
+enable(A) ::= ENABLE.           {A = true;}
+enable(A) ::= DISABLE.          {A = false;}
+
 %type sortorder {int}
 
 sortorder(A) ::= ASC.           {A = SORT_ORDER_ASC;}
@@ -1785,6 +1789,12 @@ cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). {
   sql_drop_foreign_key(pParse);
 }
 
+cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). {
+    enable_entity_def_init(&pParse->enable_entity_def, ENTITY_TYPE_CK, A,
+                           &Z, E);
+    sql_alter_ck_constraint_enable(pParse);
+}
+
 //////////////////////// COMMON TABLE EXPRESSIONS ////////////////////////////
 %type with {With*}
 %type wqlist {With*}
diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h
index df1238b9e..2f433e4c0 100644
--- a/src/box/sql/parse_def.h
+++ b/src/box/sql/parse_def.h
@@ -169,7 +169,8 @@ enum entity_type {
 enum alter_action {
 	ALTER_ACTION_CREATE = 0,
 	ALTER_ACTION_DROP,
-	ALTER_ACTION_RENAME
+	ALTER_ACTION_RENAME,
+	ALTER_ACTION_ENABLE,
 };
 
 struct alter_entity_def {
@@ -181,6 +182,14 @@ struct alter_entity_def {
 	struct SrcList *entity_name;
 };
 
+struct enable_entity_def {
+	struct alter_entity_def base;
+	/** Name of constraint to be enabled/disabled. */
+	struct Token name;
+	/** A new state to be set for entity found. */
+	bool is_enabled;
+};
+
 struct rename_entity_def {
 	struct alter_entity_def base;
 	struct Token new_name;
@@ -327,6 +336,17 @@ rename_entity_def_init(struct rename_entity_def *rename_def,
 	rename_def->new_name = *new_name;
 }
 
+static inline void
+enable_entity_def_init(struct enable_entity_def *enable_def,
+		       enum entity_type type, struct SrcList *parent_name,
+		       struct Token *name, bool is_enabled)
+{
+	alter_entity_def_init(&enable_def->base, parent_name, type,
+			      ALTER_ACTION_ENABLE);
+	enable_def->name = *name;
+	enable_def->is_enabled = is_enabled;
+}
+
 static inline void
 create_entity_def_init(struct create_entity_def *create_def,
 		       enum entity_type type, struct SrcList *parent_name,
diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h
index 35fc81dfb..bcc099fa9 100644
--- a/src/box/sql/sqlInt.h
+++ b/src/box/sql/sqlInt.h
@@ -2202,6 +2202,7 @@ struct Parse {
 		struct drop_table_def drop_table_def;
 		struct drop_trigger_def drop_trigger_def;
 		struct drop_view_def drop_view_def;
+		struct enable_entity_def enable_entity_def;
 	};
 	/**
 	 * Table def is not part of union since information
@@ -3971,6 +3972,15 @@ extern int sqlPendingByte;
 void
 sql_alter_table_rename(struct Parse *parse);
 
+/**
+ * Generate code to implement the "ALTER TABLE xxx ENABLE/DISABLE
+ * CHECK CONSTRAINT" command.
+ *
+ * @param parse Current parsing context.
+ */
+void
+sql_alter_ck_constraint_enable(struct Parse *parse);
+
 /**
  * Return the length (in bytes) of the token that begins at z[0].
  * Store the token type in *type before returning.
diff --git a/test/sql-tap/keyword1.test.lua b/test/sql-tap/keyword1.test.lua
index 03b1054d2..571e1dcbe 100755
--- a/test/sql-tap/keyword1.test.lua
+++ b/test/sql-tap/keyword1.test.lua
@@ -1,6 +1,6 @@
 #!/usr/bin/env tarantool
 test = require("sqltester")
-test:plan(180)
+test:plan(184)
 
 --!./tcltestrunner.lua
 -- 2009 January 29
@@ -44,6 +44,8 @@ local kwlist = {
 	"query",
 	"restrict",
 	"raise",
+	"enable",
+	"disable"
 }
 
 local bannedkws = { 
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 72b1fa26f..6d584c301 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -790,6 +790,55 @@ box.space.TEST:insert({13})
 box.space.TEST:drop()
 ---
 ...
+--
+-- Test ENABLE/DISABLE CK constraints from SQL works.
+--
+box.execute("ALTER TABLE falsch DISABLE CHECK CONSTRAINT \"some_ck\"")
+---
+- null
+- Space 'FALSCH' does not exist
+...
+box.execute("CREATE TABLE test(a INT PRIMARY KEY);");
+---
+- row_count: 1
+...
+box.execute('ALTER TABLE test ADD CONSTRAINT \"some_ck\" CHECK(a < 10);')
+---
+- row_count: 1
+...
+box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"falsch\"")
+---
+- null
+- Constraint falsch does not exist
+...
+box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"some_ck\"")
+---
+- row_count: 0
+...
+assert(box.space.TEST.ck_constraint.some_ck.is_enabled == false)
+---
+- true
+...
+box.space.TEST:insert({11})
+---
+- [11]
+...
+box.execute("ALTER TABLE test ENABLE CHECK CONSTRAINT \"some_ck\"")
+---
+- row_count: 0
+...
+assert(box.space.TEST.ck_constraint.some_ck.is_enabled == true)
+---
+- true
+...
+box.space.TEST:insert({12})
+---
+- error: 'Check constraint failed ''some_ck'': a < 10'
+...
+box.execute("DROP TABLE test;")
+---
+- row_count: 1
+...
 test_run:cmd("clear filter")
 ---
 - true
diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
index 11918dced..f8dc5d363 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -257,4 +257,19 @@ assert(box.space.TEST.ck_constraint.CK.is_enabled == true)
 box.space.TEST:insert({13})
 box.space.TEST:drop()
 
+--
+-- Test ENABLE/DISABLE CK constraints from SQL works.
+--
+box.execute("ALTER TABLE falsch DISABLE CHECK CONSTRAINT \"some_ck\"")
+box.execute("CREATE TABLE test(a INT PRIMARY KEY);");
+box.execute('ALTER TABLE test ADD CONSTRAINT \"some_ck\" CHECK(a < 10);')
+box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"falsch\"")
+box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"some_ck\"")
+assert(box.space.TEST.ck_constraint.some_ck.is_enabled == false)
+box.space.TEST:insert({11})
+box.execute("ALTER TABLE test ENABLE CHECK CONSTRAINT \"some_ck\"")
+assert(box.space.TEST.ck_constraint.some_ck.is_enabled == true)
+box.space.TEST:insert({12})
+box.execute("DROP TABLE test;")
+
 test_run:cmd("clear filter")
-- 
2.23.0

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints

[Kirill Shcherbatov]

> -> when processed data is verified and constraints validation
> is not required. For instance, during casual recovery process there's
> no need to provide any checks since data is assumed to be consistent.
Applied.

> Also explain why we have to store this option.
> To be honest, I doubt that 'is_enable' option should be persisted..
> At least, we can assume that it is always 'on' by default, so that
> save user from having to specify this option each time.

Verbally discussed. To implement a behavior that Oracle users are familiar with.
It was a Kostya's point of view and it is already implemented.

> Extra diff.
Fixed.

=====================================================

Now it is possible to disable and enable ck constraints in LUA.
This option is persistent. All ck constraints are constructed
in enabled state when Tarantool is configured. This ability
may be usefulwhen processed data is verified and constraints
validation is not required. For instance, during casual recovery
process there's no need to provide any checks since data is
assumed to be consistent.

Part of #4244
---
 src/box/alter.cc             |  31 ++++++--
 src/box/bootstrap.snap       | Bin 5934 -> 5944 bytes
 src/box/ck_constraint.c      |   6 +-
 src/box/ck_constraint.h      |   8 +-
 src/box/lua/schema.lua       |  16 +++-
 src/box/lua/space.cc         |   3 +
 src/box/lua/upgrade.lua      |  13 ++++
 src/box/schema_def.h         |   1 +
 src/box/sql/build.c          |  11 +--
 test/box-py/bootstrap.result |   3 +-
 test/box/access_misc.result  | 137 ++++++++++++++++++-----------------
 test/sql/checks.result       | 108 ++++++++++++++++++++++-----
 test/sql/checks.test.lua     |  57 ++++++++++-----
 test/sql/errinj.result       |  12 +--
 test/sql/errinj.test.lua     |  12 +--
 15 files changed, 285 insertions(+), 133 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index bb3686d6e..f8f1802d0 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -4595,9 +4595,12 @@ ck_constraint_def_new_from_tuple(struct tuple *tuple)
 	const char *expr_str =
 		tuple_field_str_xc(tuple, BOX_CK_CONSTRAINT_FIELD_CODE,
 				   &expr_str_len);
+	bool is_enabled =
+		tuple_field_count(tuple) <= BOX_CK_CONSTRAINT_FIELD_IS_ENABLED ||
+		tuple_field_bool_xc(tuple, BOX_CK_CONSTRAINT_FIELD_IS_ENABLED);
 	struct ck_constraint_def *ck_def =
 		ck_constraint_def_new(name, name_len, expr_str, expr_str_len,
-				      space_id, language);
+				      space_id, language, is_enabled);
 	if (ck_def == NULL)
 		diag_raise();
 	return ck_def;
@@ -4697,6 +4700,26 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 		auto ck_def_guard = make_scoped_guard([=] {
 			ck_constraint_def_delete(ck_def);
 		});
+		/*
+		 * A corner case: enabling/disabling an existent
+		 * ck constraint doesn't require the object
+		 * rebuilding.
+		 */
+		const char *name = ck_def->name;
+		struct ck_constraint *old_ck_constraint =
+			space_ck_constraint_by_name(space, name, strlen(name));
+		if (old_ck_constraint != NULL) {
+			struct ck_constraint_def *old_def =
+						old_ck_constraint->def;
+			assert(old_def->space_id == ck_def->space_id);
+			assert(strcmp(old_def->name, ck_def->name) == 0);
+			if (old_def->language == ck_def->language &&
+			    strcmp(old_def->expr_str, ck_def->expr_str) == 0) {
+				old_def->is_enabled = ck_def->is_enabled;
+				trigger_run_xc(&on_alter_space, space);
+				return;
+			}
+		}
 		/*
 		 * FIXME: Ck constraint creation on non-empty
 		 * space is not implemented yet.
@@ -4704,8 +4727,7 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 		struct index *pk = space_index(space, 0);
 		if (pk != NULL && index_size(pk) > 0) {
 			tnt_raise(ClientError, ER_CREATE_CK_CONSTRAINT,
-				  ck_def->name,
-				  "referencing space must be empty");
+				  name, "referencing space must be empty");
 		}
 		struct ck_constraint *new_ck_constraint =
 			ck_constraint_new(ck_def, space->def);
@@ -4715,9 +4737,6 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 		auto ck_guard = make_scoped_guard([=] {
 			ck_constraint_delete(new_ck_constraint);
 		});
-		const char *name = new_ck_constraint->def->name;
-		struct ck_constraint *old_ck_constraint =
-			space_ck_constraint_by_name(space, name, strlen(name));
 		if (old_ck_constraint != NULL)
 			rlist_del_entry(old_ck_constraint, link);
 		if (space_add_ck_constraint(space, new_ck_constraint) != 0)
diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap
index 4c9aea7f50f8ac86be32ca9f126fea9a3d2d182f..59f6cc16bb48841fa99b4e13590b8e0677433b35 100644
GIT binary patch
delta 4861

diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
index 1cde27022..d21717f94 100644
--- a/src/box/ck_constraint.c
+++ b/src/box/ck_constraint.c
@@ -44,7 +44,7 @@ const char *ck_constraint_language_strs[] = {"SQL"};
 struct ck_constraint_def *
 ck_constraint_def_new(const char *name, uint32_t name_len, const char *expr_str,
 		      uint32_t expr_str_len, uint32_t space_id,
-		      enum ck_constraint_language language)
+		      enum ck_constraint_language language, bool is_enabled)
 {
 	uint32_t expr_str_offset;
 	uint32_t ck_def_sz = ck_constraint_def_sizeof(name_len, expr_str_len,
@@ -55,6 +55,7 @@ ck_constraint_def_new(const char *name, uint32_t name_len, const char *expr_str,
 		diag_set(OutOfMemory, ck_def_sz, "malloc", "ck_def");
 		return NULL;
 	}
+	ck_def->is_enabled = is_enabled;
 	ck_def->expr_str = (char *)ck_def + expr_str_offset;
 	ck_def->language = language;
 	ck_def->space_id = space_id;
@@ -201,7 +202,8 @@ ck_constraint_on_replace_trigger(struct trigger *trigger, void *event)
 
 	struct ck_constraint *ck_constraint;
 	rlist_foreach_entry(ck_constraint, &space->ck_constraint, link) {
-		if (ck_constraint_program_run(ck_constraint, field_ref) != 0)
+		if (ck_constraint->def->is_enabled &&
+		    ck_constraint_program_run(ck_constraint, field_ref) != 0)
 			diag_raise();
 	}
 }
diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
index f26f77a38..6761318d6 100644
--- a/src/box/ck_constraint.h
+++ b/src/box/ck_constraint.h
@@ -71,6 +71,11 @@ struct ck_constraint_def {
 	 * defined for.
 	 */
 	uint32_t space_id;
+	/**
+	 * Per constraint option regulating its execution: it is
+	 * disabled (set to false) contraint won't be fired.
+	 */
+	bool is_enabled;
 	/** The language of ck constraint. */
 	enum ck_constraint_language language;
 	/**
@@ -140,6 +145,7 @@ ck_constraint_def_sizeof(uint32_t name_len, uint32_t expr_str_len,
  * @param expr_str_len The length of the @a expr string.
  * @param space_id The identifier of the target space.
  * @param language The language of the @a expr string.
+ * @param is_enabled Whether this ck constraint should be fired.
  * @retval not NULL Check constraint definition object pointer
  *                  on success.
  * @retval NULL Otherwise. The diag message is set.
@@ -147,7 +153,7 @@ ck_constraint_def_sizeof(uint32_t name_len, uint32_t expr_str_len,
 struct ck_constraint_def *
 ck_constraint_def_new(const char *name, uint32_t name_len, const char *expr,
 		      uint32_t expr_str_len, uint32_t space_id,
-		      enum ck_constraint_language language);
+		      enum ck_constraint_language language, bool is_enabled);
 
 /**
  * Destroy check constraint definition memory, release acquired
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 98067f795..e898c3aa6 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -1706,7 +1706,7 @@ space_mt.create_check_constraint = function(space, name, code)
         box.error(box.error.PROC_LUA,
                   "Usage: space:create_constraint(name, code)")
     end
-    box.space._ck_constraint:insert({space.id, name, false, 'SQL', code})
+    box.space._ck_constraint:insert({space.id, name, false, 'SQL', code, true})
     return space.ck_constraint[name]
 end
 space_mt.pairs = function(space, key, opts)
@@ -1759,6 +1759,20 @@ ck_constraint_mt.drop = function(ck_constraint)
     check_ck_constraint_arg(ck_constraint, 'drop')
     box.space._ck_constraint:delete({ck_constraint.space_id, ck_constraint.name})
 end
+ck_constraint_mt.enable = function(ck_constraint, yesno)
+    check_ck_constraint_arg(ck_constraint, 'enable')
+    local s = builtin.space_by_id(ck_constraint.space_id)
+    if s == nil then
+        box.error(box.error.NO_SUCH_SPACE, tostring(ck_constraint.space_id))
+    end
+    local t = box.space._ck_constraint:get({ck_constraint.space_id,
+                                            ck_constraint.name})
+    if t == nil then
+        box.error(box.error.NO_SUCH_CONSTRAINT, tostring(ck_constraint.name))
+    end
+    box.space._ck_constraint:update({ck_constraint.space_id, ck_constraint.name},
+                                    {{'=', 6, yesno}})
+end
 
 box.schema.index_mt = base_index_mt
 box.schema.memtx_index_mt = memtx_index_mt
diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
index d0a7e7815..8b84e1fb2 100644
--- a/src/box/lua/space.cc
+++ b/src/box/lua/space.cc
@@ -205,6 +205,9 @@ lbox_push_ck_constraint(struct lua_State *L, struct space *space, int i)
 		lua_pushstring(L, ck_constraint->def->expr_str);
 		lua_setfield(L, -2, "expr");
 
+		lua_pushboolean(L, ck_constraint->def->is_enabled);
+		lua_setfield(L, -2, "is_enabled");
+
 		lua_setfield(L, -2, ck_constraint->def->name);
 	}
 	lua_pop(L, 1);
diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua
index 2abd75dff..e71b7fb41 100644
--- a/src/box/lua/upgrade.lua
+++ b/src/box/lua/upgrade.lua
@@ -927,6 +927,19 @@ local function upgrade_to_2_3_0()
                                         datetime, datetime})
         _priv:replace{ADMIN, PUBLIC, 'function', t.id, box.priv.X}
     end
+
+    log.info("Extend _ck_constraint space format with is_enabled field")
+    local _ck_constraint = box.space._ck_constraint
+    for _, tuple in _ck_constraint:pairs() do
+        _ck_constraint:update({tuple[1], tuple[2]}, {{'=', 6, true}})
+    end
+    local format = {{name='space_id', type='unsigned'},
+                    {name='name', type='string'},
+                    {name='is_deferred', type='boolean'},
+                    {name='language', type='str'},
+                    {name='code', type='str'},
+                    {name='is_enabled', type='boolean'}}
+    _ck_constraint:format(format)
 end
 
 --------------------------------------------------------------------------------
diff --git a/src/box/schema_def.h b/src/box/schema_def.h
index 85f652d52..ba870ff42 100644
--- a/src/box/schema_def.h
+++ b/src/box/schema_def.h
@@ -267,6 +267,7 @@ enum {
 	BOX_CK_CONSTRAINT_FIELD_DEFERRED = 2,
 	BOX_CK_CONSTRAINT_FIELD_LANGUAGE = 3,
 	BOX_CK_CONSTRAINT_FIELD_CODE = 4,
+	BOX_CK_CONSTRAINT_FIELD_IS_ENABLED = 5,
 };
 
 /** _func_index fields. */
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 233f56734..51cd7ce63 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -982,7 +982,7 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 	 * Occupy registers for 5 fields: each member in
 	 * _ck_constraint space plus one for final msgpack tuple.
 	 */
-	int ck_constraint_reg = sqlGetTempRange(parser, 6);
+	int ck_constraint_reg = sqlGetTempRange(parser, 7);
 	sqlVdbeAddOp2(v, OP_SCopy, reg_space_id, ck_constraint_reg);
 	sqlVdbeAddOp4(v, OP_String8, 0, ck_constraint_reg + 1, 0,
 		      sqlDbStrDup(db, ck_def->name), P4_DYNAMIC);
@@ -991,8 +991,9 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 		      ck_constraint_language_strs[ck_def->language], P4_STATIC);
 	sqlVdbeAddOp4(v, OP_String8, 0, ck_constraint_reg + 4, 0,
 		      sqlDbStrDup(db, ck_def->expr_str), P4_DYNAMIC);
-	sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 5,
-		      ck_constraint_reg + 5);
+	sqlVdbeAddOp2(v, OP_Bool, true, ck_constraint_reg + 5);
+	sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 6,
+		      ck_constraint_reg + 6);
 	const char *error_msg =
 		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS),
 					    ck_def->name);
@@ -1002,9 +1003,9 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 					      false, OP_NoConflict) != 0)
 		return;
 	sqlVdbeAddOp2(v, OP_SInsert, BOX_CK_CONSTRAINT_ID,
-		      ck_constraint_reg + 5);
+		      ck_constraint_reg + 6);
 	VdbeComment((v, "Create CK constraint %s", ck_def->name));
-	sqlReleaseTempRange(parser, ck_constraint_reg, 6);
+	sqlReleaseTempRange(parser, ck_constraint_reg, 7);
 }
 
 /**
diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result
index a59979e62..123aa2feb 100644
--- a/test/box-py/bootstrap.result
+++ b/test/box-py/bootstrap.result
@@ -92,7 +92,8 @@ box.space._space:select{}
       {'name': 'child_cols', 'type': 'array'}, {'name': 'parent_cols', 'type': 'array'}]]
   - [364, 1, '_ck_constraint', 'memtx', 0, {}, [{'name': 'space_id', 'type': 'unsigned'},
       {'name': 'name', 'type': 'string'}, {'name': 'is_deferred', 'type': 'boolean'},
-      {'name': 'language', 'type': 'str'}, {'name': 'code', 'type': 'str'}]]
+      {'name': 'language', 'type': 'str'}, {'name': 'code', 'type': 'str'}, {'name': 'is_enabled',
+        'type': 'boolean'}]]
   - [372, 1, '_func_index', 'memtx', 0, {}, [{'name': 'space_id', 'type': 'unsigned'},
       {'name': 'index_id', 'type': 'unsigned'}, {'name': 'func_id', 'type': 'unsigned'}]]
 ...
diff --git a/test/box/access_misc.result b/test/box/access_misc.result
index a1b6435bc..27eb47aa0 100644
--- a/test/box/access_misc.result
+++ b/test/box/access_misc.result

diff --git a/test/sql/checks.result b/test/sql/checks.result
index 50347bc3a..72b1fa26f 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -37,35 +37,35 @@ _ = box.space.test:create_index('pk')
 ---
 ...
 -- Invalid expression test.
-box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X><5'})
+box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X><5', true})
 ---
 - error: 'Failed to create check constraint ''CK_CONSTRAINT_01'': Syntax error near
     ''<'''
 ...
 -- Non-existent space test.
-box.space._ck_constraint:insert({550, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'})
+box.space._ck_constraint:insert({550, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true})
 ---
 - error: Space '550' does not exist
 ...
 -- Pass integer instead of expression.
-box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 666})
+box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 666, true})
 ---
 - error: 'Tuple field 5 type does not match one required by operation: expected string'
 ...
 -- Deferred CK constraints are not supported.
-box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', true, 'SQL', 'X<5'})
+box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', true, 'SQL', 'X<5', true})
 ---
 - error: Tarantool does not support deferred ck constraints
 ...
 -- The only supported language is SQL.
-box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'LUA', 'X<5'})
+box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'LUA', 'X<5', true})
 ---
 - error: Unsupported language 'LUA' specified for function 'CK_CONSTRAINT_01'
 ...
 -- Check constraints LUA creation test.
-box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'})
+box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true})
 ---
-- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5']
+- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true]
 ...
 box.space._ck_constraint:count({})
 ---
@@ -80,9 +80,9 @@ box.space.test:insert({5})
 ---
 - error: 'Check constraint failed ''CK_CONSTRAINT_01'': X<5'
 ...
-box.space._ck_constraint:replace({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'})
+box.space._ck_constraint:replace({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true})
 ---
-- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5']
+- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true]
 ...
 box.execute("INSERT INTO \"test\" VALUES(5);")
 ---
@@ -111,7 +111,7 @@ box.space._space:delete({513})
 ...
 box.space._ck_constraint:delete({513, 'CK_CONSTRAINT_01'})
 ---
-- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5']
+- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true]
 ...
 box.space._space:delete({513})
 ---
@@ -177,14 +177,14 @@ s = box.schema.create_space('test', {engine = engine})
 _ = s:create_index('pk')
 ---
 ...
-_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y'})
+_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y', true})
 ---
 - error: Tarantool does not support CK constraint for space without format
 ...
 s:format({{name='X', type='integer'}, {name='Y', type='integer'}})
 ---
 ...
-_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y'})
+_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y', true})
 ---
 ...
 box.execute("INSERT INTO \"test\" VALUES(2, 1);")
@@ -229,7 +229,7 @@ s:insert({2, 1})
 ---
 - [2, 1]
 ...
-_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10'})
+_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10', true})
 ---
 - error: 'Failed to create check constraint ''conflict'': referencing space must be
     empty'
@@ -237,7 +237,7 @@ _ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10'})
 s:truncate()
 ---
 ...
-_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10'})
+_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10', true})
 ---
 ...
 box.execute("INSERT INTO \"test\" VALUES(1, 2);")
@@ -332,7 +332,7 @@ _ = s:create_index('pk')
 s:format({{name='X', type='any'}, {name='Y', type='integer'}, {name='Z', type='integer', is_nullable=true}})
 ---
 ...
-ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'X = 1 AND Z IS NOT NULL'})
+ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'X = 1 AND Z IS NOT NULL', true})
 ---
 ...
 s:insert({2, 1})
@@ -374,7 +374,7 @@ _ = s:create_index('pk')
 s:format({{name='X', type='any'}, {name='Y', type='integer'}, {name='Z', type='integer', is_nullable=true}})
 ---
 ...
-ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'Z IS NOT NULL'})
+ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'Z IS NOT NULL', true})
 ---
 ...
 s:insert({1, 2, box.NULL})
@@ -388,7 +388,7 @@ s:insert({1, 2})
 _ = box.space._ck_constraint:delete({s.id, 'ZnotNULL'})
 ---
 ...
-_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y and Y < Z'})
+_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y and Y < Z', true})
 ---
 ...
 s:insert({'1', 2})
@@ -464,7 +464,7 @@ _ = s:create_index('pk', {parts = {3, 'integer'}})
 _ = s:create_index('unique', {parts = {1, 'integer'}})
 ---
 ...
-_ = box.space._ck_constraint:insert({s.id, 'complex1', false, 'SQL', 'x+y==11 OR x*y==12 OR x/y BETWEEN 5 AND 8 OR -x == y+10'})
+_ = box.space._ck_constraint:insert({s.id, 'complex1', false, 'SQL', 'x+y==11 OR x*y==12 OR x/y BETWEEN 5 AND 8 OR -x == y+10', true})
 ---
 ...
 s:insert({1, 10, 1})
@@ -513,7 +513,7 @@ s:format({{name='X', type='integer'}, {name='Z', type='any'}})
 _ = s:create_index('pk', {parts = {1, 'integer'}})
 ---
 ...
-_ = box.space._ck_constraint:insert({s.id, 'complex2', false, 'SQL', 'typeof(coalesce(z,0))==\'integer\''})
+_ = box.space._ck_constraint:insert({s.id, 'complex2', false, 'SQL', 'typeof(coalesce(z,0))==\'integer\'', true})
 ---
 ...
 s:insert({1, 'string'})
@@ -564,7 +564,7 @@ test_run:cmd("setopt delimiter ''");
 s:format(format65)
 ---
 ...
-_ = box.space._ck_constraint:insert({s.id, 'X1is666andX65is666', false, 'SQL', 'X1 == 666 and X65 == 666 and X63 IS NOT NULL'})
+_ = box.space._ck_constraint:insert({s.id, 'X1is666andX65is666', false, 'SQL', 'X1 == 666 and X65 == 666 and X63 IS NOT NULL', true})
 ---
 ...
 s:insert(s:frommap({X1 = 1, X65 = 1}))
@@ -642,24 +642,28 @@ _ = s2:create_check_constraint('greater', 'X > 20')
 s1.ck_constraint.physics
 ---
 - space_id: <ID>
+  is_enabled: true
   name: physics
   expr: X < Y
 ...
 s1.ck_constraint.greater
 ---
 - space_id: <ID>
+  is_enabled: true
   name: greater
   expr: X > 20
 ...
 s2.ck_constraint.physics
 ---
 - space_id: <ID>
+  is_enabled: true
   name: physics
   expr: X > Y
 ...
 s2.ck_constraint.greater
 ---
 - space_id: <ID>
+  is_enabled: true
   name: greater
   expr: X > 20
 ...
@@ -685,6 +689,7 @@ s2.ck_constraint.greater:drop()
 s2.ck_constraint.physics
 ---
 - space_id: <ID>
+  is_enabled: true
   name: physics
   expr: X > Y
 ...
@@ -716,12 +721,75 @@ s2:drop()
 physics_ck
 ---
 - space_id: <ID>
+  is_enabled: true
   name: physics
   expr: X > Y
 ...
 physics_ck:drop()
 ---
 ...
+--
+-- gh-4244: Add an ability to disable CK constraints
+-- Make sure that ck constraints are turned on/off with
+-- :enable configurator.
+--
+engine = test_run:get_cfg('engine')
+---
+...
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+---
+- row_count: 0
+...
+box.execute("CREATE TABLE test(a INT PRIMARY KEY);");
+---
+- row_count: 1
+...
+box.execute('ALTER TABLE test ADD CONSTRAINT CK CHECK(a < 5);')
+---
+- row_count: 1
+...
+box.space.TEST:insert({10})
+---
+- error: 'Check constraint failed ''CK'': a < 5'
+...
+box.space.TEST.ck_constraint.CK:enable(false)
+---
+...
+assert(box.space.TEST.ck_constraint.CK.is_enabled == false)
+---
+- true
+...
+box.space.TEST:insert({11})
+---
+- [11]
+...
+-- Test is_enabled state persistency.
+test_run:cmd("restart server default")
+test_run = require('test_run').new()
+---
+...
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+---
+- true
+...
+box.space.TEST:insert({12})
+---
+- [12]
+...
+box.space.TEST.ck_constraint.CK:enable(true)
+---
+...
+assert(box.space.TEST.ck_constraint.CK.is_enabled == true)
+---
+- true
+...
+box.space.TEST:insert({13})
+---
+- error: 'Check constraint failed ''CK'': a < 5'
+...
+box.space.TEST:drop()
+---
+...
 test_run:cmd("clear filter")
 ---
 - true
diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
index cde213f8b..11918dced 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -18,23 +18,23 @@ s = box.space._space:insert(t)
 _ = box.space.test:create_index('pk')
 
 -- Invalid expression test.
-box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X><5'})
+box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X><5', true})
 -- Non-existent space test.
-box.space._ck_constraint:insert({550, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'})
+box.space._ck_constraint:insert({550, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true})
 -- Pass integer instead of expression.
-box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 666})
+box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 666, true})
 -- Deferred CK constraints are not supported.
-box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', true, 'SQL', 'X<5'})
+box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', true, 'SQL', 'X<5', true})
 -- The only supported language is SQL.
-box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'LUA', 'X<5'})
+box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'LUA', 'X<5', true})
 
 -- Check constraints LUA creation test.
-box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'})
+box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true})
 box.space._ck_constraint:count({})
 
 box.execute("INSERT INTO \"test\" VALUES(5);")
 box.space.test:insert({5})
-box.space._ck_constraint:replace({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'})
+box.space._ck_constraint:replace({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true})
 box.execute("INSERT INTO \"test\" VALUES(5);")
 box.execute("INSERT INTO \"test\" VALUES(6);")
 box.space.test:insert({6})
@@ -64,9 +64,9 @@ box.space._ck_constraint:count() == 0
 -- Ck constraints are disallowed for spaces having no format.
 s = box.schema.create_space('test', {engine = engine})
 _ = s:create_index('pk')
-_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y'})
+_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y', true})
 s:format({{name='X', type='integer'}, {name='Y', type='integer'}})
-_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y'})
+_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y', true})
 box.execute("INSERT INTO \"test\" VALUES(2, 1);")
 s:format({{name='Y', type='integer'}, {name='X', type='integer'}})
 box.execute("INSERT INTO \"test\" VALUES(1, 2);")
@@ -78,9 +78,9 @@ s:format()
 s:format({{name='Y1', type='integer'}, {name='X1', type='integer'}})
 -- Ck constraint creation is forbidden for non-empty space
 s:insert({2, 1})
-_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10'})
+_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10', true})
 s:truncate()
-_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10'})
+_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10', true})
 box.execute("INSERT INTO \"test\" VALUES(1, 2);")
 box.execute("INSERT INTO \"test\" VALUES(11, 11);")
 box.execute("INSERT INTO \"test\" VALUES(12, 11);")
@@ -114,7 +114,7 @@ box.execute("DROP TABLE t1")
 s = box.schema.create_space('test', {engine = engine})
 _ = s:create_index('pk')
 s:format({{name='X', type='any'}, {name='Y', type='integer'}, {name='Z', type='integer', is_nullable=true}})
-ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'X = 1 AND Z IS NOT NULL'})
+ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'X = 1 AND Z IS NOT NULL', true})
 s:insert({2, 1})
 s:insert({1, 1})
 s:insert({1, 1, box.NULL})
@@ -129,11 +129,11 @@ s:drop()
 s = box.schema.create_space('test', {engine = engine})
 _ = s:create_index('pk')
 s:format({{name='X', type='any'}, {name='Y', type='integer'}, {name='Z', type='integer', is_nullable=true}})
-ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'Z IS NOT NULL'})
+ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'Z IS NOT NULL', true})
 s:insert({1, 2, box.NULL})
 s:insert({1, 2})
 _ = box.space._ck_constraint:delete({s.id, 'ZnotNULL'})
-_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y and Y < Z'})
+_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y and Y < Z', true})
 s:insert({'1', 2})
 s:insert({})
 s:insert({2, 1})
@@ -157,7 +157,7 @@ s = box.schema.create_space('test', {engine = engine})
 s:format({{name='X', type='integer'}, {name='Y', type='integer'}, {name='Z', type='integer'}})
 _ = s:create_index('pk', {parts = {3, 'integer'}})
 _ = s:create_index('unique', {parts = {1, 'integer'}})
-_ = box.space._ck_constraint:insert({s.id, 'complex1', false, 'SQL', 'x+y==11 OR x*y==12 OR x/y BETWEEN 5 AND 8 OR -x == y+10'})
+_ = box.space._ck_constraint:insert({s.id, 'complex1', false, 'SQL', 'x+y==11 OR x*y==12 OR x/y BETWEEN 5 AND 8 OR -x == y+10', true})
 s:insert({1, 10, 1})
 s:update({1}, {{'=', 1, 4}, {'=', 2, 3}})
 s:update({1}, {{'=', 1, 12}, {'=', 2, 2}})
@@ -171,7 +171,7 @@ s:drop()
 s = box.schema.create_space('test', {engine = engine})
 s:format({{name='X', type='integer'}, {name='Z', type='any'}})
 _ = s:create_index('pk', {parts = {1, 'integer'}})
-_ = box.space._ck_constraint:insert({s.id, 'complex2', false, 'SQL', 'typeof(coalesce(z,0))==\'integer\''})
+_ = box.space._ck_constraint:insert({s.id, 'complex2', false, 'SQL', 'typeof(coalesce(z,0))==\'integer\'', true})
 s:insert({1, 'string'})
 s:insert({1, {map=true}})
 s:insert({1, {'a', 'r','r','a','y'}})
@@ -191,7 +191,7 @@ for i = 1,66 do
 end
 test_run:cmd("setopt delimiter ''");
 s:format(format65)
-_ = box.space._ck_constraint:insert({s.id, 'X1is666andX65is666', false, 'SQL', 'X1 == 666 and X65 == 666 and X63 IS NOT NULL'})
+_ = box.space._ck_constraint:insert({s.id, 'X1is666andX65is666', false, 'SQL', 'X1 == 666 and X65 == 666 and X63 IS NOT NULL', true})
 s:insert(s:frommap({X1 = 1, X65 = 1}))
 s:insert(s:frommap({X1 = 666, X65 = 1}))
 s:insert(s:frommap({X1 = 1, X65 = 666}))
@@ -234,4 +234,27 @@ s2:drop()
 physics_ck
 physics_ck:drop()
 
+--
+-- gh-4244: Add an ability to disable CK constraints
+-- Make sure that ck constraints are turned on/off with
+-- :enable configurator.
+--
+engine = test_run:get_cfg('engine')
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+box.execute("CREATE TABLE test(a INT PRIMARY KEY);");
+box.execute('ALTER TABLE test ADD CONSTRAINT CK CHECK(a < 5);')
+box.space.TEST:insert({10})
+box.space.TEST.ck_constraint.CK:enable(false)
+assert(box.space.TEST.ck_constraint.CK.is_enabled == false)
+box.space.TEST:insert({11})
+-- Test is_enabled state persistency.
+test_run:cmd("restart server default")
+test_run = require('test_run').new()
+test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
+box.space.TEST:insert({12})
+box.space.TEST.ck_constraint.CK:enable(true)
+assert(box.space.TEST.ck_constraint.CK.is_enabled == true)
+box.space.TEST:insert({13})
+box.space.TEST:drop()
+
 test_run:cmd("clear filter")
diff --git a/test/sql/errinj.result b/test/sql/errinj.result
index ecc194fb8..7ab522f49 100644
--- a/test/sql/errinj.result
+++ b/test/sql/errinj.result
@@ -410,7 +410,7 @@ errinj.set("ERRINJ_WAL_IO", true)
 ---
 - ok
 ...
-_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'})
+_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true})
 ---
 - error: Failed to write to disk
 ...
@@ -418,7 +418,7 @@ errinj.set("ERRINJ_WAL_IO", false)
 ---
 - ok
 ...
-_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'})
+_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true})
 ---
 ...
 box.execute("INSERT INTO \"test\" VALUES(5);")
@@ -430,7 +430,7 @@ errinj.set("ERRINJ_WAL_IO", true)
 ---
 - ok
 ...
-_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'})
+_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true})
 ---
 - error: Failed to write to disk
 ...
@@ -438,7 +438,7 @@ errinj.set("ERRINJ_WAL_IO", false)
 ---
 - ok
 ...
-_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'})
+_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true})
 ---
 ...
 box.execute("INSERT INTO \"test\" VALUES(5);")
@@ -484,10 +484,10 @@ _ = s:create_index('pk')
 s:format({{name='X', type='integer'}, {name='Y', type='integer'}})
 ---
 ...
-_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y'})
+_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y', true})
 ---
 ...
-_ = box.space._ck_constraint:insert({s.id, 'Xgreater10', false, 'SQL', 'X > 10'})
+_ = box.space._ck_constraint:insert({s.id, 'Xgreater10', false, 'SQL', 'X > 10', true})
 ---
 ...
 box.execute("INSERT INTO \"test\" VALUES(1, 2);")
diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua
index 2b4f74a82..b978767ad 100644
--- a/test/sql/errinj.test.lua
+++ b/test/sql/errinj.test.lua
@@ -126,14 +126,14 @@ 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({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'})
+_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true})
 errinj.set("ERRINJ_WAL_IO", false)
-_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'})
+_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true})
 box.execute("INSERT INTO \"test\" VALUES(5);")
 errinj.set("ERRINJ_WAL_IO", true)
-_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'})
+_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true})
 errinj.set("ERRINJ_WAL_IO", false)
-_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'})
+_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true})
 box.execute("INSERT INTO \"test\" VALUES(5);")
 errinj.set("ERRINJ_WAL_IO", true)
 _ = box.space._ck_constraint:delete({s.id, 'CK_CONSTRAINT_01'})
@@ -149,8 +149,8 @@ s:drop()
 s = box.schema.create_space('test')
 _ = s:create_index('pk')
 s:format({{name='X', type='integer'}, {name='Y', type='integer'}})
-_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y'})
-_ = box.space._ck_constraint:insert({s.id, 'Xgreater10', false, 'SQL', 'X > 10'})
+_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y', true})
+_ = box.space._ck_constraint:insert({s.id, 'Xgreater10', false, 'SQL', 'X > 10', true})
 box.execute("INSERT INTO \"test\" VALUES(1, 2);")
 box.execute("INSERT INTO \"test\" VALUES(20, 10);")
 box.execute("INSERT INTO \"test\" VALUES(20, 100);")
-- 
2.23.0

Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 4/4] sql: use name instead of function pointer for UDF

[Nikita Pettik]

On 15 Oct 14:13, Kirill Shcherbatov wrote:
> > Could you add test case covering mentioned situation?
> Yes. Appended.
> 
> =================================================
> 
> This patch changes OP_Function parameters convention: now a
> function's name is passed instead of pointer to the function
> object. This allows to normally handle the situation, when UDF
> has been deleted to the moment of the VDBE code execution.
> In particular case this may happen with CK constraints that
> refers to a deleted persistent function.
> 
> Closes #4176
> ---
>  src/box/sql/expr.c       | 17 ++++++++++++-----
>  src/box/sql/vdbe.c       | 11 ++++++++---
>  test/sql/checks.result   | 28 ++++++++++++++++++++++++++++
>  test/sql/checks.test.lua | 11 +++++++++++
>  4 files changed, 59 insertions(+), 8 deletions(-)
> 
> diff --git a/test/sql/checks.result b/test/sql/checks.result
> index b8bd19a84..02ee82a4b 100644
> --- a/test/sql/checks.result
> +++ b/test/sql/checks.result
> @@ -918,6 +918,34 @@ box.execute("DROP TABLE test;")
>  ---
>  - row_count: 1
>  ...
> +--
> +-- gh-4176: Can't recover if check constraint involves function.
> +--
> +function myfunc(x) return x < 10 end
> +---
> +...
> +box.schema.func.create('MYFUNC', {param_list = {'integer'}, returns = 'integer', is_deterministic = true, exports = {'LUA', 'SQL'}})
> +---
> +...
> +box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
> +---
> +- row_count: 1
> +...
> +myfunc = nil
> +---
> +...
> +box.execute("INSERT INTO t6 VALUES(11);");
> +---
> +- null
> +- Procedure 'MYFUNC' is not defined

Something is wrong with this test case. Firstly, it works fine
even on master branch. Secondly, I've dropped myfunc = nil and
got the same error (on master as well) - i.e. INSERT fails with
'procedure is not defined' error even if function is not dropped.
Obviously, to make it crash on master you should drop function
before insertion; to fix second problem you should uppercase function's
name.

> +box.execute("DROP TABLE t6")
> +---
> +- row_count: 1
> +...
> +box.func.MYFUNC:drop()
> +---
> +...
>  test_run:cmd("clear filter")
>  ---
>  - true
> diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
> index 832322190..fa68920f6 100644
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> @@ -302,4 +302,15 @@ assert(box.space.TEST.ck_constraint.some_ck.is_enabled == true)
>  box.space.TEST:insert({12})
>  box.execute("DROP TABLE test;")
>  
> +--
> +-- gh-4176: Can't recover if check constraint involves function.
> +--
> +function myfunc(x) return x < 10 end
> +box.schema.func.create('MYFUNC', {param_list = {'integer'}, returns = 'integer', is_deterministic = true, exports = {'LUA', 'SQL'}})
> +box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
> +myfunc = nil
> +box.execute("INSERT INTO t6 VALUES(11);");
> +box.execute("DROP TABLE t6")
> +box.func.MYFUNC:drop()
> +
>  test_run:cmd("clear filter")
> -- 
> 2.23.0
> 

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints

[Nikita Pettik]

On 15 Oct 14:13, Kirill Shcherbatov wrote:
> > -> when processed data is verified and constraints validation
> > is not required. For instance, during casual recovery process there's
> > no need to provide any checks since data is assumed to be consistent.
> Applied.
> 
> > Also explain why we have to store this option.
> > To be honest, I doubt that 'is_enable' option should be persisted..
> > At least, we can assume that it is always 'on' by default, so that
> > save user from having to specify this option each time.
> 
> Verbally discussed. To implement a behavior that Oracle users are familiar with.
> It was a Kostya's point of view and it is already implemented.

Verbally I said that 'somebody told me to do so' is a quite weak argument.
So we either add fine rationale for this feature or remove it. Since it
was not my suggestion I can't say why do we need it.
 

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/10/16 08:45]:
> On 15 Oct 14:13, Kirill Shcherbatov wrote:
> > > -> when processed data is verified and constraints validation
> > > is not required. For instance, during casual recovery process there's
> > > no need to provide any checks since data is assumed to be consistent.
> > Applied.
> > 
> > > Also explain why we have to store this option.
> > > To be honest, I doubt that 'is_enable' option should be persisted..
> > > At least, we can assume that it is always 'on' by default, so that
> > > save user from having to specify this option each time.
> > 
> > Verbally discussed. To implement a behavior that Oracle users are familiar with.
> > It was a Kostya's point of view and it is already implemented.
> 
> Verbally I said that 'somebody told me to do so' is a quite weak argument.
> So we either add fine rationale for this feature or remove it. Since it
> was not my suggestion I can't say why do we need it.

Nikita, if the option is not stored the following scenario is
possible:

- the option is turned off
- data is changed so that a constraint is violated
- the system is restarted while the option state is lost
- there is no way (even in theory) to discover it and find that
  data is incorrect.

If the option is persistent you can run a check when it is turned
back on again.

If we don't store the option, we should not wire it up in the
interface and only use internally, during recovery, which would
make it difficult to test the option.
>  

-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints

[Nikita Pettik]

On 16 Oct 08:52, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/10/16 08:45]:
> > On 15 Oct 14:13, Kirill Shcherbatov wrote:
> > > > -> when processed data is verified and constraints validation
> > > > is not required. For instance, during casual recovery process there's
> > > > no need to provide any checks since data is assumed to be consistent.
> > > Applied.
> > > 
> > > > Also explain why we have to store this option.
> > > > To be honest, I doubt that 'is_enable' option should be persisted..
> > > > At least, we can assume that it is always 'on' by default, so that
> > > > save user from having to specify this option each time.
> > > 
> > > Verbally discussed. To implement a behavior that Oracle users are familiar with.
> > > It was a Kostya's point of view and it is already implemented.
> > 
> > Verbally I said that 'somebody told me to do so' is a quite weak argument.
> > So we either add fine rationale for this feature or remove it. Since it
> > was not my suggestion I can't say why do we need it.
> 
> Nikita, if the option is not stored the following scenario is
> possible:
> 
> - the option is turned off
> - data is changed so that a constraint is violated
> - the system is restarted while the option state is lost
> - there is no way (even in theory) to discover it and find that
>   data is incorrect.

Thanks for the scenario, now it looks reasonable. On the other hand,
if we decided to always run check constraints (both on casual and force
recovery), we would easily find out that data violates contraints.

Kirill, could you please extend commit message with Konstantin's explanation?

> If the option is persistent you can run a check when it is turned
> back on again.
> 
> If we don't store the option, we should not wire it up in the
> interface and only use internally, during recovery, which would
> make it difficult to test the option.
> >  
> 
> -- 
> Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints

[Kirill Shcherbatov]

> Thanks for the scenario, now it looks reasonable. On the other hand,
> if we decided to always run check constraints (both on casual and force
> recovery), we would easily find out that data violates contraints.
> 
> Kirill, could you please extend commit message with Konstantin's explanation?

Done.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 4/4] sql: use name instead of function pointer for UDF

[Kirill Shcherbatov]

> Something is wrong with this test case. Firstly, it works fine
> even on master branch. Secondly, I've dropped myfunc = nil and
> got the same error (on master as well) - i.e. INSERT fails with
> 'procedure is not defined' error even if function is not dropped.
> Obviously, to make it crash on master you should drop function
> before insertion; to fix second problem you should uppercase function's
> name.
You are right. A fixed test case is below.

+--
+-- gh-4176: Can't recover if check constraint involves function.
+--
+function myfunc(x) return x < 10 end
+box.schema.func.create('MYFUNC', {param_list = {'integer'}, returns = 'integer', is_deterministic = true, exports = {'LUA', 'SQL'}})
+box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
+box.func.MYFUNC:drop()
+box.execute("INSERT INTO t6 VALUES(11);");
+box.execute("DROP TABLE t6")
+
 test_run:cmd("clear filter")

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 4/4] sql: use name instead of function pointer for UDF

[Nikita Pettik]

On 16 Oct 16:51, Kirill Shcherbatov wrote:
> > Something is wrong with this test case. Firstly, it works fine
> > even on master branch. Secondly, I've dropped myfunc = nil and
> > got the same error (on master as well) - i.e. INSERT fails with
> > 'procedure is not defined' error even if function is not dropped.
> > Obviously, to make it crash on master you should drop function
> > before insertion; to fix second problem you should uppercase function's
> > name.
> You are right. A fixed test case is below.
> 
> +--
> +-- gh-4176: Can't recover if check constraint involves function.
> +--
> +function myfunc(x) return x < 10 end

Still name is not upper-cased. Fixed and pushed.

> +box.schema.func.create('MYFUNC', {param_list = {'integer'}, returns = 'integer', is_deterministic = true, exports = {'LUA', 'SQL'}})
> +box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
> +box.func.MYFUNC:drop()
> +box.execute("INSERT INTO t6 VALUES(11);");
> +box.execute("DROP TABLE t6")
> +
>  test_run:cmd("clear filter")

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints

[Nikita Pettik]

On 16 Oct 16:50, Kirill Shcherbatov wrote:
> > Thanks for the scenario, now it looks reasonable. On the other hand,
> > if we decided to always run check constraints (both on casual and force
> > recovery), we would easily find out that data violates contraints.
> > 
> > Kirill, could you please extend commit message with Konstantin's explanation?
> 
> Done.

Pushed to master.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 2/4] sql: add an ability to disable CK constraints

[Nikita Pettik]

On 15 Oct 14:13, Kirill Shcherbatov wrote:
> >> Now it is possible to disable and enable ck constraints.
> >> This option is not persistent.
> > 
> > Outdated comment.
> 
> Fixed.
> 
> > Too many new lines.
> 
> Fixed.

Pushed to master.
 

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery

[Nikita Pettik]

On 15 Oct 14:13, Kirill Shcherbatov wrote:
> 
> Tarantool used to validate ck constraints for all spaces on each
> recovery. It is not good practice, because the recovery process
> should be fast and data in snapshot is typically valid.
> The new practice coincides with secondary keys building practice:
> we do not perform consistency checks by default, except the
> force_recovery = true option is specified.
> 
> Closes #4176

Note that I haven't pushed this patch. It is not required to fix #4176,
it is rather optimization issue. And there are still things to work on them. 

> ---
>  src/box/alter.cc         |  8 +++-
>  src/box/box.cc           |  8 ++--
>  src/box/ck_constraint.c  |  6 ++-
>  src/box/ck_constraint.h  |  9 ++++-
>  src/box/schema.cc        |  3 ++
>  src/box/schema.h         |  9 +++++
>  src/box/space.c          | 22 +++++++++++
>  src/box/space.h          |  7 ++++
>  test/sql/checks.result   | 79 ++++++++++++++++++++++++++++++++++++++++
>  test/sql/checks.test.lua | 30 +++++++++++++++
>  10 files changed, 173 insertions(+), 8 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index f8f1802d0..83d297665 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1558,7 +1558,7 @@ RebuildCkConstraints::prepare(struct alter_space *alter)
>  			    link) {
>  		struct ck_constraint *new_ck_constraint =
>  			ck_constraint_new(old_ck_constraint->def,
> -					  alter->new_space->def);
> +					  alter->new_space->def, true);
>  		if (new_ck_constraint == NULL)
>  			diag_raise();
>  		rlist_add_entry(&ck_constraint, new_ck_constraint, link);
> @@ -4716,6 +4716,8 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
>  			if (old_def->language == ck_def->language &&
>  			    strcmp(old_def->expr_str, ck_def->expr_str) == 0) {
>  				old_def->is_enabled = ck_def->is_enabled;
> +				old_ck_constraint->is_enabled =
> +					is_force_recovery || ck_def->is_enabled;

You made is_force_recovery be global. However, there's a lot of functions
taking force_recovery param. Please, refactor them: remove that argument
and use instead global status. What is more, you don't set is_force_recovery
to false after recovery process is finished. What is more, AFAIR we discussed
replacement of memtx_recovery_state with one global state shared among
all engines (instead, you added is_recovery_complete).


>  				trigger_run_xc(&on_alter_space, space);
>  				return;
>  			}
> @@ -4730,7 +4732,9 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
>  				  name, "referencing space must be empty");
>  		}
>  		struct ck_constraint *new_ck_constraint =
> -			ck_constraint_new(ck_def, space->def);
> +			ck_constraint_new(ck_def, space->def,
> +					  is_recovery_complete ||
> +					  is_force_recovery);
>  		if (new_ck_constraint == NULL)
>  			diag_raise();
>  		ck_def_guard.is_active = false;
> diff --git a/src/box/box.cc b/src/box/box.cc
> index baf029a09..85ae78b57 100644
> --- a/src/box/box.cc
> +++ b/src/box/box.cc
> @@ -1692,9 +1692,9 @@ engine_init()
>  	 * in checkpoints (in enigne_foreach order),
>  	 * so it must be registered first.
>  	 */
> +	is_force_recovery = cfg_geti("force_recovery");
>  	struct memtx_engine *memtx;
> -	memtx = memtx_engine_new_xc(cfg_gets("memtx_dir"),
> -				    cfg_geti("force_recovery"),
> +	memtx = memtx_engine_new_xc(cfg_gets("memtx_dir"), is_force_recovery,
>  				    cfg_getd("memtx_memory"),
>  				    cfg_geti("memtx_min_tuple_size"),
>  				    cfg_geti("strip_core"),
> @@ -210,7 +210,7 @@ ck_constraint_on_replace_trigger(struct trigger *trigger, void *event)
>  
>  struct ck_constraint *
>  ck_constraint_new(struct ck_constraint_def *ck_constraint_def,
> -		  struct space_def *space_def)
> +		  struct space_def *space_def, bool is_enabled)
>  {
>  	if (space_def->field_count == 0) {
>  		diag_set(ClientError, ER_UNSUPPORTED, "Tarantool",
> @@ -225,6 +225,8 @@ ck_constraint_new(struct ck_constraint_def *ck_constraint_def,
>  	}
>  	ck_constraint->def = NULL;
>  	ck_constraint->stmt = NULL;
> +	ck_constraint->is_enabled = ck_constraint_def->is_enabled && is_enabled;
> +
>  	rlist_create(&ck_constraint->link);
>  	struct Expr *expr =
>  		sql_expr_compile(sql_get(), ck_constraint_def->expr_str,
> diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
> index 6761318d6..836ff2822 100644
> --- a/src/box/ck_constraint.h
> +++ b/src/box/ck_constraint.h
> @@ -98,6 +98,12 @@ struct ck_constraint {
>  	 * message when ck condition unsatisfied.
>  	 */
>  	struct sql_stmt *stmt;
> +	/**
> +	 * Dispite ck_constraint_def::is_enabled property this
> +	 * option defines a state of the runtine ck constraint
> +	 * object and may be overridden manually.
> +	 */
> +	bool is_enabled;

Duplication of is_enabled status is total mess IMHO. It is required
only for recovery. Please, avoid introducing this flag.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/10/17 17:06]:
> You made is_force_recovery be global. However, there's a lot of functions
> taking force_recovery param. Please, refactor them: remove that argument
> and use instead global status. What is more, you don't set is_force_recovery
> to false after recovery process is finished. What is more, AFAIR we discussed
> replacement of memtx_recovery_state with one global state shared among
> all engines (instead, you added is_recovery_complete).

Why do you think that now that there is a global state it would be
good to use it everywhere?

Besides, given that now there is is_force_recovery and
is_recovery_complete, maybe have:

engine_is_force_recovery() 
engine_is_recovery_complete() 

declared in engine.h, rather than directly access global
variables?


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery

[Nikita Pettik]

On 17 Oct 17:12, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/10/17 17:06]:
> > You made is_force_recovery be global. However, there's a lot of functions
> > taking force_recovery param. Please, refactor them: remove that argument
> > and use instead global status. What is more, you don't set is_force_recovery
> > to false after recovery process is finished. What is more, AFAIR we discussed
> > replacement of memtx_recovery_state with one global state shared among
> > all engines (instead, you added is_recovery_complete).
> 
> Why do you think that now that there is a global state it would be
> good to use it everywhere?

Em, I don't see any pros of duplicating global state by passing it
as a parameter to functions. Meanwile using local version of force_recovery
may turn out to be confusing (at least for developers looking into code).

> Besides, given that now there is is_force_recovery and
> is_recovery_complete, maybe have:
> 
> engine_is_force_recovery() 
> engine_is_recovery_complete() 
> 
> declared in engine.h, rather than directly access global
> variables?

It's OK.

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery

[Konstantin Osipov]

* Nikita Pettik <korablev@tarantool.org> [19/10/17 17:41]:
> Em, I don't see any pros of duplicating global state by passing it
> as a parameter to functions. Meanwile using local version of force_recovery
> may turn out to be confusing (at least for developers looking into code).

Historically xlog.force_recovery is not the same as global
force_recovery. The logic is a bit more tricky. There used to be
panic_on_wal_error and panic_on_snap_error, and traces of it are
still in place.


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery

[Nikita Pettik]

On 17 Oct 18:18, Konstantin Osipov wrote:
> * Nikita Pettik <korablev@tarantool.org> [19/10/17 17:41]:
> > Em, I don't see any pros of duplicating global state by passing it
> > as a parameter to functions. Meanwile using local version of force_recovery
> > may turn out to be confusing (at least for developers looking into code).
> 
> Historically xlog.force_recovery is not the same as global
> force_recovery. The logic is a bit more tricky. There used to be
> panic_on_wal_error and panic_on_snap_error, and traces of it are
> still in place.

Ok, then I mean "refactor all places where global is_force_recover is the
same as argument force_recovery".