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 112B92E053 for ; Sun, 26 May 2019 08:07:37 -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 DKHDSL43eU4B for ; Sun, 26 May 2019 08:07:36 -0400 (EDT) Received: from smtp39.i.mail.ru (smtp39.i.mail.ru [94.100.177.99]) (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 96C5E2C75B for ; Sun, 26 May 2019 08:07:36 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v5 5/6] box: run check constraint tests on space alter References: <404d8f6200796b0cf2ef5eccc8c8e67bd593ab07.1558605591.git.kshcherbatov@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Sun, 26 May 2019 15:07:34 +0300 MIME-Version: 1.0 In-Reply-To: <404d8f6200796b0cf2ef5eccc8c8e67bd593ab07.1558605591.git.kshcherbatov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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, Kirill Shcherbatov Thanks for the patch! See 9 comments below. In the commit title you somewhy said, that you run CHECKs on alter, but it is not so. CHECKs on a non-empty space disable its alteration. This patch runs CHECKs on DML, not on DDL. Additionally I just realized, that probably the standard explicitly says in which order we should run CHECKs - before ON REPLACE triggers or after? Or it does not matter? Please, investigate. If CHECKs are always first, for example, then we should append CHECKs on_replace trigger in the first place. On 23/05/2019 13:19, Kirill Shcherbatov wrote: > To perform ck constraints tests before insert or update space > operation, we use precompiled VDBE machine associated with > each ck constraint, that is executed in on_replace trigger. > Each ck constraint VDBE code consists of > 1) prologue code that maps new(or updated) tuple via binding, > 2) ck constraint code generated by CK constraint AST. > In case of ck constraint error the tuple insert/replace operation > is aborted and ck constraint error is handled as diag message. > > Needed for #3691 > --- > src/box/alter.cc | 64 ++++- > src/box/ck_constraint.c | 131 ++++++++++- > src/box/ck_constraint.h | 29 ++- > src/box/errcode.h | 1 + > src/box/space.c | 4 + > src/box/space.h | 3 + > src/box/sql/expr.c | 25 +- > src/box/sql/insert.c | 92 ++------ > src/box/sql/sqlInt.h | 36 ++- > src/box/sql/vdbe.h | 1 - > src/box/sql/vdbeapi.c | 8 - > test/box/misc.result | 1 + > test/sql-tap/check.test.lua | 32 +-- > test/sql-tap/fkey2.test.lua | 4 +- > test/sql-tap/table.test.lua | 12 +- > test/sql/checks.result | 324 +++++++++++++++++++++++++- > test/sql/checks.test.lua | 96 ++++++++ > test/sql/errinj.result | 18 +- > test/sql/gh-2981-check-autoinc.result | 12 +- > test/sql/types.result | 3 +- > 20 files changed, 744 insertions(+), 152 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 463357c67..e4ded1995 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -1421,6 +1421,12 @@ RebuildCkConstraints::space_swap_ck_constraint(struct space *old_space, > { > rlist_swap(&new_space->ck_constraint, &ck_constraint); > rlist_swap(&ck_constraint, &old_space->ck_constraint); > + > + trigger_clear(&old_space->ck_constraint_trigger); > + if (!rlist_empty(&new_space->ck_constraint)) { > + trigger_add(&old_space->on_replace, > + &new_space->ck_constraint_trigger); > + } 1. But it is not a swap. Before it was old_space: old_space.trigger, new_space: nil After it is old_space: new_space.trigger, new_space: nil After one another 'swap' nothing changes - you just always assign a new trigger to the old space. swap(swap(a, b)) should be equal (a, b). In your case you somehow get swap(swap(a, nil)) = (c, nil). What is more, now old_space has 2 pointers at new_space: the new_space pointer itself (via trigger.data), and &new_space.trigger (via on_replace) list. It is not ok, obviously. Probably you decided to do so, because this error is later recovered by space_swap_triggers() - it is a hack, sorry. I think, that in this function you should not even touch on_replace. Lets deal with these problems one-by-one. 1) About old_space referencing new_space via trigger.data. You don't need it. In the trigger function you obtain struct txn_stmt, which already contains struct space pointer. You do not need to assign trigger.data at all. Remove it, please. 2) About old_space referencing &new_space.trigger. Lets store in struct space a pointer at struct trigger for ck constraints. Not the trigger itself, but a pointer. It solves the problem, and allows to swap struct trigger pointers in Move and Rebuild. Also, it allows to do not allocate struct trigger when it is not used. Now it is allocated always, because is a part of struct space. You will allocate that trigger on demand, when a first CK appears. When you will have the things above done, your Move and RebuildCK will not even touch space.on_replace. They will only swap ck lists and struct trigger *ck_trigger pointer in old_ and new_space. Space.on_replace swap will be done by space_swap_triggers(). > } > > diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c > index c74687b31..ba2df4b67 100644 > --- a/src/box/ck_constraint.c > +++ b/src/box/ck_constraint.c > @@ -57,6 +62,116 @@ ck_constraint_resolve_field_names(struct Expr *expr, > return rc; > } > > +/** > + * Create a VDBE machine for the ck constraint by a given > + * definition and an expression AST. The generated instructions > + * consist of prologue code that maps tuple_fetcher via binding > + * and ck constraint code that implements a given expression. > + * @param ck_constraint_def Check constraint definition to prepare > + * an error description. > + * @param expr Ck constraint expression AST built for a given > + * @a ck_constraint_def, see for (sql_expr_compile and > + * ck_constraint_resolve_space_def) implementation. > + * @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. > + */ > +static struct sql_stmt * > +ck_constraint_program_compile(struct ck_constraint_def *ck_constraint_def, > + struct Expr *expr) > +{ > + struct sql *db = sql_get(); > + struct Parse parser; > + sql_parser_create(&parser, db, default_flags); > + struct Vdbe *v = sqlGetVdbe(&parser); > + if (v == NULL) { > + diag_set(OutOfMemory, sizeof(struct Vdbe), "sqlGetVdbe", > + "vdbe"); 2. You did not destroy a parser. > + return NULL; > + } > + /* > + * Generate a prologue code that introduces variables to > + * bind tuple_fetcher before execution. > + */ > + int tuple_fetcher_reg = sqlGetTempReg(&parser); > + sqlVdbeAddOp2(v, OP_Variable, ++parser.nVar, tuple_fetcher_reg); > + /* Generate ck constraint test code. */ > + vdbe_emit_ck_constraint(&parser, expr, ck_constraint_def->name, > + ck_constraint_def->expr_str, tuple_fetcher_reg); > + > + /* Clean-up and restore user-defined sql context. */ > + bool is_error = parser.is_aborted; > + sql_finish_coding(&parser); > + sql_parser_destroy(&parser); > + > + if (is_error) { > + diag_set(ClientError, ER_CREATE_CK_CONSTRAINT, > + ck_constraint_def->name, > + box_error_message(box_error_last())); > + sql_finalize((struct sql_stmt *) v); > + return NULL; > + } > + return (struct sql_stmt *) v; > +} > + > +void > +ck_constraint_on_replace_trigger(struct trigger *trigger, void *event) > +{ > + struct txn *txn = (struct txn *) event; > + struct txn_stmt *stmt = txn_current_stmt(txn); > + assert(stmt != NULL); > + struct tuple *new_tuple = stmt->new_tuple; > + if (new_tuple == NULL) > + return; > + > + struct space *space = (struct space *) trigger->data; 3. Take struct space from txn_stmt and do not touch trigger.data. Otherwise the trigger object depends on space, and you can't swap them. > + uint32_t fetcher_sz = sizeof(struct tuple_fetcher) + > + sizeof(uint32_t) * space->def->field_count; > + struct tuple_fetcher *fetcher = region_alloc(&fiber()->gc, fetcher_sz); > + if (fetcher == NULL) { > + diag_set(OutOfMemory, fetcher_sz, "region_alloc", "fetcher"); > + diag_raise(); > + } > + tuple_fetcher_create(fetcher, new_tuple, tuple_data(new_tuple), > + new_tuple->bsize); > + > + struct ck_constraint *ck_constraint; > + rlist_foreach_entry(ck_constraint, &space->ck_constraint, link) { > + if (ck_constraint_program_run(ck_constraint, fetcher) != 0) > + diag_raise(); > + } 4. Perfect, this should work quite swiftly. > diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h > index e20203bb5..f76e2d5f4 100644 > --- a/src/box/ck_constraint.h > +++ b/src/box/ck_constraint.h > @@ -32,6 +32,8 @@ > */ > > #include > +#include "trigger.h" 5. You do not need that header. Just announce struct trigger. > +#include "sql.h" 6. Why do you need it? The only new function here is ck_constraint_on_replace_trigger and its declaration does not depend on SQL. The only new attribute here is struct sql_stmt, but you announced it, and it is ok. > #include "small/rlist.h" > > #if defined(__cplusplus) > diff --git a/src/box/sql/expr.c b/src/box/sql/expr.c > index 017b36fb4..7f13098ed 100644 > --- a/src/box/sql/expr.c > +++ b/src/box/sql/expr.c > @@ -3730,16 +3730,23 @@ sqlExprCodeTarget(Parse * pParse, Expr * pExpr, int target) > case TK_COLUMN:{ > int iTab = pExpr->iTable; > int col = pExpr->iColumn; > + if (pParse->tuple_fetcher_reg > 0) { > + /* Generating CHECK constraints. */ > + assert(iTab < 0); > + sqlVdbeAddOp3(v, OP_Fetch, > + pParse->tuple_fetcher_reg, > + col, target); > + return target; > + } 7. Please, move that code into 'if (iTab < 0)'. Otherwise non-CHECK code will check two conditions - tuple_fetcher_reg and iTab. Before your patch it was checking iTab only. > if (iTab < 0) { > - if (pParse->ckBase > 0) { > - /* Generating CHECK constraints. */ > - return col + pParse->ckBase; > - } else { > - /* Coding an expression that is part of an index where column names > - * in the index refer to the table to which the index belongs > - */ > - iTab = pParse->iSelfTab; > - } > + /* > + * Coding an expression that is > + * a part of an index where column > + * names in the index refer to > + * the table to which the > + * index belongs. > + */ > + iTab = pParse->iSelfTab; > } > return sqlExprCodeGetColumn(pParse, > pExpr->space_def, col, > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index abb7b9c2a..d602116da 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -605,6 +605,17 @@ sql_column_value(sql_stmt *, > int > sql_finalize(sql_stmt * pStmt); > > +/* > + * Terminate the current execution of an SQL statement and reset > + * it back to its starting state so that it can be reused. > + * > + * @param stmt VDBE program, may be NULL. 8. Why do you allow NULL? I grepped and see that it is never NULL. > + * @retval SQL_OK On success. > + * @retval sql_ret_code Error code on error. > + */ > +int > +sql_reset(struct sql_stmt *stmt); > @@ -3907,6 +3920,23 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, > enum on_conflict_action on_conflict, > int ignore_label, int *upd_cols); > > +/** > + * Gnerate code to make check constraints tests on tuple insertion > + * on INSERT, REPLACE or UPDATE operations. > + * @param parser Current parsing context. > + * @param expr Check constraint AST. > + * @param expr_str Ck constraint expression source string to > + * raise an informative error. > + * @param name Check constraint name to raise an informative > + * error. 9. Mismatching order of parameters. > + * @param tuple_fetcher_reg The VDBE register with prepared > + * tuple_fetcher pointer inside is > + * initialized with a tuple to be inserted. > + */ > +void > +vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr, > + const char *name, const char *expr_str, > + int tuple_fetcher_reg);