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 BDB0B2A733 for ; Tue, 2 Apr 2019 10:14:07 -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 MQ__jC051VuV for ; Tue, 2 Apr 2019 10:14:07 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 E5D8E2A5CD for ; Tue, 2 Apr 2019 10:14:06 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\)) Subject: [tarantool-patches] Re: [PATCH v2 9/9] sql: run check constraint tests on space alter From: "n.pettik" In-Reply-To: <383ecdc1-35d8-7a15-fe7c-e1cdba941dfd@tarantool.org> Date: Tue, 2 Apr 2019 17:14:04 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <121C5F34-691B-4FCC-A1C9-C995683D4A6A@tarantool.org> References: <9ba32d3784fdf28096b8ccde7d5fb3c6a55c0826.1548838034.git.kshcherbatov@tarantool.org> <383ecdc1-35d8-7a15-fe7c-e1cdba941dfd@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 26 Mar 2019, at 13:59, Kirill Shcherbatov = wrote: >=20 > Introduced reusable pre-compiled VDBE programs for ck constraints > and space trigger to fire checks on insert and update operations. As usual, extremely brief description of vital feature. Fill commit message with details of chosen approach. > Closes #3691 >=20 > @TarantoolBot document > Title: check constraint for LUA space >=20 > Now it is possible to create check constraints for LUA spaces: > insert the tuple {, , } in > the _ck_constraint space to create new check constraint. >=20 > Example: > s =3D box.schema.create_space('test') > s:create_index('pk') > format =3D {{'X', 'unsigned'}, {'Y', 'unsigned'}} > s:format(format) Mention that format is required condition to use check constraints: otherwise, name of field can=E2=80=99t be resolved. Now it leads to assertion fault: s =3D box.schema.create_space('test=E2=80=99) s:create_index('pk=E2=80=99) box.space._ck_constraint:insert({'physics', s.id, 'Xfield_count > 0), function lookupName, = file tarantool/src/box/sql/resolve.c, line 242. > box.space._ck_constraint:insert({'physics', s.id, 'X box.space.test:insert({6, 5}) > - error: 'Check constraint failed: physics=E2=80=99 To finish this patch-set I suggest to add Lua-wrapper to create check constraints on any space using NoSQL interface and introduce ALTER TABLE ADD CONSTRAINT CHECK(). Last issue you can implement in a separate patch or delegate its implementation to smb else. > --- > src/box/alter.cc | 21 +++++- > src/box/ck_constraint.c | 142 ++++++++++++++++++++++++++++++++++++++- > src/box/ck_constraint.h | 24 +++++-- > src/box/sql.c | 1 + > src/box/sql/insert.c | 94 ++++++-------------------- > src/box/sql/sqlInt.h | 25 +++++++ > src/box/sql/vdbeapi.c | 8 --- > test/sql/checks.result | 20 ++++++ > test/sql/checks.test.lua | 5 ++ > 9 files changed, 248 insertions(+), 92 deletions(-) >=20 > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 9aa5e3653..b6d1c8537 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -1379,6 +1379,11 @@ BuildCkConstraints::alter(struct alter_space = *alter) > { > rlist_swap(&alter->new_space->ck_constraint, &ck_constraint); > rlist_swap(&ck_constraint, &alter->old_space->ck_constraint); > + struct ck_constraint *ck; > + rlist_foreach_entry(ck, &ck_constraint, link) > + trigger_clear(&ck->trigger); > + rlist_foreach_entry(ck, &alter->new_space->ck_constraint, link) > + trigger_add(&alter->new_space->before_replace, = &ck->trigger); > } >=20 > void > @@ -1386,6 +1391,11 @@ BuildCkConstraints::rollback(struct alter_space = *alter) > { > rlist_swap(&alter->old_space->ck_constraint, &ck_constraint); > rlist_swap(&ck_constraint, &alter->new_space->ck_constraint); > + struct ck_constraint *ck; > + rlist_foreach_entry(ck, &ck_constraint, link) > + trigger_clear(&ck->trigger); > + rlist_foreach_entry(ck, &alter->old_space->ck_constraint, link) > + trigger_add(&alter->old_space->before_replace, = &ck->trigger); > } >=20 > BuildCkConstraints::~BuildCkConstraints() >=20 > diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c > index 110098efc..c2e8547f1 100644 > --- a/src/box/ck_constraint.c > +++ b/src/box/ck_constraint.c > @@ -29,11 +29,16 @@ > * SUCH DAMAGE. > */ > #include > +#include "bind.h" > #include "ck_constraint.h" > #include "errcode.h" > +#include "session.h" > +#include "schema.h" > #include "small/rlist.h" > +#include "tuple.h" > #include "sql.h" > #include "sql/sqlInt.h" > +#include "sql/vdbeInt.h" >=20 > /** > * Resolve space_def references for check constraint via AST > @@ -84,6 +89,130 @@ ck_constraint_def_create(struct ck_constraint_def = *ck_constraint_def, > rlist_create(&ck_constraint_def->link); > } >=20 > +/** > + * Compile constraint check subroutine. > + * @param ck_constraint Check constraint to compile. > + * @param expr Check constraint expression AST is built for > + * ck_constraint->def. Can=E2=80=99t understand this description. > + * @param space_def The space definition of the space this check > + * constraint is constructed for. > + * @retval not NULL sql_stmt program pointer on success. > + * @retval NULL otherwise. > + */ > +int This function is used only in ck_constraint.c, let=E2=80=99s make it = static. > +ck_constraint_test_compile(struct ck_constraint *ck_constraint, I would call it =E2=80=9Cck_constraint_program_compile=E2=80=9D or = =E2=80=9Cck_constraint_routine_compile=E2=80=9D or =E2=80=9Cck_constraint_bytecode_compile=E2=80=9D. > + struct Expr *expr, const struct space_def = *space_def) > +{ > + int rc =3D -1; Remove this rc pls. Diff: diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c index c2e8547f1..6f54e35e1 100644 --- a/src/box/ck_constraint.c +++ b/src/box/ck_constraint.c @@ -103,15 +103,13 @@ int ck_constraint_test_compile(struct ck_constraint *ck_constraint, struct Expr *expr, const struct space_def = *space_def) { - int rc =3D -1; assert(ck_constraint->space_id =3D=3D space_def->id); struct Parse parser; sql_parser_create(&parser, sql_get()); struct Vdbe *v =3D sqlGetVdbe(&parser); if (v =3D=3D NULL) { - diag_set(OutOfMemory, sizeof(struct Vdbe), - "sqlGetVdbe", "vdbe"); - goto end; + diag_set(OutOfMemory, sizeof(struct Vdbe), = "sqlDbMalloc", "v"); + return -1; } =20 /* Compile VDBE with default sql parameters. */ @@ -138,16 +136,14 @@ ck_constraint_test_compile(struct ck_constraint = *ck_constraint, diag_set(ClientError, ER_CREATE_CK_CONSTRAINT, ck_constraint->def->name, "can not compile expression"); - goto end; + return -1; } sql_parser_destroy(&parser); =20 /* Restore original sql flags for user_session. */ user_session->sql_flags =3D sql_flags; ck_constraint->stmt =3D (struct sql_stmt *)v; - rc =3D 0; -end: - return rc; + return 0; } > + assert(ck_constraint->space_id =3D=3D space_def->id); > + struct Parse parser; > + sql_parser_create(&parser, sql_get()); > + struct Vdbe *v =3D sqlGetVdbe(&parser); > + if (v =3D=3D NULL) { > + diag_set(OutOfMemory, sizeof(struct Vdbe), > + "sqlGetVdbe", "vdbe"); > + goto end; > + } > + > + /* 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 to bind variable new_tuple_var > + * to new_tuple_reg. > + */ This comment explains nothing. > + uint32_t field_count =3D space_def->field_count; > + int new_tuple_reg =3D sqlGetTempRange(&parser, field_count); > + struct Expr bind =3D {.op =3D TK_VARIABLE, .u.zToken =3D "?"}; > + ck_constraint->new_tuple_var =3D parser.nVar + 1; > + for (uint32_t i =3D 0; i < field_count; i++) { > + sqlExprAssignVarNumber(&parser, &bind, 1); > + sqlExprCodeTarget(&parser, &bind, new_tuple_reg + i); > + } > + vdbe_emit_ck_constraint(&parser, expr, ck_constraint->def->name, > + new_tuple_reg); > + sql_finish_coding(&parser); Do we need to call this function at all? > + if (parser.is_aborted) { > + diag_set(ClientError, ER_CREATE_CK_CONSTRAINT, > + ck_constraint->def->name, > + "can not compile expression=E2=80=9D); This error will re-set original parsing error. I suggest to concatenate them. > +/** > + * Perform ck constraint checks with new tuple data new_tuple_raw > + * before insert or replace in space space_def. These variable names don=E2=80=99t say anything. > + * @param ck_constraint Check constraint to test. "To perform" or "to execute=E2=80=9D. > + * @param space_def The space definition of the space this check > + * constraint is constructed for. > + * @param new_tuple_raw The tuple to be inserted in space. > + * @retval 0 if check constraint test is passed, -1 otherwise. > + */ > +static int > +ck_constraint_test(struct ck_constraint *ck_constraint, > + struct space_def *space_def, const char = *new_tuple_raw) -> ck_constraint_execute/ck_constraint_run/ck_constraint_fire etc. > +{ > + assert(new_tuple_raw !=3D NULL); > + /* > + * Prepare parameters for checks->stmt execution: > + * Unpacked new tuple fields mapped to Vdbe memory from > + * variables from range: > + * [new_tuple_var,new_tuple_var+field_count] > + */ > + mp_decode_array(&new_tuple_raw); > + /* Reset VDBE to make new bindings. */ > + sql_reset(ck_constraint->stmt); This function returns error code, don=E2=80=99t ignore it. > + for (uint32_t i =3D 0; i < space_def->field_count; i++) { > + struct sql_bind bind; > + if (sql_bind_decode(&bind, ck_constraint->new_tuple_var = + i, > + &new_tuple_raw) !=3D 0) > + return -1; > + if (sql_bind_column(ck_constraint->stmt, &bind, > + ck_constraint->new_tuple_var + i) !=3D= 0) > + return -1; > + } > + /* Checks VDBE can't expire, reset expired flag & Burn. */ What does mean =E2=80=9C& Burn=E2=80=9D ? Not =E2=80=9CChecks VDBE=E2=80=9D= but =E2=80=9Cbytecode implementing check constraint=E2=80=9D. > + struct Vdbe *v =3D (struct Vdbe *)ck_constraint->stmt; > + v->expired =3D 0; > + int rc; > + while ((rc =3D sql_step(ck_constraint->stmt)) =3D=3D SQL_ROW) {} > + if (v->rc !=3D SQL_DONE && v->rc !=3D SQL_TARANTOOL_ERROR) > + diag_set(ClientError, ER_SQL, v->zErrMsg); > + return rc =3D=3D SQL_DONE ? 0 : -1; > +} > + > +/** > + * Trigger routine executing ck constraint check on space > + * insert and replace. Sounds like a set of random words :)) Re-phrase somehow pls. > +static void > +ck_constraint_space_trigger(struct trigger *trigger, void *event) ck_constraint_before_replace_trigger would sound better IMHO. > +{ > + struct ck_constraint *ck_constraint =3D > + (struct ck_constraint *)trigger->data; > + struct space *space =3D space_by_id(ck_constraint->space_id); > + assert(space !=3D NULL); > + struct txn *txn =3D (struct txn *) event; > + struct txn_stmt *stmt =3D txn_current_stmt(txn); > + struct tuple *new_tuple =3D stmt->new_tuple; > + if (stmt =3D=3D NULL || new_tuple =3D=3D NULL) > + return; > + if (ck_constraint_test(ck_constraint, space->def, > + tuple_data(new_tuple)) !=3D 0) > + diag_raise(); > +} > + > struct ck_constraint * > ck_constraint_new(const struct ck_constraint_def *ck_constraint_def, > struct space_def *space_def) > @@ -110,6 +239,8 @@ ck_constraint_new(const struct ck_constraint_def = *ck_constraint_def, > ck_constraint_def_create(ck_constraint->def, = ck_constraint_def->name, > ck_constraint_name_len, > ck_constraint_def->expr_str, = expr_str_len); > + trigger_create(&ck_constraint->trigger, = ck_constraint_space_trigger, > + ck_constraint, NULL); > struct Expr *expr =3D > sql_expr_compile(sql_get(), ck_constraint_def->expr_str, > expr_str_len); > @@ -120,18 +251,23 @@ ck_constraint_new(const struct ck_constraint_def = *ck_constraint_def, > box_error_message(box_error_last())); > goto error; > } > - ck_constraint->expr =3D expr; > + if (ck_constraint_test_compile(ck_constraint, expr, space_def) = !=3D 0) > + goto error; >=20 > +end: > + sql_expr_delete(sql_get(), expr, false); > return ck_constraint; > error: > ck_constraint_delete(ck_constraint); > - return NULL; > + ck_constraint =3D NULL; > + goto end; Trivial refactoring: @@ -254,13 +250,12 @@ ck_constraint_new(const struct ck_constraint_def = *ck_constraint_def, if (ck_constraint_test_compile(ck_constraint, expr, space_def) = !=3D 0) goto error; =20 -end: sql_expr_delete(sql_get(), expr, false); return ck_constraint; error: ck_constraint_delete(ck_constraint); - ck_constraint =3D NULL; - goto end; + sql_expr_delete(sql_get(), expr, false); + return NULL; } > diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h > index 02aa525ce..77b1a9bed 100644 > --- a/src/box/ck_constraint.h > +++ b/src/box/ck_constraint.h > @@ -32,6 +32,7 @@ > */ >=20 > #include > +#include "trigger.h" > #include "small/rlist.h=E2=80=9D You can remove this header: trigger.h includes it as well. >=20 > #if defined(__cplusplus) > @@ -40,6 +41,7 @@ extern "C" { >=20 > struct space; > struct space_def; > +struct sql_stmt; > struct Expr; >=20 > /** > @@ -72,17 +74,29 @@ struct ck_constraint { > */ > struct ck_constraint_def *def; > /** > - * The check constraint expression AST is built for > - * ck_constraint::def::expr_str with sql_expr_compile > - * and resolved with sqlResolveExprNames for > - * space with space[ck_constraint::space_id] definition. > + * Precompiled reusable VDBE program for proceeding ck proceeding -> processing ck constraint checks -> check constraints > + * constraint checks and setting bad exitcode and error > + * message when ck condition unsatisfied. > + * Program rely on new_tuple_var parameter to be binded binded in -> bound to=20 > + * in the VDBE memory before run. > */ > - struct Expr *expr; > + struct sql_stmt *stmt; > /** > * The id of the space this check constraint is > * built for. > */ > uint32_t space_id; > + /** > + * The first ck_constraint::stmt VDBE variable of the > + * range space[ck_constraint::space_id]->def->field_count > + * representing a new tuple to be inserted. > + */ > + int new_tuple_var; > + /** > + * Trigger object executing check constraint on space > + * insert and replace. > + */ > + struct trigger trigger; > /** > * Organize check constraint structs into linked list > * with space::ck_constraint. > diff --git a/src/box/sql.c b/src/box/sql.c > index fc469126e..912b24bf0 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -29,6 +29,7 @@ > * SUCH DAMAGE. > */ > #include > +#include "bind.h" > #include "field_def.h" > #include "cfg.h" > #include "sql.h" > diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c > index 2fe74a027..3caad3c24 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -798,51 +798,26 @@ sqlInsert(Parse * pParse, /* Parser = context */ > sqlDbFree(db, aRegIdx); > } >=20 > -/* > - * Meanings of bits in of pWalker->eCode for = checkConstraintUnchanged() > - */ > -#define CKCNSTRNT_COLUMN 0x01 /* CHECK constraint uses a = changing column */ > - > -/* This is the Walker callback from checkConstraintUnchanged(). Set > - * bit 0x01 of pWalker->eCode if > - * pWalker->eCode to 0 if this expression node references any of the > - * columns that are being modifed by an UPDATE statement. > - */ > -static int > -checkConstraintExprNode(Walker * pWalker, Expr * pExpr) > -{ > - if (pExpr->op =3D=3D TK_COLUMN) { > - assert(pExpr->iColumn >=3D 0 || pExpr->iColumn =3D=3D = -1); > - if (pExpr->iColumn >=3D 0) { > - if (pWalker->u.aiCol[pExpr->iColumn] >=3D 0) { > - pWalker->eCode |=3D CKCNSTRNT_COLUMN; > - } > - } > - } > - return WRC_Continue; > -} > - > -/* > - * pExpr is a CHECK constraint on a row that is being UPDATE-ed. The > - * only columns that are modified by the UPDATE are those for which > - * aiChng[i]>=3D0. > - * > - * Return true if CHECK constraint pExpr does not use any of the > - * changing columns. In other words, return true if this CHECK = constraint > - * can be skipped when validating the new row in the UPDATE = statement. > - */ > -static int > -checkConstraintUnchanged(Expr * pExpr, int *aiChng) > +void > +vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr, > + const char *name, int new_tuple_reg) > { > - Walker w; > - memset(&w, 0, sizeof(w)); > - w.eCode =3D 0; > - w.xExprCallback =3D checkConstraintExprNode; > - w.u.aiCol =3D aiChng; > - sqlWalkExpr(&w, pExpr); > - testcase(w.eCode =3D=3D 0); > - testcase(w.eCode =3D=3D CKCNSTRNT_COLUMN); > - return !w.eCode; > + parser->ckBase =3D new_tuple_reg; > + struct Vdbe *v =3D sqlGetVdbe(parser); > + const char *ck_constraint_name =3D sqlDbStrDup(parser->db, = name); Where's this pointer released? > + VdbeNoopComment((v, "BEGIN: ck constraint %s test", name)); > + /* Skip check when it is turned off. */ When it can be turned off? > + int all_is_ok =3D sqlVdbeMakeLabel(v); all_is_ok -> check_is_passed > + sqlExprIfTrue(parser, expr, all_is_ok, SQL_JUMPIFNULL); > + sqlMayAbort(parser); > + const char *fmt =3D tnt_errcode_desc(ER_CK_CONSTRAINT_FAILED); > + const char *error_msg =3D tt_sprintf(fmt, ck_constraint_name); > + sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR, > + ON_CONFLICT_ACTION_ABORT, 0, > + sqlDbStrDup(parser->db, error_msg), P4_DYNAMIC); > + sqlVdbeChangeP5(v, ER_CK_CONSTRAINT_FAILED); > + VdbeNoopComment((v, "END: ck constraint %s test", name)); > + sqlVdbeResolveLabel(v, all_is_ok); > } >=20 >=20 > diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua > index 9a7e5faf4..0e6c990c9 100644 > --- a/test/sql/checks.test.lua > +++ b/test/sql/checks.test.lua > @@ -27,9 +27,11 @@ = box.space._ck_constraint:insert({'CK_CONSTRAINT_01', 513, 'X<5'}) > box.space._ck_constraint:count({}) >=20 > box.sql.execute("INSERT INTO \"test\" VALUES(5);") > +box.space.test:insert({5}) > box.space._ck_constraint:replace({'CK_CONSTRAINT_01', 513, 'X<=3D5'}) > box.sql.execute("INSERT INTO \"test\" VALUES(5);") > box.sql.execute("INSERT INTO \"test\" VALUES(6);") > +box.space.test:insert({6}) > -- Can't drop table with check constraints. > box.space.test:delete({5}) > box.space.test.index.pk:drop() > @@ -41,8 +43,11 @@ box.space.test:drop() > box.sql.execute("CREATE TABLE t1(x INTEGER CONSTRAINT ONE CHECK( x<5 = ), y REAL CONSTRAINT TWO CHECK( y>x ), z INTEGER PRIMARY KEY);") > box.space._ck_constraint:count() > box.sql.execute("INSERT INTO t1 VALUES (7, 1, 1)") > +box.space.T1:insert({7, 1, 1}) > box.sql.execute("INSERT INTO t1 VALUES (2, 1, 1)") > +box.space.T1:insert({2, 1, 1}) > box.sql.execute("INSERT INTO t1 VALUES (2, 4, 1)") > +box.space.T1:update({1}, {{'+', 1, 5}}) > box.sql.execute("DROP TABLE t1=E2=80=9D) Please, add descent set of tests verifying that check constraints work in any possible scenario. Make sure that check occurs before replace action.