[tarantool-patches] Re: [PATCH v2 9/9] sql: run check constraint tests on space alter
n.pettik
korablev at tarantool.org
Tue Apr 2 17:14:04 MSK 2019
> On 26 Mar 2019, at 13:59, Kirill Shcherbatov <kshcherbatov at 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.
More information about the Tarantool-patches
mailing list