[tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter

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

> On 16 Apr 2019, at 16:51, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
> To perform ck constraints tests before insert or update space operation,
> we use precompiled VDBE machine is associated with each ck constraint,
> that is executed in on_replace trigger. Each ck constraint VDBE code is
> consists of
> 1) prologue code that maps new(or updated) tuple fields via bindings,
> 2) ck constraint code is generated by CK constraint AST.
> In case of ck constraint error the transaction is aborted and ck
> constraint error is handled as diag message.
> Each ck constraint use own on_replace trigger.
> 
> Closes #3691

One unpleasant “feature” that I’ve noticed:

create table t4 (id int primary key check(id > id*id))
insert into t4 values(0.5)
- error: 'Failed to execute SQL statement: Check constraint failed ''CK_CONSTRAINT_1_T4'':
    id > id*id’

Conversion to INT occurs before tuples reaches on_replace trigger.
We should discuss what to do with that. I guess you already raised
this question when we were talking about typeof() functions inside
check constraint.

Also, I’ve noticed that check expression bytecode is not optimized.
For example:

create table t4 (id int primary key check(id > 6+7*3-1*31))
insert into t4 values(1)

VDBE Program Listing:
 102>    0 Init             0    7    0               00 Start at 7
 102>    1 Variable         1    1    0               00 r[1]=parameter(1,)
 102>    2 Noop             0    0    0               00 BEGIN: ck constraint CK_CONSTRAINT_1_T4 test
 102>    3 Gt               3    6    1 ({type = binary}) 13 if r[1]>r[3] goto 6
 102>    4 Halt            21    2    0 Check constraint failed 'CK_CONSTRAINT_1_T4': id > 6+7*3-1*31 C3 
 102>    5 Noop             0    0    0               00 END: ck constraint CK_CONSTRAINT_1_T4 test
 102>    6 Halt             0    0    0               00 
 102>    7 Integer          6    4    0               00 r[4]=6
 102>    8 Integer          7    6    0               00 r[6]=7
 102>    9 Integer          3    7    0               00 r[7]=3
 102>   10 Multiply         7    6    5               00 r[5]=r[7]*r[6]
 102>   11 Add              5    4    2               00 r[2]=r[5]+r[4]
 102>   12 Integer          1    4    0               00 r[4]=1
 102>   13 Integer         31    7    0               00 r[7]=31
 102>   14 Multiply         7    4    5               00 r[5]=r[7]*r[4]
 102>   15 Subtract         5    2    3               00 r[3]=r[2]-r[5]
 102>   16 Goto             0    1    0               00 

Looks extremely inefficient. We could once traverse tree and avoid
these calculations on each insertion.

> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index e96d502c9..57662df11 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> 
> diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
> index daeebb78b..26d18330b 100644
> --- a/src/box/ck_constraint.c
> +++ b/src/box/ck_constraint.c
> @@ -28,11 +28,15 @@
>  * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
>  * SUCH DAMAGE.
>  */
> +#include "bind.h"
> #include "ck_constraint.h"
> #include "errcode.h"
> -#include "small/rlist.h"
> +#include "schema.h"
> +#include "session.h"
> #include "sql.h"
> #include "sql/sqlInt.h"
> +#include "sql/vdbeInt.h"
> +#include "tuple.h"
> 
> /**
>  * Resolve space_def references for check constraint via AST
> @@ -55,6 +59,146 @@ ck_constraint_resolve_space_def(struct Expr *expr,
> 
> +static struct sql_stmt *
> +ck_constraint_program_compile(struct ck_constraint_def *ck_constraint_def,
> +			      struct Expr *expr, struct space_def *space_def,
> +			      int *new_tuple_var)
> 
> +	/* Compile VDBE with default sql parameters. */
> +	struct session *user_session = current_session();
> +	uint32_t sql_flags = user_session->sql_flags;
> +	user_session->sql_flags = default_flags;
> +	/*
> +	 * Generate a prologue code that introduce variables to
> +	 * bind tuple fields there before execution.
> +	 */
> +	uint32_t field_count = space_def->field_count;
> +	int new_tuple_reg = sqlGetTempRange(&parser, field_count);

Nit: new_tuple_reg IMHO is very general name. Mb it would be better
to call it like “bind_tuple_reg”?

> +	*new_tuple_var = parser.nVar + 1;

Won’t it be always 1? You’ve just created parser and no byte code is generated yet,
so how it could be different from 1? sqlGetTempRange() doesn’t affect nVar AFAIU.

