[tarantool-patches] Re: [PATCH v5 5/6] box: run check constraint tests on space alter

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

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 <stdint.h>
> +#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);