[tarantool-patches] Re: [PATCH v2 9/9] sql: run check constraint tests on space alter

["n.pettik" <korablev@tarantool.org>]

> On 26 Mar 2019, at 13:59, Kirill Shcherbatov <kshcherbatov@tarantool.org> wrote:
> 
> Introduced reusable pre-compiled VDBE programs for ck constraints
> and space trigger to fire checks on insert and update operations.

As usual, extremely brief description of vital feature.
Fill commit message with details of chosen approach.

> Closes #3691
> 
> @TarantoolBot document
> Title: check constraint for LUA space
> 
> Now it is possible to create check constraints for LUA spaces:
> insert the tuple {<CONSTRAINT NAME>, <DST SPACE ID>, <EXPRESSION STRING>} in
> the _ck_constraint space to create new check constraint.
> 
> Example:
> s = box.schema.create_space('test')
> s:create_index('pk')
> format = {{'X', 'unsigned'}, {'Y', 'unsigned'}}
> s:format(format)

Mention that format is required condition to use check
constraints: otherwise, name of field can’t be resolved.
Now it leads to assertion fault:

s = box.schema.create_space('test’)
s:create_index('pk’)
box.space._ck_constraint:insert({'physics', s.id, 'X<Y’})
Assertion failed: (space_def->field_count > 0), function lookupName, file tarantool/src/box/sql/resolve.c, line 242.

> box.space._ck_constraint:insert({'physics', s.id, 'X<Y'})
> box.space.test:insert({6, 5})
> - error: 'Check constraint failed: physics’

To finish this patch-set I suggest to add Lua-wrapper to create
check constraints on any space using NoSQL interface and introduce
ALTER TABLE ADD CONSTRAINT CHECK().
Last issue you can implement in a separate patch or delegate its
implementation to smb else.

> ---
> src/box/alter.cc         |  21 +++++-
> src/box/ck_constraint.c  | 142 ++++++++++++++++++++++++++++++++++++++-
> src/box/ck_constraint.h  |  24 +++++--
> src/box/sql.c            |   1 +
> src/box/sql/insert.c     |  94 ++++++--------------------
> src/box/sql/sqlInt.h     |  25 +++++++
> src/box/sql/vdbeapi.c    |   8 ---
> test/sql/checks.result   |  20 ++++++
> test/sql/checks.test.lua |   5 ++
> 9 files changed, 248 insertions(+), 92 deletions(-)
> 
> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index 9aa5e3653..b6d1c8537 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -1379,6 +1379,11 @@ BuildCkConstraints::alter(struct alter_space *alter)
> {
> 	rlist_swap(&alter->new_space->ck_constraint, &ck_constraint);
> 	rlist_swap(&ck_constraint, &alter->old_space->ck_constraint);
> +	struct ck_constraint *ck;
> +	rlist_foreach_entry(ck, &ck_constraint, link)
> +		trigger_clear(&ck->trigger);
> +	rlist_foreach_entry(ck, &alter->new_space->ck_constraint, link)
> +		trigger_add(&alter->new_space->before_replace, &ck->trigger);
> }
> 
> void
> @@ -1386,6 +1391,11 @@ BuildCkConstraints::rollback(struct alter_space *alter)
> {
> 	rlist_swap(&alter->old_space->ck_constraint, &ck_constraint);
> 	rlist_swap(&ck_constraint, &alter->new_space->ck_constraint);
> +	struct ck_constraint *ck;
> +	rlist_foreach_entry(ck, &ck_constraint, link)
> +		trigger_clear(&ck->trigger);
> +	rlist_foreach_entry(ck, &alter->old_space->ck_constraint, link)
> +		trigger_add(&alter->old_space->before_replace, &ck->trigger);
> }
> 
> BuildCkConstraints::~BuildCkConstraints()
> 
> diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
> index 110098efc..c2e8547f1 100644
> --- a/src/box/ck_constraint.c
> +++ b/src/box/ck_constraint.c
> @@ -29,11 +29,16 @@
>  * SUCH DAMAGE.
>  */
> #include <assert.h>
> +#include "bind.h"
> #include "ck_constraint.h"
> #include "errcode.h"
> +#include "session.h"
> +#include "schema.h"
> #include "small/rlist.h"
> +#include "tuple.h"
> #include "sql.h"
> #include "sql/sqlInt.h"
> +#include "sql/vdbeInt.h"
> 
> /**
>  * Resolve space_def references for check constraint via AST
> @@ -84,6 +89,130 @@ ck_constraint_def_create(struct ck_constraint_def *ck_constraint_def,
> 	rlist_create(&ck_constraint_def->link);
> }
> 
> +/**
> + * Compile constraint check subroutine.
> + * @param ck_constraint Check constraint to compile.
> + * @param expr Check constraint expression AST is built for
> + *             ck_constraint->def.

Can’t understand this description.

> + * @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.
> + */
> +int

This function is used only in ck_constraint.c, let’s make it static.

> +ck_constraint_test_compile(struct ck_constraint *ck_constraint,

I would call it “ck_constraint_program_compile” or “ck_constraint_routine_compile”
or “ck_constraint_bytecode_compile”.

> +			   struct Expr *expr, const struct space_def *space_def)
> +{
> +	int rc = -1;

Remove this rc pls. Diff:

diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c
index c2e8547f1..6f54e35e1 100644
--- a/src/box/ck_constraint.c
+++ b/src/box/ck_constraint.c
@@ -103,15 +103,13 @@ int
 ck_constraint_test_compile(struct ck_constraint *ck_constraint,
                           struct Expr *expr, const struct space_def *space_def)
 {
-       int rc = -1;
        assert(ck_constraint->space_id == space_def->id);
        struct Parse parser;
        sql_parser_create(&parser, sql_get());
        struct Vdbe *v = sqlGetVdbe(&parser);
        if (v == NULL) {
-               diag_set(OutOfMemory, sizeof(struct Vdbe),
-                        "sqlGetVdbe", "vdbe");
-               goto end;
+               diag_set(OutOfMemory, sizeof(struct Vdbe), "sqlDbMalloc", "v");
+               return -1;
        }
 
        /* Compile VDBE with default sql parameters. */
@@ -138,16 +136,14 @@ ck_constraint_test_compile(struct ck_constraint *ck_constraint,
                diag_set(ClientError, ER_CREATE_CK_CONSTRAINT,
                         ck_constraint->def->name,
                         "can not compile expression");
-               goto end;
+               return -1;
        }
        sql_parser_destroy(&parser);
 
        /* Restore original sql flags for user_session.  */
        user_session->sql_flags = sql_flags;
        ck_constraint->stmt = (struct sql_stmt *)v;
-       rc = 0;
-end:
-       return rc;
+       return 0;
 }

> +	assert(ck_constraint->space_id == space_def->id);
> +	struct Parse parser;
> +	sql_parser_create(&parser, sql_get());
> +	struct Vdbe *v = sqlGetVdbe(&parser);
> +	if (v == NULL) {
> +		diag_set(OutOfMemory, sizeof(struct Vdbe),
> +			 "sqlGetVdbe", "vdbe");
> +		goto end;
> +	}
> +
> +	/* Compile VDBE with default sql parameters. */
> +	struct session *user_session = current_session();
> +	uint32_t sql_flags = user_session->sql_flags;
> +	user_session->sql_flags = default_flags;
> +
> +	/*
> +	 * Generate a prologue code to bind variable new_tuple_var
> +	 * to new_tuple_reg.
> +	 */

This comment explains nothing.

> +	uint32_t field_count = space_def->field_count;
> +	int new_tuple_reg = sqlGetTempRange(&parser, field_count);
> +	struct Expr bind = {.op = TK_VARIABLE, .u.zToken = "?"};
> +	ck_constraint->new_tuple_var = parser.nVar + 1;
> +	for (uint32_t i = 0; i < field_count; i++) {
> +		sqlExprAssignVarNumber(&parser, &bind, 1);
> +		sqlExprCodeTarget(&parser, &bind, new_tuple_reg + i);
> +	}
> +	vdbe_emit_ck_constraint(&parser, expr, ck_constraint->def->name,
> +				new_tuple_reg);
> +	sql_finish_coding(&parser);

Do we need to call this function at all?

> +	if (parser.is_aborted) {
> +		diag_set(ClientError, ER_CREATE_CK_CONSTRAINT,
> +			 ck_constraint->def->name,
> +			 "can not compile expression”);

This error will re-set original parsing error. I suggest to
concatenate them.

> +/**
> + * Perform ck constraint checks with new tuple data new_tuple_raw
> + * before insert or replace in space space_def.

These variable names don’t say anything.

> + * @param ck_constraint Check constraint to test.

"To perform" or "to execute”.

> + * @param space_def The space definition of the space this check
> + *                  constraint is constructed for.
> + * @param new_tuple_raw The tuple to be inserted in space.
> + * @retval 0 if check constraint test is passed, -1 otherwise.
> + */
> +static int
> +ck_constraint_test(struct ck_constraint *ck_constraint,
> +		   struct space_def *space_def, const char *new_tuple_raw)

-> ck_constraint_execute/ck_constraint_run/ck_constraint_fire etc.

> +{
> +	assert(new_tuple_raw != NULL);
> +	/*
> +	 * Prepare parameters for checks->stmt execution:
> +	 * Unpacked new tuple fields mapped to Vdbe memory from
> +	 * variables from range:
> +	 * [new_tuple_var,new_tuple_var+field_count]
> +	 */
> +	mp_decode_array(&new_tuple_raw);
> +	/* Reset VDBE to make new bindings. */
> +	sql_reset(ck_constraint->stmt);

This function returns error code, don’t ignore it.

> +	for (uint32_t i = 0; i < space_def->field_count; i++) {
> +		struct sql_bind bind;
> +		if (sql_bind_decode(&bind, ck_constraint->new_tuple_var + i,
> +				    &new_tuple_raw) != 0)
> +			return -1;
> +		if (sql_bind_column(ck_constraint->stmt, &bind,
> +				    ck_constraint->new_tuple_var + i) != 0)
> +			return -1;
> +	}
> +	/* Checks VDBE can't expire, reset expired flag & Burn. */

What does mean “& Burn” ? Not “Checks VDBE” but
“bytecode implementing check constraint”.

> +	struct Vdbe *v = (struct Vdbe *)ck_constraint->stmt;
> +	v->expired = 0;
> +	int rc;
> +	while ((rc = sql_step(ck_constraint->stmt)) == SQL_ROW) {}
> +	if (v->rc != SQL_DONE && v->rc != SQL_TARANTOOL_ERROR)
> +		diag_set(ClientError, ER_SQL, v->zErrMsg);
> +	return rc == SQL_DONE ? 0 : -1;
> +}
> +
> +/**
> + * Trigger routine executing ck constraint check on space
> + * insert and replace.

Sounds like a set of random words :)) Re-phrase somehow pls.

> +static void
> +ck_constraint_space_trigger(struct trigger *trigger, void *event)

ck_constraint_before_replace_trigger would sound better IMHO.

> +{
> +	struct ck_constraint *ck_constraint =
> +		(struct ck_constraint *)trigger->data;
> +	struct space *space = space_by_id(ck_constraint->space_id);
> +	assert(space != NULL);
> +	struct txn *txn = (struct txn *) event;
> +	struct txn_stmt *stmt = txn_current_stmt(txn);
> +	struct tuple *new_tuple = stmt->new_tuple;
> +	if (stmt == NULL || new_tuple == NULL)
> +		return;
> +	if (ck_constraint_test(ck_constraint, space->def,
> +			       tuple_data(new_tuple)) != 0)
> +		diag_raise();
> +}
> +
> struct ck_constraint *
> ck_constraint_new(const struct ck_constraint_def *ck_constraint_def,
> 		  struct space_def *space_def)
> @@ -110,6 +239,8 @@ ck_constraint_new(const struct ck_constraint_def *ck_constraint_def,
> 	ck_constraint_def_create(ck_constraint->def, ck_constraint_def->name,
> 				 ck_constraint_name_len,
> 				 ck_constraint_def->expr_str, expr_str_len);
> +	trigger_create(&ck_constraint->trigger, ck_constraint_space_trigger,
> +		       ck_constraint, NULL);
> 	struct Expr *expr =
> 		sql_expr_compile(sql_get(), ck_constraint_def->expr_str,
> 				 expr_str_len);
> @@ -120,18 +251,23 @@ ck_constraint_new(const struct ck_constraint_def *ck_constraint_def,
> 			 box_error_message(box_error_last()));
> 		goto error;
> 	}
> -	ck_constraint->expr = expr;
> +	if (ck_constraint_test_compile(ck_constraint, expr, space_def) != 0)
> +		goto error;
> 
> +end:
> +	sql_expr_delete(sql_get(), expr, false);
> 	return ck_constraint;
> error:
> 	ck_constraint_delete(ck_constraint);
> -	return NULL;
> +	ck_constraint = NULL;
> +	goto end;

Trivial refactoring:

@@ -254,13 +250,12 @@ ck_constraint_new(const struct ck_constraint_def *ck_constraint_def,
        if (ck_constraint_test_compile(ck_constraint, expr, space_def) != 0)
                goto error;
 
-end:
        sql_expr_delete(sql_get(), expr, false);
        return ck_constraint;
 error:
        ck_constraint_delete(ck_constraint);
-       ck_constraint = NULL;
-       goto end;
+       sql_expr_delete(sql_get(), expr, false);
+       return NULL;
 }

> diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h
> index 02aa525ce..77b1a9bed 100644
> --- a/src/box/ck_constraint.h
> +++ b/src/box/ck_constraint.h
> @@ -32,6 +32,7 @@
>  */
> 
> #include <stdint.h>
> +#include "trigger.h"
> #include "small/rlist.h”

You can remove this header: trigger.h includes it as well.

> 
> #if defined(__cplusplus)
> @@ -40,6 +41,7 @@ extern "C" {
> 
> struct space;
> struct space_def;
> +struct sql_stmt;
> struct Expr;
> 
> /**
> @@ -72,17 +74,29 @@ struct ck_constraint {
> 	 */
> 	struct ck_constraint_def *def;
> 	/**
> -	 * The check constraint expression AST is built for
> -	 * ck_constraint::def::expr_str with sql_expr_compile
> -	 * and resolved with sqlResolveExprNames for
> -	 * space with space[ck_constraint::space_id] definition.
> +	 * Precompiled reusable VDBE program for proceeding ck

proceeding -> processing

ck constraint checks -> check constraints

> +	 * constraint checks and setting bad exitcode and error
> +	 * message when ck condition unsatisfied.
> +	 * Program rely on new_tuple_var parameter to be binded

binded in -> bound to 

> +	 * in the VDBE memory before run.
> 	 */
> -	struct Expr *expr;
> +	struct sql_stmt *stmt;
> 	/**
> 	 * The id of the space this check constraint is
> 	 * built for.
> 	 */
> 	uint32_t space_id;
> +	/**
> +	 * The first ck_constraint::stmt VDBE variable of the
> +	 * range space[ck_constraint::space_id]->def->field_count
> +	 * representing a new tuple to be inserted.
> +	 */
> +	int new_tuple_var;
> +	/**
> +	 * Trigger object executing check constraint on space
> +	 * insert and replace.
> +	 */
> +	struct trigger trigger;
> 	/**
> 	 * Organize check constraint structs into linked list
> 	 * with space::ck_constraint.
> diff --git a/src/box/sql.c b/src/box/sql.c
> index fc469126e..912b24bf0 100644
> --- a/src/box/sql.c
> +++ b/src/box/sql.c
> @@ -29,6 +29,7 @@
>  * SUCH DAMAGE.
>  */
> #include <assert.h>
> +#include "bind.h"
> #include "field_def.h"
> #include "cfg.h"
> #include "sql.h"
> diff --git a/src/box/sql/insert.c b/src/box/sql/insert.c
> index 2fe74a027..3caad3c24 100644
> --- a/src/box/sql/insert.c
> +++ b/src/box/sql/insert.c
> @@ -798,51 +798,26 @@ sqlInsert(Parse * pParse,	/* Parser context */
> 	sqlDbFree(db, aRegIdx);
> }
> 
> -/*
> - * Meanings of bits in of pWalker->eCode for checkConstraintUnchanged()
> - */
> -#define CKCNSTRNT_COLUMN   0x01	/* CHECK constraint uses a changing column */
> -
> -/* This is the Walker callback from checkConstraintUnchanged().  Set
> - * bit 0x01 of pWalker->eCode if
> - * pWalker->eCode to 0 if this expression node references any of the
> - * columns that are being modifed by an UPDATE statement.
> - */
> -static int
> -checkConstraintExprNode(Walker * pWalker, Expr * pExpr)
> -{
> -	if (pExpr->op == TK_COLUMN) {
> -		assert(pExpr->iColumn >= 0 || pExpr->iColumn == -1);
> -		if (pExpr->iColumn >= 0) {
> -			if (pWalker->u.aiCol[pExpr->iColumn] >= 0) {
> -				pWalker->eCode |= CKCNSTRNT_COLUMN;
> -			}
> -		}
> -	}
> -	return WRC_Continue;
> -}
> -
> -/*
> - * pExpr is a CHECK constraint on a row that is being UPDATE-ed.  The
> - * only columns that are modified by the UPDATE are those for which
> - * aiChng[i]>=0.
> - *
> - * Return true if CHECK constraint pExpr does not use any of the
> - * changing columns.  In other words, return true if this CHECK constraint
> - * can be skipped when validating the new row in the UPDATE statement.
> - */
> -static int
> -checkConstraintUnchanged(Expr * pExpr, int *aiChng)
> +void
> +vdbe_emit_ck_constraint(struct Parse *parser, struct Expr *expr,
> +			const char *name, int new_tuple_reg)
> {
> -	Walker w;
> -	memset(&w, 0, sizeof(w));
> -	w.eCode = 0;
> -	w.xExprCallback = checkConstraintExprNode;
> -	w.u.aiCol = aiChng;
> -	sqlWalkExpr(&w, pExpr);
> -	testcase(w.eCode == 0);
> -	testcase(w.eCode == CKCNSTRNT_COLUMN);
> -	return !w.eCode;
> +	parser->ckBase = new_tuple_reg;
> +	struct Vdbe *v = sqlGetVdbe(parser);
> +	const char *ck_constraint_name = sqlDbStrDup(parser->db, name);

Where's this pointer released?

> +	VdbeNoopComment((v, "BEGIN: ck constraint %s test", name));
> +	/* Skip check when it is turned off. */

When it can be turned off?

> +	int all_is_ok = sqlVdbeMakeLabel(v);

all_is_ok -> check_is_passed

> +	sqlExprIfTrue(parser, expr, all_is_ok, SQL_JUMPIFNULL);
> +	sqlMayAbort(parser);
> +	const char *fmt = tnt_errcode_desc(ER_CK_CONSTRAINT_FAILED);
> +	const char *error_msg = tt_sprintf(fmt, ck_constraint_name);
> +	sqlVdbeAddOp4(v, OP_Halt, SQL_TARANTOOL_ERROR,
> +		      ON_CONFLICT_ACTION_ABORT, 0,
> +		      sqlDbStrDup(parser->db, error_msg), P4_DYNAMIC);
> +	sqlVdbeChangeP5(v, ER_CK_CONSTRAINT_FAILED);
> +	VdbeNoopComment((v, "END: ck constraint %s test", name));
> +	sqlVdbeResolveLabel(v, all_is_ok);
> }
> 
> 
> diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua
> index 9a7e5faf4..0e6c990c9 100644
> --- a/test/sql/checks.test.lua
> +++ b/test/sql/checks.test.lua
> @@ -27,9 +27,11 @@ box.space._ck_constraint:insert({'CK_CONSTRAINT_01', 513, 'X<5'})
> box.space._ck_constraint:count({})
> 
> box.sql.execute("INSERT INTO \"test\" VALUES(5);")
> +box.space.test:insert({5})
> box.space._ck_constraint:replace({'CK_CONSTRAINT_01', 513, 'X<=5'})
> box.sql.execute("INSERT INTO \"test\" VALUES(5);")
> box.sql.execute("INSERT INTO \"test\" VALUES(6);")
> +box.space.test:insert({6})
> -- Can't drop table with check constraints.
> box.space.test:delete({5})
> box.space.test.index.pk:drop()
> @@ -41,8 +43,11 @@ box.space.test:drop()
> box.sql.execute("CREATE TABLE t1(x INTEGER CONSTRAINT ONE CHECK( x<5 ), y REAL CONSTRAINT TWO CHECK( y>x ), z INTEGER PRIMARY KEY);")
> box.space._ck_constraint:count()
> box.sql.execute("INSERT INTO t1 VALUES (7, 1, 1)")
> +box.space.T1:insert({7, 1, 1})
> box.sql.execute("INSERT INTO t1 VALUES (2, 1, 1)")
> +box.space.T1:insert({2, 1, 1})
> box.sql.execute("INSERT INTO t1 VALUES (2, 4, 1)")
> +box.space.T1:update({1}, {{'+', 1, 5}})
> box.sql.execute("DROP TABLE t1”)

Please, add descent set of tests verifying that check constraints
work in any possible scenario.
Make sure that check occurs before replace action.