[tarantool-patches] Re: [PATCH v4 2/4] box: run check constraint tests on space alter
Vladislav Shpilevoy
v.shpilevoy at tarantool.org
Sun May 19 19:02:07 MSK 2019
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
More information about the Tarantool-patches
mailing list