[tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter
n.pettik
korablev at tarantool.org
Thu Apr 25 23:38:47 MSK 2019
> On 16 Apr 2019, at 16:51, Kirill Shcherbatov <kshcherbatov at 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;
More information about the Tarantool-patches
mailing list