[tarantool-patches] Re: [PATCH v4 2/4] box: run check constraint tests on space alter

[Vladislav Shpilevoy <v.shpilevoy@tarantool.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