> +	struct Expr bind = {.op = TK_VARIABLE, .u.zToken = "?"};
> +	for (uint32_t i = 0; i < field_count; i++) {
> +		sqlExprAssignVarNumber(&parser, &bind, 1);
> +		sqlExprCodeTarget(&parser, &bind, new_tuple_reg + i);

You don’t need to call this huge function: all you need is this:
sqlVdbeAddOp2(v, OP_Variable, bind.iColumn, new_tuple_reg + i);

> +	}
> +	/* Generate ck constraint test code. */
> +	vdbe_emit_ck_constraint(&parser, expr, ck_constraint_def->name,
> +				ck_constraint_def->expr_str, new_tuple_reg);
> +
> +	/* Clean-up and restore user-defined sql context. */
> +	bool is_error = parser.is_aborted;
> +	sql_finish_coding(&parser);

Can we just add OP_Halt + sqlVdbeMakeReady() ?

> +/**
> + * Perform ck constraint test on new tuple data new_tuple
> + * before insert or replace in space space_def.
> + * @param ck_constraint Check constraint to perform.
> + * @param space_def The space definition of the space this check
> + *                  constraint is constructed for.
> + * @param new_tuple The tuple to be inserted in space.
> + * @retval 0 if check constraint test is passed, -1 otherwise.
> + */
> +static int
> +ck_constraint_program_run(struct ck_constraint *ck_constraint,
> +			  const char *new_tuple)
> +{
> +	assert(new_tuple != NULL);
> +	/*
> +	 * Prepare parameters for checks->stmt execution:
> +	 * Map new tuple fields to Vdbe memory variables in range:
> +	 * [new_tuple_var, new_tuple_var + field_count]
> +	 */
> +	mp_decode_array(&new_tuple);
> +	struct space *space = space_by_id(ck_constraint->space_id);
> +	assert(space != NULL);
> +	for (uint32_t i = 0; i < space->def->field_count; i++) {

Mm, this code differs from one on brach:

uint32_t field_count =
       MIN(tuple_field_count, space->def->field_count);

Comment (in code) please this part. How space field count could
be less than tuple field count?

> diff --git a/test/sql-tap/check.test.lua b/test/sql-tap/check.test.lua
> index 0dda3fac8..1ff2eff20 100755
> --- a/test/sql-tap/check.test.lua
> +++ b/test/sql-tap/check.test.lua
> @@ -484,7 +484,7 @@ test:do_catchsql_test(
>         UPDATE t4 SET x=0, y=1;
>     ]], {
>         -- <check-4.6>
> -        1, "Failed to execute SQL statement: CHECK constraint failed: CK_CONSTRAINT_1_T4"
> +        1, "Check constraint failed 'CK_CONSTRAINT_1_T4': x+y==11\n            OR x*y==12\n            OR x/y BETWEEN 5 AND 8\n            OR -x==y+10”

Mb it is worth removing from check expr extra spaces and
return carriage symbols to make it more readable?

> diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
> index 0cdfde2e9..2652f3b7d 100644
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> 
> test_run:cmd("clear filter")
> +
> +--
> +-- Test ck constraint corner cases
> +--
> +s = box.schema.create_space('test')
> +_ = s:create_index('pk')
> +s:format({{name='X', type='any'}, {name='Y', type='integer'}, {name='Z', type='integer', is_nullable=true}})
> +_ = box.space._ck_constraint:insert({'XlessY', s.id, 'X < Y and Y < Z'})
> +s:insert({'1', 2})
> +s:insert({})
> +s:insert({2, 1})
> +s:insert({1, 2})
> +s:insert({2, 3, 1})
> +s:insert({2, 3, 4})
> +s:update({2}, {{'+', 2, 3}})
> +s:update({2}, {{'+', 2, 3}, {'+', 3, 3}})
> +s:replace({2, 1, 3})
> +box.snapshot()
> +test_run:cmd("restart server default")
> +s = box.space["test"]
> +s:update({2}, {{'+', 2, 3}})
> +s:update({2}, {{'+', 2, 3}, {'+', 3, 3}})
> +s:replace({2, 1, 3})

What about NULLs and NOT NULL check?

A few comment fixes:

diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
index d692e2116..f88f06f96 100644
--- a/src/box/ck_constraint.c
+++ b/src/box/ck_constraint.c
@@ -61,21 +61,21 @@ ck_constraint_resolve_space_def(struct Expr *expr,
 
 /**
  * Create VDBE machine for ck constraint by given definition and
- * Expression AST. The generated instructions are consist of
+ * expression AST. The generated instructions consist of
  * prologue code that maps tuple fields via bindings and ck
- * constraint code is generated by given expression.
- * In case of ck constraint error the VDBE execution is aborted
- * and error is handled as diag message.
+ * constraint code which implements given expression.
+ * In case of ck constraint error during VDBE execution, it is
+ * aborted and error is handled as diag message.
  * @param ck_constraint_def Check constraint definition to prepare
- *                          errors descriptions.
+ *                          error description.
  * @param expr Check constraint expression AST is built for
  *             given @ck_constraint_def, see for
  *             (sql_expr_compile +
  *              ck_constraint_resolve_space_def) implementation.
  * @param space_def The space definition of the space this check
  *                  constraint is constructed for.
- * @param[out] new_tuple_var The VDBE register that contains the
- *                           first(of allocation) variable to bind
+ * @param[out] new_tuple_var VDBE register that contains the
+ *                           first (of allocated) variable to bind
  *                           the corresponding first tuple field.
  * @retval not NULL sql_stmt program pointer on success.
  * @retval NULL otherwise.
@@ -99,7 +99,7 @@ ck_constraint_program_compile(struct ck_constraint_def *ck_constraint_def,
        uint32_t sql_flags = user_session->sql_flags;
        user_session->sql_flags = default_flags;
        /*
-        * Generate a prologue code that introduce variables to
+        * Generate a prologue code that introduces variables to
         * bind tuple fields there before execution.
         */
        uint32_t field_count = space_def->field_count;
@@ -108,7 +108,7 @@ ck_constraint_program_compile(struct ck_constraint_def *ck_constraint_def,
        struct Expr bind = {.op = TK_VARIABLE, .u.zToken = "?"};
        for (uint32_t i = 0; i < field_count; i++) {
                sqlExprAssignVarNumber(&parser, &bind, 1);
-               sqlExprCodeTarget(&parser, &bind, new_tuple_reg + i);
+               sqlVdbeAddOp2(v, OP_Variable, bind.iColumn, new_tuple_reg + i);
        }
        /* Generate ck constraint test code. */
        vdbe_emit_ck_constraint(&parser, expr, ck_constraint_def->name,
@@ -131,9 +131,9 @@ ck_constraint_program_compile(struct ck_constraint_def *ck_constraint_def,
 }
 
 /**
- * Perform ck constraint test on new tuple data new_tuple
+ * Run bytecode implementing check constraint on new tuple
  * before insert or replace in space space_def.
- * @param ck_constraint Check constraint to perform.
+ * @param ck_constraint Check constraint to run.
  * @param space_def The space definition of the space this check
  *                  constraint is constructed for.
  * @param new_tuple The tuple to be inserted in space.
@@ -150,10 +150,10 @@ ck_constraint_program_run(struct ck_constraint *ck_constraint,
         */
        struct space *space = space_by_id(ck_constraint->space_id);
        assert(space != NULL);
-       uint32_t field_count = mp_decode_array(&new_tuple);
-       uint32_t defined_field_count =
-               MIN(field_count, space->def->field_count);
-       for (uint32_t i = 0; i < defined_field_count; i++) {
+       uint32_t tuple_field_count = mp_decode_array(&new_tuple);
+       uint32_t field_count =
+               MIN(tuple_field_count, space->def->field_count);
+       for (uint32_t i = 0; i < field_count; i++) {
                struct sql_bind bind;
                if (sql_bind_decode(&bind, ck_constraint->new_tuple_var + i,
                                    &new_tuple) != 0 ||
@@ -177,12 +177,12 @@ ck_constraint_program_run(struct ck_constraint *ck_constraint,
 }
 
 /**
- * Ck constraint trigger function. Is expected to be executed in
- * space::on_replace trigger.
+ * Ck constraint trigger function. It ss expected to be executed
+ * in space::on_replace trigger.
  *
- * Extracts all ck constraint related context from event and run
- * bytecode implementing check constraint to test a new tuple
- * before it would be inserted in destination space.
+ * It extracts all ck constraint required context from event and
+ * run bytecode implementing check constraint to test a new tuple
+ * before it will be inserted in destination space.
  */
 static void
 ck_constraint_on_replace_trigger(struct trigger *trigger, void *event)
diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
index 66f812fcc..69654d4bf 100644
--- a/src/box/ck_constraint.h
+++ b/src/box/ck_constraint.h
@@ -82,8 +82,8 @@ struct ck_constraint {
         * Precompiled reusable VDBE program for processing check
         * constraints and setting bad exitcode and error
         * message when ck condition unsatisfied.
-        * Program rely on new_tuple_var parameter to be bound to
-        * the VDBE memory before run.
+        * Program relies on new_tuple_var parameter to be bound
+        * to VDBE memory before run.
         */
        struct sql_stmt *stmt;
        /**
@@ -93,8 +93,8 @@ struct ck_constraint {
         */
        int new_tuple_var;
        /**
-        * Trigger object executing check constraint on space
-        * insert and replace.
+        * Trigger object executing check constraint before
+        * insert and replace operations.
         */
        struct trigger trigger;