[tarantool-patches] Re: [PATCH v4 4/4] box: user-friendly interface to manage ck constraints

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch! See 6 comments below.

On 16/05/2019 16:56, Kirill Shcherbatov wrote:
> Closes #3691
> 
> @TarantoolBot document
> Title: check constraint for Lua space
> 
> The check constraint is a type of integrity constraint which
> specifies a requirement that must be met by tuple before it
> is inserted into space. The constraint result must be predictable.
> Expression in check constraint must be <boolean value expression>
> I.e. return boolean result.
> 
> Now it is possible to create ck constraints only for empty space
> having format. Constraint expression is a string that defines
> relations between top-level tuple fields.
> Take into account that all names are converted to an uppercase
> before resolve(like SQL does), use \" sign for names of fields
> that were created not with SQL.
> 
> The check constraints are fired on insertion to the Lua space
> together with Lua space triggers. The execution order of
> ck constraints checks and space triggers follows their creation
> sequence.
> 
> Note: this patch changes the CK constraints execution order for
> SQL. Previously check of CK constraints integrity was fired before
> tuple is formed; meanwhile now they are implemented as NoSQL before
> replace triggers, which are fired right before tuple insertion.
> In turn, type casts are performed earlier than msgpack
> serialization. You should be careful with functions that use
> field types in your check constrains (like typeof()).
> 
> Consider following situation:
> ```
>  box.execute("CREATE TABLE t2(id  INT primary key,
>                               x INTEGER CHECK (x > 1));")
>  box.execute("INSERT INTO t2 VALUES(3, 1.1)")
> ```
> the last operation would fail because 1.1 is silently
> cast to integer 1 which is not greater than 1.

1. I do not see that comment on the branch.

> 
> To create a new CK constraint for a space, use
> space:create_check_constraint method. All space constraints are
> shown in space.ck_constraint table. To drop ck constraint,
> use :drop method.
> 
> Example:
> ```
> s1 = box.schema.create_space('test1')
> pk = s1:create_index('pk')
> ck = s1:create_check_constraint('physics', 'X < Y')
> s1:insert({2, 1}) -- fail
> ck:drop()
> ```
> ---
> diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua
> index e01f500e6..a30d6aa5b 100644
> --- a/src/box/lua/schema.lua
> +++ b/src/box/lua/schema.lua> @@ -1579,10 +1597,17 @@ end
>  space_mt.frommap = box.internal.space.frommap
>  space_mt.__index = space_mt
>  
> +local ck_constraint_mt = {}
> +ck_constraint_mt.drop = function(ck_constraint)
> +    check_ck_constraint_arg(ck_constraint, 'drop')
> +    box.space._ck_constraint:delete({ck_constraint.name, ck_constraint.space_id})
> +end
> +
>  box.schema.index_mt = base_index_mt
>  box.schema.memtx_index_mt = memtx_index_mt
>  box.schema.vinyl_index_mt = vinyl_index_mt
>  box.schema.space_mt = space_mt
> +box.schema.ck_constraint_mt = ck_constraint_mt

2. Please, do not expose ck_constraint_mt.
box.schema.<space/index>_mt are public and documented
metatables to replace certain DML DQL operations with
user's one.

Checks does not provide methods except drop, so it
makes no sense to make their metatable configurable.

It means, that you don't need to wrap checks below.

>  
>  --
>  -- Wrap a global space/index metatable into a space/index local
> @@ -1628,6 +1653,14 @@ function box.schema.space.bless(space)
>              end
>          end
>      end
> +    if type(space.ck_constraint) == 'table' and space.enabled then
> +        for j, ck_constraint in pairs(space.ck_constraint) do
> +            if type(j) == 'string' then
> +                setmetatable(ck_constraint,
> +                             wrap_schema_object_mt('ck_constraint_mt'))
> +            end
> +        end
> +    end
>  end
>  
>  local sequence_mt = {}
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index ce4287bba..9cec1cbde 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -147,6 +148,66 @@ lbox_space_before_replace(struct lua_State *L)
>  				  lbox_push_txn_stmt, lbox_pop_txn_stmt);
>  }
>  
> +/**
> + * Make ck_constraints available in Lua, via ck_constraint[]
> + * array.
> + * Returns a new table representing a space on top of the Lua
> + * stack.

3. Representing a space? How lbox_ck_constraint can return a space?
As I see, it does not return anything. It just modifies an existing
space table.

> + */
> +static void
> +lbox_ck_constraint(struct lua_State *L, struct space *space, int i)

4. lbox_ck_constraint_what? If a function is not a getter, it should
have a verb saying what the function does.

What is 'i' argument? Index of space table?

