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 354BA2E554 for ; Sun, 19 May 2019 12:02:11 -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 pRQoFRsnkGuY for ; Sun, 19 May 2019 12:02:11 -0400 (EDT) Received: from smtp56.i.mail.ru (smtp56.i.mail.ru [217.69.128.36]) (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 C0C972E575 for ; Sun, 19 May 2019 12:02:10 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v4 2/4] box: run check constraint tests on space alter References: From: Vladislav Shpilevoy Message-ID: <9dfc18b3-8566-8b25-3c08-c94d83849f5f@tarantool.org> Date: Sun, 19 May 2019 19:02:07 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Language: en-US Content-Transfer-Encoding: 8bit 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: Kirill Shcherbatov , tarantool-patches@freelists.org Thanks for the patch! See 18 comments below. On 16/05/2019 16:56, Kirill Shcherbatov wrote: > To perform ck constraints tests before insert or update space operation, > we use precompiled VDBE machine is associated with each ck constraint, 1. 'we use machine is associated' - ??? Can't parse, please, rephrase. I see that error quite often in your English. Looks, like you don't know, that verbs can be adjectives. Lesson #345223. Verbs as adjectives. http://www.rhlschool.com/eng4n7.htm Today we will learn how to say, that an object has a quality created/added by a verb. In other words, how to turn a verb into an adjective. 1. Many kind carpenters offered to repair the *broken porch*. Not "to repair the porch is broken". 2. My father prefers to drink *filtered spring water*. Not "to drink spring water is filtered". 3. This isn’t chocolate ice cream; it’s *frozen chocolate milk*! Not "it is chocolate milk is frozen". 4. The *fallen leaves* covered the new driveway. Not "The leaves is fallen covered the new driveway". 5. She was happy to find the *translated version* of the book. Not "to find the version is translated of the book". 6. The sleeping dog’s snoring was louder than a *freight train*. Not "louder than a train is freight". 7. We pushed our way through the *newly driven snow*. Not "through the snow is driven newly". 8. I’d rather eat at a recently *inspected restaurant*. Not "at a restaurant is inspected recently". 9. Are you just hoping it will happen or is it *a done deal*? Not "is it a deal is done". 10. Sadly, as she aged, he became just another *forgotten name*. Not "became just another name is forgotten". > that is executed in on_replace trigger. Each ck constraint VDBE code is > consists of 2. 'is consists' - ??? 'Consist' is a verb, you can't say that. > 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 3. Why do you abort a transaction because of just one bad DML operation? A failed operation should not lead to rollback of the entire transaction. > constraint error is handled as diag message. > Each ck constraint use own on_replace trigger. 4. It is too expensive, triggers are virtual functions. Please, call them all from one. Also, it is necessary to be able to turn them off - you will just remove this single trigger. > > Needed for #3691 > --- > diff --git a/src/box/alter.cc b/src/box/alter.cc > index e65c49d14..746ce62f6 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -1410,14 +1410,29 @@ void > RebuildCkConstraints::alter(struct alter_space *alter) > { > rlist_swap(&alter->new_space->ck_constraint, &ck_constraint); > + struct ck_constraint *ck; > + rlist_foreach_entry(ck, &alter->old_space->ck_constraint, link) > + trigger_clear(&ck->trigger); > rlist_swap(&ck_constraint, &alter->old_space->ck_constraint); > + rlist_foreach_entry(ck, &alter->new_space->ck_constraint, link) { > + /** > + * Triggers would be swapped later on > + * alter_space_do. > + */ 5. When 'later'? This function (alter) is called already in alter_space_do(). > + trigger_add(&alter->old_space->on_replace, &ck->trigger); > + } > } > @@ -1435,6 +1450,35 @@ RebuildCkConstraints::~RebuildCkConstraints() > } > } > > +/** > + * Move CK constraints from old space to the new one. > + * Despite RebuildCkConstraints, this operation doesn't perform > + * objects rebuild. 6. 'Despite' does not mean the thing you wanted to say here. Please, rephrase. > + * This may be used in scenarios where space > + * format doesn't change i.e. in alter index or space trim > + * requests. > + */ > +class MoveCkConstraints: public AlterSpaceOp > +{ > +public: > + MoveCkConstraints(struct alter_space *alter) : AlterSpaceOp(alter) {} > + virtual void alter(struct alter_space *alter); > + virtual void rollback(struct alter_space *alter); > +}; > /* }}} */ > > /** > @@ -2145,8 +2189,8 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) > * old space. > */ > alter_space_move_indexes(alter, iid + 1, old_space->index_id_max + 1); > + (void) new MoveCkConstraints(alter); > /* Add an op to update schema_version on commit. */ > - (void) new RebuildCkConstraints(alter); > (void) new UpdateSchemaVersion(alter); > alter_space_do(txn, alter); > scoped_guard.is_active = false; > @@ -2214,7 +2258,7 @@ on_replace_dd_truncate(struct trigger * /* trigger */, void *event) > (void) new TruncateIndex(alter, old_index->def->iid); > } > > - (void) new RebuildCkConstraints(alter); > + (void) new MoveCkConstraints(alter); 7. Why is not MoveCkConstraints in the previous patch? > alter_space_do(txn, alter); > scoped_guard.is_active = false; > } > diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c > index db012837b..43798be76 100644 > --- a/src/box/ck_constraint.c > +++ b/src/box/ck_constraint.c > @@ -29,10 +29,15 @@ > * SUCH DAMAGE. > */ > #include "box/session.h" > +#include "bind.h" > #include "ck_constraint.h" > #include "errcode.h" > +#include "schema.h" > +#include "session.h" > #include "sql.h" > #include "sql/sqlInt.h" > +#include "sql/vdbeInt.h" > +#include "tuple.h" > > const char *ck_constraint_language_strs[] = {"SQL"}; 8. Do not see that on the branch. > > @@ -57,6 +62,138 @@ ck_constraint_resolve_field_names(struct Expr *expr, > return rc; > } > > +/** > + * Create VDBE machine for ck constraint by given definition and > + * expression AST. The generated instructions consist of > + * prologue code that maps tuple fields via bindings and ck > + * 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 > + * error description. > + * @param expr Check constraint expression AST is built for 9. When you say 'AST is built ...', I can't understand, how this sentence relates to expr. If you wanted to describe 'expr', then remove 'is'. See comment 1. > + * given @ck_constraint_def, see for 10. Doxygen does not have '@ck_constraint_def' command. Please, see the list of commands and do not use not existing. Here you should use '@a ck_constraint'. > + * (sql_expr_compile + > + * 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 space_def *space_def) > +{ > + 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"); > + return NULL; > + } > + /* > + * Generate a prologue code that introduces variables to > + * bind tuple fields there before execution. > + */ > + uint32_t field_count = space_def->field_count; > + int bind_tuple_reg = sqlGetTempRange(&parser, field_count); > + for (uint32_t i = 0; i < field_count; i++) { > + sqlVdbeAddOp2(v, OP_Variable, ++parser.nVar, > + bind_tuple_reg + i); > + } > + /* Generate ck constraint test code. */ > + vdbe_emit_ck_constraint(&parser, expr, ck_constraint_def->name, > + ck_constraint_def->expr_str, bind_tuple_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; > +} > + > +/** > + * Run bytecode implementing check constraint on new tuple > + * before insert or replace in space space_def. 11. 'space space_def' ??? > + * @param ck_constraint Check constraint to run. > + * @param space_def The space definition of the space this check > + * constraint is constructed for. 12. There is no parameter named 'space_def'. > + * @param new_tuple The tuple to be inserted in space. > + * @retval 0 if check constraint test is passed, -1 otherwise. 13. As I said earlier, multiple times, @retval is not designed to describe multiple values. @retval means 'return value', one value. Not 'valueS'. Use one @retval for each single value, or use @return, which does not require an argument and is used to describe return values in general. > + */ > +static int > +ck_constraint_program_run(struct ck_constraint *ck_constraint, > + const char *new_tuple) > +{ > + /* > + * Prepare parameters for checks->stmt execution: 14. What is 'checks' here? I do not see such a variable here to dereference it. > + * Map new tuple fields to Vdbe memory variables in range: > + * [1, field_count] > + */ > + struct space *space = space_by_id(ck_constraint->space_id); > + assert(space != NULL); > + /* > + * When last format fields are nullable, they are > + * 'optional' i.e. they may not be present in the tuple. > + */ > + 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, i + 1, &new_tuple) != 0 || > + sql_bind_column(ck_constraint->stmt, &bind, i + 1) != 0) { > + diag_set(ClientError, ER_CK_CONSTRAINT_FAILED, > + ck_constraint->def->name, > + ck_constraint->def->expr_str); > + return -1; > + } > + } > + /* Checks VDBE can't expire, reset expired flag and go. */ > + struct Vdbe *v = (struct Vdbe *) ck_constraint->stmt; > + v->expired = 0; > + while (sql_step(ck_constraint->stmt) == SQL_ROW) {} 15. Why do you iterate? Only DQL has multiple steps, because it returns tuples. DML, DDL and any other statement can't return multiple tuples. > + /* > + * Get VDBE execution state and reset VM to run it > + * next time. > + */ > + return sql_reset(ck_constraint->stmt) != SQL_OK ? -1 : 0; > +} > + > +/** > + * Ck constraint trigger function. It ss expected to be executed 16. 'ss' -> 'is'. > + * in space::on_replace trigger. > + * > + * 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/sql/insert.c b/src/box/sql/insert.c > index 7b25d18b3..8409ab343 100644 > --- a/src/box/sql/insert.c > +++ b/src/box/sql/insert.c > @@ -926,35 +901,6 @@ vdbe_emit_constraint_checks(struct Parse *parse_context, struct space *space, > unreachable(); > } > } > - /* > - * For CHECK constraint and for INSERT/UPDATE conflict > - * action DEFAULT and ABORT in fact has the same meaning. > - */ > - if (on_conflict == ON_CONFLICT_ACTION_DEFAULT) > - on_conflict = ON_CONFLICT_ACTION_ABORT; > - /* Test all CHECK constraints. */ > - enum on_conflict_action on_conflict_check = on_conflict; > - if (on_conflict == ON_CONFLICT_ACTION_REPLACE) > - on_conflict_check = ON_CONFLICT_ACTION_ABORT; > - if (!rlist_empty(&space->ck_constraint)) > - parse_context->ckBase = new_tuple_reg; > - struct ck_constraint *ck_constraint; > - rlist_foreach_entry(ck_constraint, &space->ck_constraint, link) { > - struct Expr *expr = ck_constraint->expr; > - if (is_update && checkConstraintUnchanged(expr, upd_cols) != 0) > - continue; > - int all_ok = sqlVdbeMakeLabel(v); > - sqlExprIfTrue(parse_context, expr, all_ok, SQL_JUMPIFNULL); > - if (on_conflict == ON_CONFLICT_ACTION_IGNORE) { > - sqlVdbeGoto(v, ignore_label); > - } else { > - char *name = ck_constraint->def->name; > - sqlHaltConstraint(parse_context, SQL_CONSTRAINT_CHECK, 17. SQL_CONSTRAINT_CHECK is unused now. P5_ConstraintCheck too. > - on_conflict_check, name, P4_TRANSIENT, > - P5_ConstraintCheck); > - } > - sqlVdbeResolveLabel(v, all_ok); > - } > sql_emit_table_types(v, space->def, new_tuple_reg); > /* > * Other actions except for REPLACE and UPDATE OR IGNORE > diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h > index 0753705ec..d353d7906 100644 > --- a/src/box/sql/sqlInt.h > +++ b/src/box/sql/sqlInt.h > @@ -3894,6 +3905,21 @@ 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 name Check constraint name to raise error. > + * @param new_tuple_reg The first ck_constraint::stmt VDBE > + * register of the range > + * space_def::field_count representing a > + * new tuple to be inserted. > + */ > +void > +vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr, > + const char *name, const char *expr_str, 18. expr_str is not described. > + int new_tuple_reg); > /** > * This routine generates code to finish the INSERT or UPDATE > * operation that was started by a prior call to