From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 50161266BD for ; Thu, 25 Apr 2019 16:38:50 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id EzxqgtN4FdEK for ; Thu, 25 Apr 2019 16:38:50 -0400 (EDT) Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id DE3DE26358 for ; Thu, 25 Apr 2019 16:38:49 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.8\)) Subject: [tarantool-patches] Re: [PATCH v3 2/3] box: run check constraint tests on space alter From: "n.pettik" In-Reply-To: <5ecf24bcc8c4676d1bcfe0a36c9bb27ebc8281cd.1555420166.git.kshcherbatov@tarantool.org> Date: Thu, 25 Apr 2019 23:38:47 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <5ecf24bcc8c4676d1bcfe0a36c9bb27ebc8281cd.1555420166.git.kshcherbatov@tarantool.org> Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: tarantool-patches@freelists.org Cc: Kirill Shcherbatov > On 16 Apr 2019, at 16:51, Kirill Shcherbatov = wrote: >=20 > 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. >=20 > Closes #3691 One unpleasant =E2=80=9Cfeature=E2=80=9D that I=E2=80=99ve 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=E2=80=99 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=E2=80=99ve 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]=3Dparameter(1,) 102> 2 Noop 0 0 0 00 BEGIN: ck = constraint CK_CONSTRAINT_1_T4 test 102> 3 Gt 3 6 1 ({type =3D 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=20 102> 5 Noop 0 0 0 00 END: ck = constraint CK_CONSTRAINT_1_T4 test 102> 6 Halt 0 0 0 00=20 102> 7 Integer 6 4 0 00 r[4]=3D6 102> 8 Integer 7 6 0 00 r[6]=3D7 102> 9 Integer 3 7 0 00 r[7]=3D3 102> 10 Multiply 7 6 5 00 r[5]=3Dr[7]*r[6] 102> 11 Add 5 4 2 00 r[2]=3Dr[5]+r[4] 102> 12 Integer 1 4 0 00 r[4]=3D1 102> 13 Integer 31 7 0 00 r[7]=3D31 102> 14 Multiply 7 4 5 00 r[5]=3Dr[7]*r[4] 102> 15 Subtract 5 2 3 00 r[3]=3Dr[2]-r[5] 102> 16 Goto 0 1 0 00=20 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 >=20 > 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" >=20 > /** > * Resolve space_def references for check constraint via AST > @@ -55,6 +59,146 @@ ck_constraint_resolve_space_def(struct Expr *expr, >=20 > +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) >=20 > + /* Compile VDBE with default sql parameters. */ > + struct session *user_session =3D current_session(); > + uint32_t sql_flags =3D user_session->sql_flags; > + user_session->sql_flags =3D default_flags; > + /* > + * Generate a prologue code that introduce variables to > + * bind tuple fields there before execution. > + */ > + uint32_t field_count =3D space_def->field_count; > + int new_tuple_reg =3D sqlGetTempRange(&parser, field_count); Nit: new_tuple_reg IMHO is very general name. Mb it would be better to call it like =E2=80=9Cbind_tuple_reg=E2=80=9D? > + *new_tuple_var =3D parser.nVar + 1; Won=E2=80=99t it be always 1? You=E2=80=99ve just created parser and no = byte code is generated yet, so how it could be different from 1? sqlGetTempRange() doesn=E2=80=99t = affect nVar AFAIU. > + struct Expr bind =3D {.op =3D TK_VARIABLE, .u.zToken =3D "?"}; > + for (uint32_t i =3D 0; i < field_count; i++) { > + sqlExprAssignVarNumber(&parser, &bind, 1); > + sqlExprCodeTarget(&parser, &bind, new_tuple_reg + i); You don=E2=80=99t 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 =3D 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 !=3D 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 =3D space_by_id(ck_constraint->space_id); > + assert(space !=3D NULL); > + for (uint32_t i =3D 0; i < space->def->field_count; i++) { Mm, this code differs from one on brach: uint32_t field_count =3D 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=3D0, y=3D1; > ]], { > -- > - 1, "Failed to execute SQL statement: CHECK constraint failed: = CK_CONSTRAINT_1_T4" > + 1, "Check constraint failed 'CK_CONSTRAINT_1_T4': x+y=3D=3D11\n= OR x*y=3D=3D12\n OR x/y BETWEEN 5 AND 8\n = OR -x=3D=3Dy+10=E2=80=9D 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 >=20 > test_run:cmd("clear filter") > + > +-- > +-- Test ck constraint corner cases > +-- > +s =3D box.schema.create_space('test') > +_ =3D s:create_index('pk') > +s:format({{name=3D'X', type=3D'any'}, {name=3D'Y', type=3D'integer'}, = {name=3D'Z', type=3D'integer', is_nullable=3Dtrue}}) > +_ =3D 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 =3D 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, =20 /** * 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 =3D user_session->sql_flags; user_session->sql_flags =3D 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 =3D space_def->field_count; @@ -108,7 +108,7 @@ ck_constraint_program_compile(struct = ck_constraint_def *ck_constraint_def, struct Expr bind =3D {.op =3D TK_VARIABLE, .u.zToken =3D "?"}; for (uint32_t i =3D 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, } =20 /** - * 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 =3D space_by_id(ck_constraint->space_id); assert(space !=3D NULL); - uint32_t field_count =3D mp_decode_array(&new_tuple); - uint32_t defined_field_count =3D - MIN(field_count, space->def->field_count); - for (uint32_t i =3D 0; i < defined_field_count; i++) { + uint32_t tuple_field_count =3D mp_decode_array(&new_tuple); + uint32_t field_count =3D + MIN(tuple_field_count, space->def->field_count); + for (uint32_t i =3D 0; i < field_count; i++) { struct sql_bind bind; if (sql_bind_decode(&bind, ck_constraint->new_tuple_var = + i, &new_tuple) !=3D 0 || @@ -177,12 +177,12 @@ ck_constraint_program_run(struct ck_constraint = *ck_constraint, } =20 /** - * 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;