[tarantool-patches] [PATCH v2 0/3] sql: fixes for ck constraints involving a function

[tarantool-patches] [PATCH v2 0/3] sql: fixes for ck constraints involving a function

[Kirill Shcherbatov]

This patchset fixes some problems with CK constraints using user-defined
function in some corner case.

Branch: http://github.com/tarantool/tarantool/tree/kshch/gh-4176-ck-func-recovery-failure
Issue: https://github.com/tarantool/tarantool/issues/4176

Kirill Shcherbatov (3):
  box: an ability to disable CK constraints
  sql: disallow ck using non-persistent function
  sql: use name instead of function pointer for UDF

 extra/exports            |   1 +
 src/box/ck_constraint.c  |  23 +++++-
 src/box/ck_constraint.h  |  19 +++++
 src/box/lua/schema.lua   |  13 ++++
 src/box/lua/space.cc     |   3 +
 src/box/memtx_engine.c   |  15 ++++
 src/box/sql/expr.c       |  17 +++--
 src/box/sql/resolve.c    |  10 +++
 src/box/sql/vdbe.c       |  17 +++--
 src/box/sql/vdbe.h       |   1 +
 test/sql/checks.result   | 155 +++++++++++++++++++++++++++++++++++++++
 test/sql/checks.test.lua |  61 +++++++++++++++
 12 files changed, 323 insertions(+), 12 deletions(-)

-- 
2.23.0

[tarantool-patches] [PATCH v2 1/3] box: an ability to disable CK constraints

[Kirill Shcherbatov]

@TarantoolBot document
Title: an ability to disable CK constraints

Now it is possible to disable and enable ck constraints.
This option is not persistent. 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.

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
x.space.T6:insert({11})
-- passed

Closes #4244
---
 extra/exports            |   1 +
 src/box/ck_constraint.c  |  23 +++++++-
 src/box/ck_constraint.h  |  19 +++++++
 src/box/lua/schema.lua   |  13 +++++
 src/box/lua/space.cc     |   3 ++
 src/box/memtx_engine.c   |  15 ++++++
 test/sql/checks.result   | 112 +++++++++++++++++++++++++++++++++++++++
 test/sql/checks.test.lua |  41 ++++++++++++++
 8 files changed, 226 insertions(+), 1 deletion(-)

diff --git a/extra/exports b/extra/exports
index 7b84a1452..ecc7d102b 100644
--- a/extra/exports
+++ b/extra/exports
@@ -78,6 +78,7 @@ tarantool_exit
 log_pid
 space_by_id
 space_run_triggers
+space_ck_constraint_enable
 space_bsize
 box_schema_version
 
diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
index 1cde27022..f65715096 100644
--- a/src/box/ck_constraint.c
+++ b/src/box/ck_constraint.c
@@ -28,6 +28,7 @@
  * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
  * SUCH DAMAGE.
  */
+#include "memtx_engine.h"
 #include "box/session.h"
 #include "bind.h"
 #include "ck_constraint.h"
@@ -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->is_enabled &&
+		    ck_constraint_program_run(ck_constraint, field_ref) != 0)
 			diag_raise();
 	}
 }
@@ -223,6 +225,13 @@ ck_constraint_new(struct ck_constraint_def *ck_constraint_def,
 	}
 	ck_constraint->def = NULL;
 	ck_constraint->stmt = NULL;
+
+	struct memtx_engine *memtx =
+		(struct memtx_engine *)engine_by_name("memtx");
+	assert(memtx != NULL);
+	bool is_recovery = memtx->state != MEMTX_OK;
+	ck_constraint->is_enabled = !is_recovery;
+
 	rlist_create(&ck_constraint->link);
 	struct Expr *expr =
 		sql_expr_compile(sql_get(), ck_constraint_def->expr_str,
@@ -269,3 +278,15 @@ space_ck_constraint_by_name(struct space *space, const char *name,
 	}
 	return NULL;
 }