> +{
> +	lua_getfield(L, i, "ck_constraint");
> +	if (lua_isnil(L, -1)) {
> +		lua_pop(L, 1);
> +		lua_pushstring(L, "ck_constraint");
> +		lua_newtable(L);
> +		lua_settable(L, i);
> +		lua_getfield(L, i, "ck_constraint");
> +	} else {
> +		lua_pushnil(L);
> +		while (lua_next(L, -2) != 0) {
> +			size_t name_len;
> +			const char *name = lua_tolstring(L, -2, &name_len);
> +			/*
> +			 * Remove ck_constraint only if it was
> +			 * deleted.
> +			 */
> +			if (space_ck_constraint_by_name(space, name,
> +					(uint32_t)name_len) == NULL) {
> +				lua_pushlstring(L, name, name_len);
> +				lua_pushnil(L);
> +				lua_settable(L, -5);
> +			}
> +			lua_pop(L, 1);
> +		}
> +	}
> +	struct ck_constraint *ck_constraint = NULL;
> +	rlist_foreach_entry(ck_constraint, &space->ck_constraint, link) {
> +		lua_getfield(L, i, ck_constraint->def->name);
> +		if (lua_isnil(L, -1)) {
> +			lua_pop(L, 1);
> +			lua_pushstring(L, ck_constraint->def->name);
> +			lua_newtable(L);
> +			lua_settable(L, -3);
> +			lua_getfield(L, -1, ck_constraint->def->name);
> +			assert(!lua_isnil(L, -1));
> +		}
> +
> +		lua_pushstring(L, ck_constraint->def->name);
> +		lua_setfield(L, -2, "name");
> +
> +		lua_pushnumber(L, space->def->id);
> +		lua_setfield(L, -2, "space_id");
> +
> +		lua_pushstring(L, ck_constraint->def->expr_str);
> +		lua_setfield(L, -2, "expr_str");
> +
> +		lua_setfield(L, -2, ck_constraint->def->name);
> +	}
> +	lua_pop(L, 1);
> +}
> +
>  /**
>   * Make a single space available in Lua,
>   * via box.space[] array.
> diff --git a/test/sql/checks.result b/test/sql/checks.result
> index 04f58c90d..d87106c42 100644
> --- a/test/sql/checks.result
> +++ b/test/sql/checks.result
> @@ -513,6 +513,96 @@ s:insert(s:frommap({X1 = 666, X65 = 666, X63 = 1}))
>  s:drop()
>  ---
>  ...
> +--
> +-- Test ck constraints LUA integration.
> +--
> +s1 = box.schema.create_space('test1')
> +---
> +...
> +_ = s1:create_index('pk')
> +---
> +...
> +s1:format({{name='X', type='any'}, {name='Y', type='integer'}})
> +---
> +...
> +s2 = box.schema.create_space('test2')
> +---
> +...
> +_ = s2:create_index('pk')
> +---
> +...
> +s2:format({{name='X', type='any'}, {name='Y', type='integer'}})
> +---
> +...
> +_ = s1:create_check_constraint('physics', 'X < Y')
> +---
> +...
> +_ = s1:create_check_constraint('greater', 'X > 20')
> +---
> +...
> +_ = s2:create_check_constraint('physics', 'X > Y')
> +---
> +...
> +_ = s2:create_check_constraint('greater', 'X > 20')
> +---
> +...
> +s1.ck_constraint.physics ~= nil
> +---
> +- true

5. Why do you avoid serialization? What will happen,
if I will write

    tarantool> s1.ck_constraint.physics

?

> +...
> +s1.ck_constraint.greater ~= nil
> +---
> +- true
> +...
> +s2.ck_constraint.physics ~= nil
> +---
> +- true
> +...
> +s2.ck_constraint.greater ~= nil
> +---
> +- true
> +...
> +s1:insert({2, 1})
> +---
> +- error: 'Check constraint failed ''greater'': X > 20'
> +...
> +s1:insert({21, 20})
> +---
> +- error: 'Check constraint failed ''physics'': X < Y'
> +...
> +s2:insert({1, 2})
> +---
> +- error: 'Check constraint failed ''greater'': X > 20'
> +...
> +s2:insert({21, 22})
> +---
> +- error: 'Check constraint failed ''physics'': X > Y'
> +...
> +s2.ck_constraint.greater:drop()
> +---
> +...
> +s2.ck_constraint.physics ~= nil
> +---
> +- true
> +...
> +s2.ck_constraint.greater == nil
> +---
> +- true
> +...
> +s1:insert({2, 1})
> +---
> +- error: 'Check constraint failed ''greater'': X > 20'
> +...
> +s2:insert({1, 2})
> +---
> +- error: 'Check constraint failed ''physics'': X > Y'
> +...
> +s1:drop()
> +---
> +...
> +s2:drop()

6. The test does not check, that after a constraint is
dropped, a tuple can be inserted violating the dropped
constraint.

Also, I do not see a test, that a user can't insert
constraints with the same names.

> +---
> +...
>  test_run:cmd("clear filter")
>  ---
>  - true