+
+int
+space_ck_constraint_enable(struct space *space, const char *name,
+			   bool is_enabled)
+{
+	struct ck_constraint *ck =
+		space_ck_constraint_by_name(space, name, strlen(name));
+	if (ck == NULL)
+		return -1;
+	ck->is_enabled = is_enabled;
+	return 0;
+}
diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
index f26f77a38..4f2a3c20e 100644
--- a/src/box/ck_constraint.h
+++ b/src/box/ck_constraint.h
@@ -93,6 +93,12 @@ struct ck_constraint {
 	 * message when ck condition unsatisfied.
 	 */
 	struct sql_stmt *stmt;
+	/**
+	 * Whether the CK constraint object is enabled: the
+	 * checks wouldn't be performed when this flag is
+	 * set 'false'.
+	*/
+	bool is_enabled;
 	/**
 	 * Organize check constraint structs into linked list
 	 * with space::ck_constraint.
@@ -214,6 +220,19 @@ struct ck_constraint *
 space_ck_constraint_by_name(struct space *space, const char *name,
 			    uint32_t name_len);
 
+/**
+ * Find check constraint object in a given space by a given name
+ * and change is_enabled status.
+ * @param space The space to lookup check constraint.
+ * @param name The check constraint name.
+ * @param is_enabled The new is_enabled status for ck constraints.
+ * @return 0 if ck constraint is successfully found and new
+ *         is_enabled value is set, -1 otherwise.
+ */
+int
+space_ck_constraint_enable(struct space *space, const char *name,
+			   bool is_enabled);
+
 #if defined(__cplusplus)
 } /* extern "C" { */
 #endif
diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
index 98067f795..a9c05bb0d 100644
--- a/src/box/lua/schema.lua
+++ b/src/box/lua/schema.lua
@@ -29,6 +29,8 @@ ffi.cdef[[
     struct space *space_by_id(uint32_t id);
     extern uint32_t box_schema_version();
     void space_run_triggers(struct space *space, bool yesno);
+    void space_ck_constraint_enable(struct space *space, const char *name,
+                                    bool is_enabled);
     size_t space_bsize(struct space *space);
 
     typedef struct tuple box_tuple_t;
@@ -1759,6 +1761,17 @@ 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
+    if builtin.space_ck_constraint_enable(s, ck_constraint.name, yesno) ~= nil then
+        box.error(box.error.NO_SUCH_CONSTRAINT, tostring(ck_constraint.name))
+    end
+    ck_constraint.is_enabled = 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..2c686e818 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->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/memtx_engine.c b/src/box/memtx_engine.c
index f6a33282c..819571819 100644
--- a/src/box/memtx_engine.c
+++ b/src/box/memtx_engine.c
@@ -35,6 +35,7 @@
 #include <small/small.h>
 #include <small/mempool.h>
 
+#include "ck_constraint.h"
 #include "fiber.h"
 #include "errinj.h"
 #include "coio_file.h"
@@ -86,6 +87,18 @@ memtx_end_build_primary_key(struct space *space, void *param)
 	return 0;
 }
 
+static int
+space_run_ck_constraints(struct space *space, void *param)
+{
+	if (space_is_system(space))
+		return 0;
+	struct ck_constraint *ck;
+	rlist_foreach_entry(ck, &space->ck_constraint, link)
+		ck->is_enabled = (bool)param;
+	(void)trigger_run(&on_alter_space, space);
+	return 0;
+}
+
 /**
  * Secondary indexes are built in bulk after all data is
  * recovered. This function enables secondary keys on a space.
@@ -315,6 +328,8 @@ memtx_engine_end_recovery(struct engine *engine)
 		if (space_foreach(memtx_build_secondary_keys, memtx) != 0)
 			return -1;
 	}
+	if (space_foreach(space_run_ck_constraints, (void *)true) != 0)
+		unreachable();
 	xdir_collect_inprogress(&memtx->snap_dir);
 	return 0;
 }
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 50347bc3a..3f121226b 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -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,119 @@ s2:drop()
 physics_ck
 ---
 - space_id: <ID>
+  is_enabled: true
   name: physics
   expr: X > Y
 ...
 physics_ck:drop()
 ---
 ...
+--
+-- gh-4244: An ability to disable CK constraints
+-- Make shure that ck constraints are turning on and of 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 CHECK (a < 5) primary key);");
+---
+- row_count: 1
+...
+box.space.TEST:insert({10})
+---
+- error: 'Check constraint failed ''ck_unnamed_TEST_1'': a < 5'
+...
+box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(false)
+---
+...
+box.space.TEST:insert({11})
+---
+- [11]
+...
+box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(true)
+---
+...
+box.space.TEST:insert({12})
+---
+- error: 'Check constraint failed ''ck_unnamed_TEST_1'': a < 5'
+...
+-- Enshure that ck constraints are not validated during
+-- normal recovery. Now TEST space has a tuple {12} violating
+-- defined CK constraint.
+test_run:cmd("restart server default")
+box.space.TEST:select()
+---
+- - [11]
+...
+box.space.TEST:insert({12})
+---
+- error: 'Check constraint failed ''ck_unnamed_TEST_1'': a < 5'
+...
+box.execute("DROP TABLE test;")
+---
+- row_count: 1
+...
+-- Enshure that ck constraints are validated during
+-- force recovery.
+test_run:cmd('create server test with script = "xlog/force_recovery.lua"')
+---
+- true
+...
+test_run:cmd("start server test")
+---
+- true
+...
+test_run:cmd("switch test")
+---
+- true
+...
+engine = test_run:get_cfg('engine')
+---
+...
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+---
+- row_count: 0
+...
+box.execute("CREATE TABLE test(a  INT CHECK (a < 10) primary key);");
+---
+- row_count: 1
+...
+box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(false)
+---
+...
+box.space.TEST:insert({11})
+---
+- [11]
+...
+box.space.TEST:insert({2})
+---
+- [2]
+...
+test_run:cmd("restart server test")
+box.space.TEST:select()
+---
+- - [2]
+...
+box.execute("DROP TABLE test;")
+---
+- row_count: 1
+...
+test_run = require('test_run').new()
+---
+...
+test_run:cmd('switch default')
+---
+- true
+...
+test_run:cmd('stop server test')
+---
+- true
+...
 test_run:cmd("clear filter")
 ---
 - true
diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
index cde213f8b..9716647d0 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -234,4 +234,45 @@ s2:drop()
 physics_ck
 physics_ck:drop()
 
+--
+-- gh-4244: An ability to disable CK constraints
+-- Make shure that ck constraints are turning on and of with
+-- :enable configurator.
+--
+engine = test_run:get_cfg('engine')
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+box.execute("CREATE TABLE test(a  INT CHECK (a < 5) primary key);");
+box.space.TEST:insert({10})
+box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(false)
+box.space.TEST:insert({11})
+box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(true)
+box.space.TEST:insert({12})
+-- Enshure that ck constraints are not validated during
+-- normal recovery. Now TEST space has a tuple {12} violating
+-- defined CK constraint.
+test_run:cmd("restart server default")
+box.space.TEST:select()
+box.space.TEST:insert({12})
+box.execute("DROP TABLE test;")
+
+-- Enshure that ck constraints are validated during
+-- force recovery.
+test_run:cmd('create server test with script = "xlog/force_recovery.lua"')
+test_run:cmd("start server test")
+test_run:cmd("switch test")
+
+engine = test_run:get_cfg('engine')
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+box.execute("CREATE TABLE test(a  INT CHECK (a < 10) primary key);");
+box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(false)
+box.space.TEST:insert({11})
+box.space.TEST:insert({2})
+test_run:cmd("restart server test")
+box.space.TEST:select()
+box.execute("DROP TABLE test;")
+
+test_run = require('test_run').new()
+test_run:cmd('switch default')
+test_run:cmd('stop server test')
+
 test_run:cmd("clear filter")
-- 
2.23.0

[tarantool-patches] [PATCH v2 2/3] sql: disallow ck using non-persistent function

[Kirill Shcherbatov]

Each CK constraint object is a part of the database schema and
is restored during recovery. It is not possible if a CK
constraint uses some user-defined function inside. Thus we should
disallow non-persistent functions participate in ck constraints.

@TarantoolBot document
Title: disallow ck constraint using non-persistent function

Now CK constraints may use only persistent function and
predefined SQL built-in functions. In case of invalid definition
the error would be raised:

function myfunc(x) return x < 10 end
box.schema.func.create("MYFUNC", {exports = {'LUA', 'SQL'},
				  param_list = {'integer'}})
box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
---
- null
- 'Failed to create check constraint ''ck_unnamed_T6_1'': ck constraint
  could not
  use non-persistent function ''MYFUNC'''
---
 src/box/sql/resolve.c    | 10 ++++++++++
 test/sql/checks.result   | 43 ++++++++++++++++++++++++++++++++++++++--
 test/sql/checks.test.lua | 21 +++++++++++++++++++-
 3 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/src/box/sql/resolve.c b/src/box/sql/resolve.c
index 6f625dc18..0d6f146fb 100644
--- a/src/box/sql/resolve.c
+++ b/src/box/sql/resolve.c
@@ -653,6 +653,16 @@ resolveExprStep(Walker * pWalker, Expr * pExpr)
 				pExpr->iTable = func->def->name[0] == 'u' ?
 						8388608 : 125829120;
 			}
+			if ((pNC->ncFlags & NC_IsCheck) != 0 &&
+			    func->def->body == NULL &&
+			    func->def->language != FUNC_LANGUAGE_SQL_BUILTIN) {
+				diag_set(ClientError, ER_SQL_PARSER_GENERIC,
+					 "Check constraint can not invoke "
+					 "non-persistent function");
+				pParse->is_aborted = true;
+				pNC->nErr++;
+				return WRC_Abort;
+			}
 			assert(!func->def->is_deterministic ||
 			       (pNC->ncFlags & NC_IdxExpr) == 0);
 			if (func->def->is_deterministic)
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 3f121226b..7939d46df 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -830,9 +830,48 @@ test_run:cmd('switch default')
 ---
 - true
 ...
-test_run:cmd('stop server test')
+--
+-- gh-4176: Can't recover if check constraint involves function.
+-- Make sure that non-persistent functions can't participate in
+-- check constraints, since after instance reboot they disappear
+-- and check constraint can't be created.
+--
+function myfunc(x) return x < 10 end
+---
+...
+box.schema.func.create("MYFUNC", {exports = {'LUA', 'SQL'}, param_list = {'integer'}})
+---
+...
+box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
+---
+- null
+- 'Failed to create check constraint ''ck_unnamed_T6_1'': Check constraint can not
+  invoke non-persistent function'
+...
+box.func.MYFUNC:drop()
+---
+...
+box.schema.func.create("MYFUNC", {exports = {'LUA', 'SQL'}, param_list = {'integer'}, body = "function(x) return x < 10 end"})
+---
+...
+box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
+---
+- row_count: 1
+...
+box.space.T6:insert({11})
+---
+- error: 'Check constraint failed ''ck_unnamed_T6_1'': myfunc(a)'
+...
+test_run:cmd("restart server default")
+box.space.T6:insert({11})
+---
+- error: 'Check constraint failed ''ck_unnamed_T6_1'': myfunc(a)'
+...
+box.space.T6:drop()
+---
+...
+box.func.MYFUNC:drop()
 ---
-- true
 ...
 test_run:cmd("clear filter")
 ---
diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
index 9716647d0..051c9ae38 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -273,6 +273,25 @@ box.execute("DROP TABLE test;")
 
 test_run = require('test_run').new()
 test_run:cmd('switch default')
-test_run:cmd('stop server test')
+
+--
+-- gh-4176: Can't recover if check constraint involves function.
+-- Make sure that non-persistent functions can't participate in
+-- check constraints, since after instance reboot they disappear
+-- and check constraint can't be created.
+--
+function myfunc(x) return x < 10 end
+box.schema.func.create("MYFUNC", {exports = {'LUA', 'SQL'}, param_list = {'integer'}})
+box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
+box.func.MYFUNC:drop()
+
+box.schema.func.create("MYFUNC", {exports = {'LUA', 'SQL'}, param_list = {'integer'}, body = "function(x) return x < 10 end"})
+box.execute("CREATE TABLE t6(a  INT CHECK (myfunc(a)) primary key);");
+box.space.T6:insert({11})
+test_run:cmd("restart server default")
+box.space.T6:insert({11})
+
+box.space.T6:drop()
+box.func.MYFUNC:drop()
 
 test_run:cmd("clear filter")
-- 
2.23.0

[tarantool-patches] [PATCH v2 3/3] sql: use name instead of function pointer for UDF

[Kirill Shcherbatov]

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
had been deleted to the moment of the VDBE code execution.
In particular case this may happen with CK constraints that
refer to a persistent function.
---
 src/box/sql/expr.c       | 17 ++++++++++++-----
 src/box/sql/vdbe.c       | 17 +++++++++++------
 src/box/sql/vdbe.h       |  1 +
 test/sql/checks.result   | 10 +++++++---
 test/sql/checks.test.lua |  3 ++-
 5 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
index 6d5ad2a27..04c9393be 100644
--- a/src/box/sql/expr.c
+++ b/src/box/sql/expr.c
@@ -4136,11 +4136,18 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
 				sqlVdbeAddOp4(v, OP_CollSeq, 0, 0, 0,
 						  (char *)coll, P4_COLLSEQ);
 			}
-			int op = func->def->language ==
-				 FUNC_LANGUAGE_SQL_BUILTIN ?
-				 OP_BuiltinFunction0 : OP_Function;
-			sqlVdbeAddOp4(v, op, constMask, r1, target,
-				      (char *)func, P4_FUNC);
+			if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
+				sqlVdbeAddOp4(v, OP_BuiltinFunction0, constMask,
+					      r1, target, (char *)func,
+					      P4_FUNC);
+			} else {
+				sqlVdbeAddOp4(v, OP_Function,
+					      func->def->name_len, r1, target,
+					      sqlDbStrNDup(sql_get(),
+							   func->def->name,
+							   func->def->name_len),
+					      P4_DYNAMIC);
+			}
 			sqlVdbeChangeP5(v, (u8) nFarg);
 			if (nFarg && constMask == 0) {
 				sqlReleaseTempRange(pParse, r1, nFarg);
diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
index 02e16da19..4c6245348 100644
--- a/src/box/sql/vdbe.c
+++ b/src/box/sql/vdbe.c
@@ -1795,16 +1795,21 @@ case OP_BuiltinFunction: {
 	break;
 }
 
-/* Opcode: Function * P2 P3 P4 P5
+/* Opcode: Function P1 P2 P3 P4 P5
  * Synopsis: r[P3]=func(r[P2@P5])
  *
- * Invoke a user function (P4 is a pointer to a function object
- * that defines the function) with P5 arguments taken from
- * register P2 and successors. The result of the function is
- * stored in register P3.
+ * Invoke a user function (P4 is a name of the function while P1
+ * is a function name length) with P5 arguments taken from register P2 and
+ * successors. The result of the function is stored in
+ * register P3.
  */
 case OP_Function: {
-	struct func *func = pOp->p4.func;
+	assert(pOp->p4type == P4_DYNAMIC);
+	struct func *func = func_by_name(pOp->p4.z, pOp->p1);
+	if (unlikely(func == NULL)) {
+		diag_set(ClientError, ER_NO_SUCH_FUNCTION, pOp->p4.z);
+		goto abort_due_to_error;
+	}
 	int argc = pOp->p5;
 	struct Mem *argv = &aMem[pOp->p2];
 	struct port args, ret;
diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
index 29ff99867..e57d7f979 100644
--- a/src/box/sql/vdbe.h
+++ b/src/box/sql/vdbe.h
@@ -128,6 +128,7 @@ struct SubProgram {
 #define P4_COLLSEQ  (-3)	/* P4 is a pointer to a CollSeq structure */
 /** P4 is a pointer to a func structure. */
 #define P4_FUNC     (-4)
+/** P4 is a function identifier. */
 #define P4_MEM      (-7)	/* P4 is a pointer to a Mem*    structure */
 #define P4_TRANSIENT  0		/* P4 is a pointer to a transient string */
 #define P4_REAL     (-9)	/* P4 is a 64-bit floating point value */
diff --git a/test/sql/checks.result b/test/sql/checks.result
index 7939d46df..02951b3cf 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -867,12 +867,16 @@ box.space.T6:insert({11})
 ---
 - error: 'Check constraint failed ''ck_unnamed_T6_1'': myfunc(a)'
 ...
+box.func.MYFUNC:drop()
+---
+...
+box.space.T6:insert({11})
+---
+- error: Function 'MYFUNC' does not exist
+...
 box.space.T6:drop()
 ---
 ...
-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 051c9ae38..d64f3305a 100644
--- a/test/sql/checks.test.lua
+++ b/test/sql/checks.test.lua
@@ -291,7 +291,8 @@ box.space.T6:insert({11})
 test_run:cmd("restart server default")
 box.space.T6:insert({11})
 
+box.func.MYFUNC:drop()
+box.space.T6:insert({11})
 box.space.T6:drop()
-box.func.MYFUNC:drop()
 
 test_run:cmd("clear filter")
-- 
2.23.0

[tarantool-patches] Re: [PATCH v2 2/3] sql: disallow ck using non-persistent function

[Nikita Pettik]

On 12 Sep 11:06, Kirill Shcherbatov wrote:
> Each CK constraint object is a part of the database schema and
> is restored during recovery. It is not possible if a CK
> constraint uses some user-defined function inside. Thus we should
> disallow non-persistent functions participate in ck constraints.

This patch should be dropped taking into consideration last patch
in series: it implements fix for special case of the problem solved
in the next patch.
 
> diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
> index 9716647d0..051c9ae38 100644
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> @@ -273,6 +273,25 @@ box.execute("DROP TABLE test;")
>  
>  test_run = require('test_run').new()
>  test_run:cmd('switch default')
> -test_run:cmd('stop server test')
> +
> +--
> +-- gh-4176: Can't recover if check constraint involves function.
> +-- Make sure that non-persistent functions can't participate in
> +-- check constraints, since after instance reboot they disappear
> +-- and check constraint can't be created.
> +--
> +function myfunc(x) return x < 10 end
> +box.schema.func.create("MYFUNC", {exports = {'LUA', 'SQL'}, param_list = {'integer'}})

Nit: name shoul be lowercased I guess.
 

[tarantool-patches] Re: [PATCH v2 3/3] sql: use name instead of function pointer for UDF

[Nikita Pettik]

On 12 Sep 11:06, Kirill Shcherbatov wrote:
> 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
> had been deleted to the moment of the VDBE code execution.

Nit: has been deleted.

> In particular case this may happen with CK constraints that
> refer to a persistent function.

-> refer to a deleted persistent function (?).

> ---
>  src/box/sql/expr.c       | 17 ++++++++++++-----
>  src/box/sql/vdbe.c       | 17 +++++++++++------
>  src/box/sql/vdbe.h       |  1 +
>  test/sql/checks.result   | 10 +++++++---
>  test/sql/checks.test.lua |  3 ++-
>  5 files changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c
> index 6d5ad2a27..04c9393be 100644
> --- a/src/box/sql/expr.c
> +++ b/src/box/sql/expr.c
> @@ -4136,11 +4136,18 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target)
>  				sqlVdbeAddOp4(v, OP_CollSeq, 0, 0, 0,
>  						  (char *)coll, P4_COLLSEQ);
>  			}
> -			int op = func->def->language ==
> -				 FUNC_LANGUAGE_SQL_BUILTIN ?
> -				 OP_BuiltinFunction0 : OP_Function;
> -			sqlVdbeAddOp4(v, op, constMask, r1, target,
> -				      (char *)func, P4_FUNC);
> +			if (func->def->language == FUNC_LANGUAGE_SQL_BUILTIN) {
> +				sqlVdbeAddOp4(v, OP_BuiltinFunction0, constMask,
> +					      r1, target, (char *)func,
> +					      P4_FUNC);
> +			} else {
> +				sqlVdbeAddOp4(v, OP_Function,
> +					      func->def->name_len, r1, target,

Why not null-terminate function's name? Then you don't have to deal
with name's length.

> +					      sqlDbStrNDup(sql_get(),

Nit: you can use pParse->db.

> diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> index 02e16da19..4c6245348 100644
> --- a/src/box/sql/vdbe.c
> +++ b/src/box/sql/vdbe.c
> @@ -1795,16 +1795,21 @@ case OP_BuiltinFunction: {
>  	break;
>  }
>  
> -/* Opcode: Function * P2 P3 P4 P5
> +/* Opcode: Function P1 P2 P3 P4 P5
>   * Synopsis: r[P3]=func(r[P2@P5])
>   *
> - * Invoke a user function (P4 is a pointer to a function object
> - * that defines the function) with P5 arguments taken from
> - * register P2 and successors. The result of the function is
> - * stored in register P3.
> + * Invoke a user function (P4 is a name of the function while P1
> + * is a function name length) with P5 arguments taken from register P2 and

Nit: out of 72 :)

> + * successors. The result of the function is stored in
> + * register P3.
>   */
>  case OP_Function: {
> -	struct func *func = pOp->p4.func;
> +	assert(pOp->p4type == P4_DYNAMIC);
> +	struct func *func = func_by_name(pOp->p4.z, pOp->p1);
> +	if (unlikely(func == NULL)) {
> +		diag_set(ClientError, ER_NO_SUCH_FUNCTION, pOp->p4.z);
> +		goto abort_due_to_error;
> +	}
>  	int argc = pOp->p5;
>  	struct Mem *argv = &aMem[pOp->p2];
>  	struct port args, ret;
> diff --git a/src/box/sql/vdbe.h b/src/box/sql/vdbe.h
> index 29ff99867..e57d7f979 100644
> --- a/src/box/sql/vdbe.h
> +++ b/src/box/sql/vdbe.h
> @@ -128,6 +128,7 @@ struct SubProgram {
>  #define P4_COLLSEQ  (-3)	/* P4 is a pointer to a CollSeq structure */
>  /** P4 is a pointer to a func structure. */
>  #define P4_FUNC     (-4)
> +/** P4 is a function identifier. */

Nit: redundant diff.

[tarantool-patches] Re: [PATCH v2 1/3] box: an ability to disable CK constraints

[Nikita Pettik]

On 12 Sep 11:06, Kirill Shcherbatov wrote:
> @TarantoolBot document
> Title: an ability to disable CK constraints
> 
> Now it is possible to disable and enable ck constraints.
> This option is not persistent. 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.

Please, add reasoning. Please, explain why do we need per constraint
options? Why not disable them all at once? How to disable constraint
in SQL?

What is more, I'd split this patch into two: first one introduces
'is_enabled' property; second one provides check execution during
force recovery.

> 
> 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
> x.space.T6:insert({11})
> -- passed
> 
> Closes #4244
> ---
>  extra/exports            |   1 +
>  src/box/ck_constraint.c  |  23 +++++++-
>  src/box/ck_constraint.h  |  19 +++++++
>  src/box/lua/schema.lua   |  13 +++++
>  src/box/lua/space.cc     |   3 ++
>  src/box/memtx_engine.c   |  15 ++++++
>  test/sql/checks.result   | 112 +++++++++++++++++++++++++++++++++++++++
>  test/sql/checks.test.lua |  41 ++++++++++++++
>  8 files changed, 226 insertions(+), 1 deletion(-)
> 
> diff --git a/extra/exports b/extra/exports
> index 7b84a1452..ecc7d102b 100644
> --- a/extra/exports
> +++ b/extra/exports
> @@ -78,6 +78,7 @@ tarantool_exit
>  log_pid
>  space_by_id
>  space_run_triggers
> +space_ck_constraint_enable
>  space_bsize
>  box_schema_version
>  
> diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
> index 1cde27022..f65715096 100644
> --- a/src/box/ck_constraint.c
> +++ b/src/box/ck_constraint.c
> @@ -28,6 +28,7 @@
>   * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>   * SUCH DAMAGE.
>   */
> +#include "memtx_engine.h"
>  #include "box/session.h"
>  #include "bind.h"
>  #include "ck_constraint.h"
> @@ -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->is_enabled &&
> +		    ck_constraint_program_run(ck_constraint, field_ref) != 0)
>  			diag_raise();
>  	}
>  }
> @@ -223,6 +225,13 @@ ck_constraint_new(struct ck_constraint_def *ck_constraint_def,
>  	}
>  	ck_constraint->def = NULL;
>  	ck_constraint->stmt = NULL;
> +
> +	struct memtx_engine *memtx =
> +		(struct memtx_engine *)engine_by_name("memtx");
> +	assert(memtx != NULL);

Wait a second, how it is supposed to work with vinyl?

> +	bool is_recovery = memtx->state != MEMTX_OK;

Supply this code with comment.

> +	ck_constraint->is_enabled = !is_recovery;
> +
>  	rlist_create(&ck_constraint->link);
>  	struct Expr *expr =
>  		sql_expr_compile(sql_get(), ck_constraint_def->expr_str,
> @@ -269,3 +278,15 @@ space_ck_constraint_by_name(struct space *space, const char *name,
>  	}
>  	return NULL;
>  }
> +
> +int
> +space_ck_constraint_enable(struct space *space, const char *name,
> +			   bool is_enabled)
> +{

Nit: method is calles "enable", but it can also disable trigger.
I'd rename it to "_set_status"/"set_state" etc

> +	struct ck_constraint *ck =
> +		space_ck_constraint_by_name(space, name, strlen(name));
> +	if (ck == NULL)
> +		return -1;
> +	ck->is_enabled = is_enabled;
> +	return 0;
> +}
> diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
> index f26f77a38..4f2a3c20e 100644
> --- a/src/box/ck_constraint.h
> +++ b/src/box/ck_constraint.h
> @@ -93,6 +93,12 @@ struct ck_constraint {
>  	 * message when ck condition unsatisfied.
>  	 */
>  	struct sql_stmt *stmt;
> +	/**
> +	 * Whether the CK constraint object is enabled: the
> +	 * checks wouldn't be performed when this flag is
> +	 * set 'false'.

->

/**
 * Per constraint option regulating its execution: it is disabled
 * (set to false) contraint won't be fired. Note that during ordinary
 * recovery they are turned off; during force recovery they are
 * enabled.
 */

> +	*/
> +	bool is_enabled;
>  	/**
>  	 * Organize check constraint structs into linked list
>  	 * with space::ck_constraint.
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index d0a7e7815..2c686e818 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->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/memtx_engine.c b/src/box/memtx_engine.c
> index f6a33282c..819571819 100644
> --- a/src/box/memtx_engine.c
> +++ b/src/box/memtx_engine.c
> @@ -35,6 +35,7 @@
>  #include <small/small.h>
>  #include <small/mempool.h>
>  
> +#include "ck_constraint.h"
>  #include "fiber.h"
>  #include "errinj.h"
>  #include "coio_file.h"
> @@ -86,6 +87,18 @@ memtx_end_build_primary_key(struct space *space, void *param)
>  	return 0;
>  }
>  
> +static int
> +space_run_ck_constraints(struct space *space, void *param)
> +{
> +	if (space_is_system(space))
> +		return 0;
> +	struct ck_constraint *ck;
> +	rlist_foreach_entry(ck, &space->ck_constraint, link)
> +		ck->is_enabled = (bool)param;
> +	(void)trigger_run(&on_alter_space, space);
> +	return 0;
> +}
> +
>  /**
>   * Secondary indexes are built in bulk after all data is
>   * recovered. This function enables secondary keys on a space.
> @@ -315,6 +328,8 @@ memtx_engine_end_recovery(struct engine *engine)
>  		if (space_foreach(memtx_build_secondary_keys, memtx) != 0)
>  			return -1;
>  	}
> +	if (space_foreach(space_run_ck_constraints, (void *)true) != 0)
> +		unreachable();

What does this change do? Why do you need param if it is always true?
Is space_is_system() check vital?

> diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
> index cde213f8b..9716647d0 100644
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> @@ -234,4 +234,45 @@ s2:drop()
>  physics_ck
>  physics_ck:drop()
>  
> +--
> +-- gh-4244: An ability to disable CK constraints

Nit: add an ability ...

> +-- Make shure that ck constraints are turning on and of with

-> sure; are turned on/off

> +-- :enable configurator.
> +--
> +engine = test_run:get_cfg('engine')
> +box.execute('pragma sql_default_engine=\''..engine..'\'')
> +box.execute("CREATE TABLE test(a  INT CHECK (a < 5) primary key);");
> +box.space.TEST:insert({10})
> +box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(false)
> +box.space.TEST:insert({11})
> +box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(true)
> +box.space.TEST:insert({12})
> +-- Enshure that ck constraints are not validated during

-> Ensure

> +-- normal recovery. Now TEST space has a tuple {12} violating
> +-- defined CK constraint.
> +test_run:cmd("restart server default")
> +box.space.TEST:select()
> +box.space.TEST:insert({12})
> +box.execute("DROP TABLE test;")
> +
> +-- Enshure that ck constraints are validated during
> +-- force recovery.
> +test_run:cmd('create server test with script = "xlog/force_recovery.lua"')
> +test_run:cmd("start server test")
> +test_run:cmd("switch test")

Could you make result of test be clearer? From .result file it is not
obvious that check verification failed.

> +engine = test_run:get_cfg('engine')
> +box.execute('pragma sql_default_engine=\''..engine..'\'')
> +box.execute("CREATE TABLE test(a  INT CHECK (a < 10) primary key);");
> +box.space.TEST.ck_constraint.ck_unnamed_TEST_1:enable(false)
> +box.space.TEST:insert({11})
> +box.space.TEST:insert({2})
> +test_run:cmd("restart server test")
> +box.space.TEST:select()
> +box.execute("DROP TABLE test;")
> +
> +test_run = require('test_run').new()
> +test_run:cmd('switch default')
> +test_run:cmd('stop server test')
> +
>  test_run:cmd("clear filter")

[tarantool-patches] Re: [PATCH v2 1/3] box: an ability to disable CK constraints

[Kirill Shcherbatov]

>> +	struct memtx_engine *memtx =
>> +		(struct memtx_engine *)engine_by_name("memtx");
>> +	assert(memtx != NULL);
> 
> Wait a second, how it is supposed to work with vinyl?
All system spaces use memtx engine. Thus looking for memtx engine state is an
acceptable way to determine, whether Tarantool loaded.

> Nit: method is calles "enable", but it can also disable trigger.
> I'd rename it to "_set_status"/"set_state" etc
I don't like both your variants. "enable" has been proposed by Kostya.

>> +	if (space_foreach(space_run_ck_constraints, (void *)true) != 0)
>> +		unreachable();
> 
> What does this change do? Why do you need param if it is always true?
In the first patch version I used to call this function space_run_ck_constraints with
false and true in two places to enable/disable all check constraint. In my opinion
hardcoding true in smth like space_enable_ck_constraints is not really good idea.

> Is space_is_system() check vital?
It may be omitted. Initially I assumed that system spaces cold not have ck constraints
but actually it is not so now, they could.