* Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 1/4] box: add an ability to disable CK constraints [not found] ` <8232b0466f3878280a9ad35cb08f437610a36486.1570539526.git.kshcherbatov@tarantool.org> @ 2019-10-14 16:49 ` Nikita Pettik 2019-10-15 11:13 ` [Tarantool-patches] [tarantool-patches] " Kirill Shcherbatov 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2019-10-14 16:49 UTC (permalink / raw) To: tarantool-patches; +Cc: tarantool-patches On 08 Oct 16:02, Kirill Shcherbatov wrote: > Now it is possible to disable and enable ck constraints in LUA. > This option is persistent. All ck constraints are constructed > in enabled state when Tarantool is configured. This ability > may be useful when all information processed is (by some reason) -> when processed data is verified and constraints validation is not required. For instance, during casual recovery process there's no need to provide any checks since data is assumed to be consistent. Also explain why we have to store this option. > trusted and ck constraints validations are not required. > > Part of #4244 > --- > src/box/alter.cc | 31 ++++++-- > src/box/bootstrap.snap | Bin 5934 -> 5944 bytes > src/box/ck_constraint.c | 7 +- > src/box/ck_constraint.h | 8 +- > src/box/lua/schema.lua | 16 +++- > src/box/lua/space.cc | 3 + > src/box/lua/upgrade.lua | 13 ++++ > src/box/schema_def.h | 1 + > src/box/sql/build.c | 11 +-- > test/box-py/bootstrap.result | 3 +- > test/box/access_misc.result | 137 ++++++++++++++++++----------------- > test/sql/checks.result | 108 ++++++++++++++++++++++----- > test/sql/checks.test.lua | 57 ++++++++++----- > test/sql/errinj.result | 12 +-- > test/sql/errinj.test.lua | 12 +-- > 15 files changed, 286 insertions(+), 133 deletions(-) To be honest, I doubt that 'is_enable' option should be persisted.. At least, we can assume that it is always 'on' by default, so that save user from having to specify this option each time. > diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap > index 4c9aea7f50f8ac86be32ca9f126fea9a3d2d182f..59f6cc16bb48841fa99b4e13590b8e0677433b35 100644 > GIT binary patch > delta 4861 > > diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c > index 1cde27022..fafa7be12 100644 > --- a/src/box/ck_constraint.c > +++ b/src/box/ck_constraint.c > @@ -223,6 +225,7 @@ ck_constraint_new(struct ck_constraint_def *ck_constraint_def, > } > ck_constraint->def = NULL; > ck_constraint->stmt = NULL; > + Extra diff. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints 2019-10-14 16:49 ` [Tarantool-patches] [tarantool-patches] [PATCH v4 1/4] box: add an ability to disable CK constraints Nikita Pettik @ 2019-10-15 11:13 ` Kirill Shcherbatov 2019-10-15 21:47 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Kirill Shcherbatov @ 2019-10-15 11:13 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik; +Cc: tarantool-patches > -> when processed data is verified and constraints validation > is not required. For instance, during casual recovery process there's > no need to provide any checks since data is assumed to be consistent. Applied. > Also explain why we have to store this option. > To be honest, I doubt that 'is_enable' option should be persisted.. > At least, we can assume that it is always 'on' by default, so that > save user from having to specify this option each time. Verbally discussed. To implement a behavior that Oracle users are familiar with. It was a Kostya's point of view and it is already implemented. > Extra diff. Fixed. ===================================================== Now it is possible to disable and enable ck constraints in LUA. This option is persistent. All ck constraints are constructed in enabled state when Tarantool is configured. This ability may be usefulwhen processed data is verified and constraints validation is not required. For instance, during casual recovery process there's no need to provide any checks since data is assumed to be consistent. Part of #4244 --- src/box/alter.cc | 31 ++++++-- src/box/bootstrap.snap | Bin 5934 -> 5944 bytes src/box/ck_constraint.c | 6 +- src/box/ck_constraint.h | 8 +- src/box/lua/schema.lua | 16 +++- src/box/lua/space.cc | 3 + src/box/lua/upgrade.lua | 13 ++++ src/box/schema_def.h | 1 + src/box/sql/build.c | 11 +-- test/box-py/bootstrap.result | 3 +- test/box/access_misc.result | 137 ++++++++++++++++++----------------- test/sql/checks.result | 108 ++++++++++++++++++++++----- test/sql/checks.test.lua | 57 ++++++++++----- test/sql/errinj.result | 12 +-- test/sql/errinj.test.lua | 12 +-- 15 files changed, 285 insertions(+), 133 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index bb3686d6e..f8f1802d0 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -4595,9 +4595,12 @@ ck_constraint_def_new_from_tuple(struct tuple *tuple) const char *expr_str = tuple_field_str_xc(tuple, BOX_CK_CONSTRAINT_FIELD_CODE, &expr_str_len); + bool is_enabled = + tuple_field_count(tuple) <= BOX_CK_CONSTRAINT_FIELD_IS_ENABLED || + tuple_field_bool_xc(tuple, BOX_CK_CONSTRAINT_FIELD_IS_ENABLED); struct ck_constraint_def *ck_def = ck_constraint_def_new(name, name_len, expr_str, expr_str_len, - space_id, language); + space_id, language, is_enabled); if (ck_def == NULL) diag_raise(); return ck_def; @@ -4697,6 +4700,26 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) auto ck_def_guard = make_scoped_guard([=] { ck_constraint_def_delete(ck_def); }); + /* + * A corner case: enabling/disabling an existent + * ck constraint doesn't require the object + * rebuilding. + */ + const char *name = ck_def->name; + struct ck_constraint *old_ck_constraint = + space_ck_constraint_by_name(space, name, strlen(name)); + if (old_ck_constraint != NULL) { + struct ck_constraint_def *old_def = + old_ck_constraint->def; + assert(old_def->space_id == ck_def->space_id); + assert(strcmp(old_def->name, ck_def->name) == 0); + if (old_def->language == ck_def->language && + strcmp(old_def->expr_str, ck_def->expr_str) == 0) { + old_def->is_enabled = ck_def->is_enabled; + trigger_run_xc(&on_alter_space, space); + return; + } + } /* * FIXME: Ck constraint creation on non-empty * space is not implemented yet. @@ -4704,8 +4727,7 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) struct index *pk = space_index(space, 0); if (pk != NULL && index_size(pk) > 0) { tnt_raise(ClientError, ER_CREATE_CK_CONSTRAINT, - ck_def->name, - "referencing space must be empty"); + name, "referencing space must be empty"); } struct ck_constraint *new_ck_constraint = ck_constraint_new(ck_def, space->def); @@ -4715,9 +4737,6 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) auto ck_guard = make_scoped_guard([=] { ck_constraint_delete(new_ck_constraint); }); - const char *name = new_ck_constraint->def->name; - struct ck_constraint *old_ck_constraint = - space_ck_constraint_by_name(space, name, strlen(name)); if (old_ck_constraint != NULL) rlist_del_entry(old_ck_constraint, link); if (space_add_ck_constraint(space, new_ck_constraint) != 0) diff --git a/src/box/bootstrap.snap b/src/box/bootstrap.snap index 4c9aea7f50f8ac86be32ca9f126fea9a3d2d182f..59f6cc16bb48841fa99b4e13590b8e0677433b35 100644 GIT binary patch delta 4861 diff --git a/src/box/ck_constraint.c b/src/box/ck_constraint.c index 1cde27022..d21717f94 100644 --- a/src/box/ck_constraint.c +++ b/src/box/ck_constraint.c @@ -44,7 +44,7 @@ const char *ck_constraint_language_strs[] = {"SQL"}; struct ck_constraint_def * ck_constraint_def_new(const char *name, uint32_t name_len, const char *expr_str, uint32_t expr_str_len, uint32_t space_id, - enum ck_constraint_language language) + enum ck_constraint_language language, bool is_enabled) { uint32_t expr_str_offset; uint32_t ck_def_sz = ck_constraint_def_sizeof(name_len, expr_str_len, @@ -55,6 +55,7 @@ ck_constraint_def_new(const char *name, uint32_t name_len, const char *expr_str, diag_set(OutOfMemory, ck_def_sz, "malloc", "ck_def"); return NULL; } + ck_def->is_enabled = is_enabled; ck_def->expr_str = (char *)ck_def + expr_str_offset; ck_def->language = language; ck_def->space_id = space_id; @@ -201,7 +202,8 @@ ck_constraint_on_replace_trigger(struct trigger *trigger, void *event) struct ck_constraint *ck_constraint; rlist_foreach_entry(ck_constraint, &space->ck_constraint, link) { - if (ck_constraint_program_run(ck_constraint, field_ref) != 0) + if (ck_constraint->def->is_enabled && + ck_constraint_program_run(ck_constraint, field_ref) != 0) diag_raise(); } } diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h index f26f77a38..6761318d6 100644 --- a/src/box/ck_constraint.h +++ b/src/box/ck_constraint.h @@ -71,6 +71,11 @@ struct ck_constraint_def { * defined for. */ uint32_t space_id; + /** + * Per constraint option regulating its execution: it is + * disabled (set to false) contraint won't be fired. + */ + bool is_enabled; /** The language of ck constraint. */ enum ck_constraint_language language; /** @@ -140,6 +145,7 @@ ck_constraint_def_sizeof(uint32_t name_len, uint32_t expr_str_len, * @param expr_str_len The length of the @a expr string. * @param space_id The identifier of the target space. * @param language The language of the @a expr string. + * @param is_enabled Whether this ck constraint should be fired. * @retval not NULL Check constraint definition object pointer * on success. * @retval NULL Otherwise. The diag message is set. @@ -147,7 +153,7 @@ ck_constraint_def_sizeof(uint32_t name_len, uint32_t expr_str_len, struct ck_constraint_def * ck_constraint_def_new(const char *name, uint32_t name_len, const char *expr, uint32_t expr_str_len, uint32_t space_id, - enum ck_constraint_language language); + enum ck_constraint_language language, bool is_enabled); /** * Destroy check constraint definition memory, release acquired diff --git a/src/box/lua/schema.lua b/src/box/lua/schema.lua index 98067f795..e898c3aa6 100644 --- a/src/box/lua/schema.lua +++ b/src/box/lua/schema.lua @@ -1706,7 +1706,7 @@ space_mt.create_check_constraint = function(space, name, code) box.error(box.error.PROC_LUA, "Usage: space:create_constraint(name, code)") end - box.space._ck_constraint:insert({space.id, name, false, 'SQL', code}) + box.space._ck_constraint:insert({space.id, name, false, 'SQL', code, true}) return space.ck_constraint[name] end space_mt.pairs = function(space, key, opts) @@ -1759,6 +1759,20 @@ ck_constraint_mt.drop = function(ck_constraint) check_ck_constraint_arg(ck_constraint, 'drop') box.space._ck_constraint:delete({ck_constraint.space_id, ck_constraint.name}) end +ck_constraint_mt.enable = function(ck_constraint, yesno) + check_ck_constraint_arg(ck_constraint, 'enable') + local s = builtin.space_by_id(ck_constraint.space_id) + if s == nil then + box.error(box.error.NO_SUCH_SPACE, tostring(ck_constraint.space_id)) + end + local t = box.space._ck_constraint:get({ck_constraint.space_id, + ck_constraint.name}) + if t == nil then + box.error(box.error.NO_SUCH_CONSTRAINT, tostring(ck_constraint.name)) + end + box.space._ck_constraint:update({ck_constraint.space_id, ck_constraint.name}, + {{'=', 6, yesno}}) +end box.schema.index_mt = base_index_mt box.schema.memtx_index_mt = memtx_index_mt diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc index d0a7e7815..8b84e1fb2 100644 --- a/src/box/lua/space.cc +++ b/src/box/lua/space.cc @@ -205,6 +205,9 @@ lbox_push_ck_constraint(struct lua_State *L, struct space *space, int i) lua_pushstring(L, ck_constraint->def->expr_str); lua_setfield(L, -2, "expr"); + lua_pushboolean(L, ck_constraint->def->is_enabled); + lua_setfield(L, -2, "is_enabled"); + lua_setfield(L, -2, ck_constraint->def->name); } lua_pop(L, 1); diff --git a/src/box/lua/upgrade.lua b/src/box/lua/upgrade.lua index 2abd75dff..e71b7fb41 100644 --- a/src/box/lua/upgrade.lua +++ b/src/box/lua/upgrade.lua @@ -927,6 +927,19 @@ local function upgrade_to_2_3_0() datetime, datetime}) _priv:replace{ADMIN, PUBLIC, 'function', t.id, box.priv.X} end + + log.info("Extend _ck_constraint space format with is_enabled field") + local _ck_constraint = box.space._ck_constraint + for _, tuple in _ck_constraint:pairs() do + _ck_constraint:update({tuple[1], tuple[2]}, {{'=', 6, true}}) + end + local format = {{name='space_id', type='unsigned'}, + {name='name', type='string'}, + {name='is_deferred', type='boolean'}, + {name='language', type='str'}, + {name='code', type='str'}, + {name='is_enabled', type='boolean'}} + _ck_constraint:format(format) end -------------------------------------------------------------------------------- diff --git a/src/box/schema_def.h b/src/box/schema_def.h index 85f652d52..ba870ff42 100644 --- a/src/box/schema_def.h +++ b/src/box/schema_def.h @@ -267,6 +267,7 @@ enum { BOX_CK_CONSTRAINT_FIELD_DEFERRED = 2, BOX_CK_CONSTRAINT_FIELD_LANGUAGE = 3, BOX_CK_CONSTRAINT_FIELD_CODE = 4, + BOX_CK_CONSTRAINT_FIELD_IS_ENABLED = 5, }; /** _func_index fields. */ diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 233f56734..51cd7ce63 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -982,7 +982,7 @@ vdbe_emit_ck_constraint_create(struct Parse *parser, * Occupy registers for 5 fields: each member in * _ck_constraint space plus one for final msgpack tuple. */ - int ck_constraint_reg = sqlGetTempRange(parser, 6); + int ck_constraint_reg = sqlGetTempRange(parser, 7); sqlVdbeAddOp2(v, OP_SCopy, reg_space_id, ck_constraint_reg); sqlVdbeAddOp4(v, OP_String8, 0, ck_constraint_reg + 1, 0, sqlDbStrDup(db, ck_def->name), P4_DYNAMIC); @@ -991,8 +991,9 @@ vdbe_emit_ck_constraint_create(struct Parse *parser, ck_constraint_language_strs[ck_def->language], P4_STATIC); sqlVdbeAddOp4(v, OP_String8, 0, ck_constraint_reg + 4, 0, sqlDbStrDup(db, ck_def->expr_str), P4_DYNAMIC); - sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 5, - ck_constraint_reg + 5); + sqlVdbeAddOp2(v, OP_Bool, true, ck_constraint_reg + 5); + sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 6, + ck_constraint_reg + 6); const char *error_msg = tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), ck_def->name); @@ -1002,9 +1003,9 @@ vdbe_emit_ck_constraint_create(struct Parse *parser, false, OP_NoConflict) != 0) return; sqlVdbeAddOp2(v, OP_SInsert, BOX_CK_CONSTRAINT_ID, - ck_constraint_reg + 5); + ck_constraint_reg + 6); VdbeComment((v, "Create CK constraint %s", ck_def->name)); - sqlReleaseTempRange(parser, ck_constraint_reg, 6); + sqlReleaseTempRange(parser, ck_constraint_reg, 7); } /** diff --git a/test/box-py/bootstrap.result b/test/box-py/bootstrap.result index a59979e62..123aa2feb 100644 --- a/test/box-py/bootstrap.result +++ b/test/box-py/bootstrap.result @@ -92,7 +92,8 @@ box.space._space:select{} {'name': 'child_cols', 'type': 'array'}, {'name': 'parent_cols', 'type': 'array'}]] - [364, 1, '_ck_constraint', 'memtx', 0, {}, [{'name': 'space_id', 'type': 'unsigned'}, {'name': 'name', 'type': 'string'}, {'name': 'is_deferred', 'type': 'boolean'}, - {'name': 'language', 'type': 'str'}, {'name': 'code', 'type': 'str'}]] + {'name': 'language', 'type': 'str'}, {'name': 'code', 'type': 'str'}, {'name': 'is_enabled', + 'type': 'boolean'}]] - [372, 1, '_func_index', 'memtx', 0, {}, [{'name': 'space_id', 'type': 'unsigned'}, {'name': 'index_id', 'type': 'unsigned'}, {'name': 'func_id', 'type': 'unsigned'}]] ... diff --git a/test/box/access_misc.result b/test/box/access_misc.result index a1b6435bc..27eb47aa0 100644 --- a/test/box/access_misc.result +++ b/test/box/access_misc.result diff --git a/test/sql/checks.result b/test/sql/checks.result index 50347bc3a..72b1fa26f 100644 --- a/test/sql/checks.result +++ b/test/sql/checks.result @@ -37,35 +37,35 @@ _ = box.space.test:create_index('pk') --- ... -- Invalid expression test. -box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X><5'}) +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X><5', true}) --- - error: 'Failed to create check constraint ''CK_CONSTRAINT_01'': Syntax error near ''<''' ... -- Non-existent space test. -box.space._ck_constraint:insert({550, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'}) +box.space._ck_constraint:insert({550, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true}) --- - error: Space '550' does not exist ... -- Pass integer instead of expression. -box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 666}) +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 666, true}) --- - error: 'Tuple field 5 type does not match one required by operation: expected string' ... -- Deferred CK constraints are not supported. -box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', true, 'SQL', 'X<5'}) +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', true, 'SQL', 'X<5', true}) --- - error: Tarantool does not support deferred ck constraints ... -- The only supported language is SQL. -box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'LUA', 'X<5'}) +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'LUA', 'X<5', true}) --- - error: Unsupported language 'LUA' specified for function 'CK_CONSTRAINT_01' ... -- Check constraints LUA creation test. -box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'}) +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true}) --- -- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'] +- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true] ... box.space._ck_constraint:count({}) --- @@ -80,9 +80,9 @@ box.space.test:insert({5}) --- - error: 'Check constraint failed ''CK_CONSTRAINT_01'': X<5' ... -box.space._ck_constraint:replace({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'}) +box.space._ck_constraint:replace({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true}) --- -- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'] +- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true] ... box.execute("INSERT INTO \"test\" VALUES(5);") --- @@ -111,7 +111,7 @@ box.space._space:delete({513}) ... box.space._ck_constraint:delete({513, 'CK_CONSTRAINT_01'}) --- -- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'] +- [513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true] ... box.space._space:delete({513}) --- @@ -177,14 +177,14 @@ s = box.schema.create_space('test', {engine = engine}) _ = s:create_index('pk') --- ... -_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y'}) +_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y', true}) --- - error: Tarantool does not support CK constraint for space without format ... s:format({{name='X', type='integer'}, {name='Y', type='integer'}}) --- ... -_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y'}) +_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y', true}) --- ... box.execute("INSERT INTO \"test\" VALUES(2, 1);") @@ -229,7 +229,7 @@ s:insert({2, 1}) --- - [2, 1] ... -_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10'}) +_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10', true}) --- - error: 'Failed to create check constraint ''conflict'': referencing space must be empty' @@ -237,7 +237,7 @@ _ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10'}) s:truncate() --- ... -_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10'}) +_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10', true}) --- ... box.execute("INSERT INTO \"test\" VALUES(1, 2);") @@ -332,7 +332,7 @@ _ = s:create_index('pk') s:format({{name='X', type='any'}, {name='Y', type='integer'}, {name='Z', type='integer', is_nullable=true}}) --- ... -ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'X = 1 AND Z IS NOT NULL'}) +ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'X = 1 AND Z IS NOT NULL', true}) --- ... s:insert({2, 1}) @@ -374,7 +374,7 @@ _ = s:create_index('pk') s:format({{name='X', type='any'}, {name='Y', type='integer'}, {name='Z', type='integer', is_nullable=true}}) --- ... -ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'Z IS NOT NULL'}) +ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'Z IS NOT NULL', true}) --- ... s:insert({1, 2, box.NULL}) @@ -388,7 +388,7 @@ s:insert({1, 2}) _ = box.space._ck_constraint:delete({s.id, 'ZnotNULL'}) --- ... -_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y and Y < Z'}) +_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y and Y < Z', true}) --- ... s:insert({'1', 2}) @@ -464,7 +464,7 @@ _ = s:create_index('pk', {parts = {3, 'integer'}}) _ = s:create_index('unique', {parts = {1, 'integer'}}) --- ... -_ = box.space._ck_constraint:insert({s.id, 'complex1', false, 'SQL', 'x+y==11 OR x*y==12 OR x/y BETWEEN 5 AND 8 OR -x == y+10'}) +_ = box.space._ck_constraint:insert({s.id, 'complex1', false, 'SQL', 'x+y==11 OR x*y==12 OR x/y BETWEEN 5 AND 8 OR -x == y+10', true}) --- ... s:insert({1, 10, 1}) @@ -513,7 +513,7 @@ s:format({{name='X', type='integer'}, {name='Z', type='any'}}) _ = s:create_index('pk', {parts = {1, 'integer'}}) --- ... -_ = box.space._ck_constraint:insert({s.id, 'complex2', false, 'SQL', 'typeof(coalesce(z,0))==\'integer\''}) +_ = box.space._ck_constraint:insert({s.id, 'complex2', false, 'SQL', 'typeof(coalesce(z,0))==\'integer\'', true}) --- ... s:insert({1, 'string'}) @@ -564,7 +564,7 @@ test_run:cmd("setopt delimiter ''"); s:format(format65) --- ... -_ = box.space._ck_constraint:insert({s.id, 'X1is666andX65is666', false, 'SQL', 'X1 == 666 and X65 == 666 and X63 IS NOT NULL'}) +_ = box.space._ck_constraint:insert({s.id, 'X1is666andX65is666', false, 'SQL', 'X1 == 666 and X65 == 666 and X63 IS NOT NULL', true}) --- ... s:insert(s:frommap({X1 = 1, X65 = 1})) @@ -642,24 +642,28 @@ _ = s2:create_check_constraint('greater', 'X > 20') s1.ck_constraint.physics --- - space_id: <ID> + is_enabled: true name: physics expr: X < Y ... s1.ck_constraint.greater --- - space_id: <ID> + is_enabled: true name: greater expr: X > 20 ... s2.ck_constraint.physics --- - space_id: <ID> + is_enabled: true name: physics expr: X > Y ... s2.ck_constraint.greater --- - space_id: <ID> + is_enabled: true name: greater expr: X > 20 ... @@ -685,6 +689,7 @@ s2.ck_constraint.greater:drop() s2.ck_constraint.physics --- - space_id: <ID> + is_enabled: true name: physics expr: X > Y ... @@ -716,12 +721,75 @@ s2:drop() physics_ck --- - space_id: <ID> + is_enabled: true name: physics expr: X > Y ... physics_ck:drop() --- ... +-- +-- gh-4244: Add an ability to disable CK constraints +-- Make sure that ck constraints are turned on/off with +-- :enable configurator. +-- +engine = test_run:get_cfg('engine') +--- +... +box.execute('pragma sql_default_engine=\''..engine..'\'') +--- +- row_count: 0 +... +box.execute("CREATE TABLE test(a INT PRIMARY KEY);"); +--- +- row_count: 1 +... +box.execute('ALTER TABLE test ADD CONSTRAINT CK CHECK(a < 5);') +--- +- row_count: 1 +... +box.space.TEST:insert({10}) +--- +- error: 'Check constraint failed ''CK'': a < 5' +... +box.space.TEST.ck_constraint.CK:enable(false) +--- +... +assert(box.space.TEST.ck_constraint.CK.is_enabled == false) +--- +- true +... +box.space.TEST:insert({11}) +--- +- [11] +... +-- Test is_enabled state persistency. +test_run:cmd("restart server default") +test_run = require('test_run').new() +--- +... +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '") +--- +- true +... +box.space.TEST:insert({12}) +--- +- [12] +... +box.space.TEST.ck_constraint.CK:enable(true) +--- +... +assert(box.space.TEST.ck_constraint.CK.is_enabled == true) +--- +- true +... +box.space.TEST:insert({13}) +--- +- error: 'Check constraint failed ''CK'': a < 5' +... +box.space.TEST:drop() +--- +... test_run:cmd("clear filter") --- - true diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua index cde213f8b..11918dced 100644 --- a/test/sql/checks.test.lua +++ b/test/sql/checks.test.lua @@ -18,23 +18,23 @@ s = box.space._space:insert(t) _ = box.space.test:create_index('pk') -- Invalid expression test. -box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X><5'}) +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X><5', true}) -- Non-existent space test. -box.space._ck_constraint:insert({550, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'}) +box.space._ck_constraint:insert({550, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true}) -- Pass integer instead of expression. -box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 666}) +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 666, true}) -- Deferred CK constraints are not supported. -box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', true, 'SQL', 'X<5'}) +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', true, 'SQL', 'X<5', true}) -- The only supported language is SQL. -box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'LUA', 'X<5'}) +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'LUA', 'X<5', true}) -- Check constraints LUA creation test. -box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'}) +box.space._ck_constraint:insert({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true}) box.space._ck_constraint:count({}) box.execute("INSERT INTO \"test\" VALUES(5);") box.space.test:insert({5}) -box.space._ck_constraint:replace({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'}) +box.space._ck_constraint:replace({513, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true}) box.execute("INSERT INTO \"test\" VALUES(5);") box.execute("INSERT INTO \"test\" VALUES(6);") box.space.test:insert({6}) @@ -64,9 +64,9 @@ box.space._ck_constraint:count() == 0 -- Ck constraints are disallowed for spaces having no format. s = box.schema.create_space('test', {engine = engine}) _ = s:create_index('pk') -_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y'}) +_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y', true}) s:format({{name='X', type='integer'}, {name='Y', type='integer'}}) -_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y'}) +_ = box.space._ck_constraint:insert({s.id, 'physics', false, 'SQL', 'X<Y', true}) box.execute("INSERT INTO \"test\" VALUES(2, 1);") s:format({{name='Y', type='integer'}, {name='X', type='integer'}}) box.execute("INSERT INTO \"test\" VALUES(1, 2);") @@ -78,9 +78,9 @@ s:format() s:format({{name='Y1', type='integer'}, {name='X1', type='integer'}}) -- Ck constraint creation is forbidden for non-empty space s:insert({2, 1}) -_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10'}) +_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10', true}) s:truncate() -_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10'}) +_ = box.space._ck_constraint:insert({s.id, 'conflict', false, 'SQL', 'X>10', true}) box.execute("INSERT INTO \"test\" VALUES(1, 2);") box.execute("INSERT INTO \"test\" VALUES(11, 11);") box.execute("INSERT INTO \"test\" VALUES(12, 11);") @@ -114,7 +114,7 @@ box.execute("DROP TABLE t1") s = box.schema.create_space('test', {engine = engine}) _ = s:create_index('pk') s:format({{name='X', type='any'}, {name='Y', type='integer'}, {name='Z', type='integer', is_nullable=true}}) -ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'X = 1 AND Z IS NOT NULL'}) +ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'X = 1 AND Z IS NOT NULL', true}) s:insert({2, 1}) s:insert({1, 1}) s:insert({1, 1, box.NULL}) @@ -129,11 +129,11 @@ s:drop() s = box.schema.create_space('test', {engine = engine}) _ = s:create_index('pk') s:format({{name='X', type='any'}, {name='Y', type='integer'}, {name='Z', type='integer', is_nullable=true}}) -ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'Z IS NOT NULL'}) +ck_not_null = box.space._ck_constraint:insert({s.id, 'ZnotNULL', false, 'SQL', 'Z IS NOT NULL', true}) s:insert({1, 2, box.NULL}) s:insert({1, 2}) _ = box.space._ck_constraint:delete({s.id, 'ZnotNULL'}) -_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y and Y < Z'}) +_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y and Y < Z', true}) s:insert({'1', 2}) s:insert({}) s:insert({2, 1}) @@ -157,7 +157,7 @@ s = box.schema.create_space('test', {engine = engine}) s:format({{name='X', type='integer'}, {name='Y', type='integer'}, {name='Z', type='integer'}}) _ = s:create_index('pk', {parts = {3, 'integer'}}) _ = s:create_index('unique', {parts = {1, 'integer'}}) -_ = box.space._ck_constraint:insert({s.id, 'complex1', false, 'SQL', 'x+y==11 OR x*y==12 OR x/y BETWEEN 5 AND 8 OR -x == y+10'}) +_ = box.space._ck_constraint:insert({s.id, 'complex1', false, 'SQL', 'x+y==11 OR x*y==12 OR x/y BETWEEN 5 AND 8 OR -x == y+10', true}) s:insert({1, 10, 1}) s:update({1}, {{'=', 1, 4}, {'=', 2, 3}}) s:update({1}, {{'=', 1, 12}, {'=', 2, 2}}) @@ -171,7 +171,7 @@ s:drop() s = box.schema.create_space('test', {engine = engine}) s:format({{name='X', type='integer'}, {name='Z', type='any'}}) _ = s:create_index('pk', {parts = {1, 'integer'}}) -_ = box.space._ck_constraint:insert({s.id, 'complex2', false, 'SQL', 'typeof(coalesce(z,0))==\'integer\''}) +_ = box.space._ck_constraint:insert({s.id, 'complex2', false, 'SQL', 'typeof(coalesce(z,0))==\'integer\'', true}) s:insert({1, 'string'}) s:insert({1, {map=true}}) s:insert({1, {'a', 'r','r','a','y'}}) @@ -191,7 +191,7 @@ for i = 1,66 do end test_run:cmd("setopt delimiter ''"); s:format(format65) -_ = box.space._ck_constraint:insert({s.id, 'X1is666andX65is666', false, 'SQL', 'X1 == 666 and X65 == 666 and X63 IS NOT NULL'}) +_ = box.space._ck_constraint:insert({s.id, 'X1is666andX65is666', false, 'SQL', 'X1 == 666 and X65 == 666 and X63 IS NOT NULL', true}) s:insert(s:frommap({X1 = 1, X65 = 1})) s:insert(s:frommap({X1 = 666, X65 = 1})) s:insert(s:frommap({X1 = 1, X65 = 666})) @@ -234,4 +234,27 @@ s2:drop() physics_ck physics_ck:drop() +-- +-- gh-4244: Add an ability to disable CK constraints +-- Make sure that ck constraints are turned on/off with +-- :enable configurator. +-- +engine = test_run:get_cfg('engine') +box.execute('pragma sql_default_engine=\''..engine..'\'') +box.execute("CREATE TABLE test(a INT PRIMARY KEY);"); +box.execute('ALTER TABLE test ADD CONSTRAINT CK CHECK(a < 5);') +box.space.TEST:insert({10}) +box.space.TEST.ck_constraint.CK:enable(false) +assert(box.space.TEST.ck_constraint.CK.is_enabled == false) +box.space.TEST:insert({11}) +-- Test is_enabled state persistency. +test_run:cmd("restart server default") +test_run = require('test_run').new() +test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '") +box.space.TEST:insert({12}) +box.space.TEST.ck_constraint.CK:enable(true) +assert(box.space.TEST.ck_constraint.CK.is_enabled == true) +box.space.TEST:insert({13}) +box.space.TEST:drop() + test_run:cmd("clear filter") diff --git a/test/sql/errinj.result b/test/sql/errinj.result index ecc194fb8..7ab522f49 100644 --- a/test/sql/errinj.result +++ b/test/sql/errinj.result @@ -410,7 +410,7 @@ errinj.set("ERRINJ_WAL_IO", true) --- - ok ... -_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'}) +_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true}) --- - error: Failed to write to disk ... @@ -418,7 +418,7 @@ errinj.set("ERRINJ_WAL_IO", false) --- - ok ... -_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'}) +_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true}) --- ... box.execute("INSERT INTO \"test\" VALUES(5);") @@ -430,7 +430,7 @@ errinj.set("ERRINJ_WAL_IO", true) --- - ok ... -_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'}) +_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true}) --- - error: Failed to write to disk ... @@ -438,7 +438,7 @@ errinj.set("ERRINJ_WAL_IO", false) --- - ok ... -_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'}) +_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true}) --- ... box.execute("INSERT INTO \"test\" VALUES(5);") @@ -484,10 +484,10 @@ _ = s:create_index('pk') s:format({{name='X', type='integer'}, {name='Y', type='integer'}}) --- ... -_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y'}) +_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y', true}) --- ... -_ = box.space._ck_constraint:insert({s.id, 'Xgreater10', false, 'SQL', 'X > 10'}) +_ = box.space._ck_constraint:insert({s.id, 'Xgreater10', false, 'SQL', 'X > 10', true}) --- ... box.execute("INSERT INTO \"test\" VALUES(1, 2);") diff --git a/test/sql/errinj.test.lua b/test/sql/errinj.test.lua index 2b4f74a82..b978767ad 100644 --- a/test/sql/errinj.test.lua +++ b/test/sql/errinj.test.lua @@ -126,14 +126,14 @@ s = box.schema.space.create('test', {format = {{name = 'X', type = 'unsigned'}}} pk = box.space.test:create_index('pk') errinj.set("ERRINJ_WAL_IO", true) -_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'}) +_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true}) errinj.set("ERRINJ_WAL_IO", false) -_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5'}) +_ = box.space._ck_constraint:insert({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<5', true}) box.execute("INSERT INTO \"test\" VALUES(5);") errinj.set("ERRINJ_WAL_IO", true) -_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'}) +_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true}) errinj.set("ERRINJ_WAL_IO", false) -_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5'}) +_ = box.space._ck_constraint:replace({s.id, 'CK_CONSTRAINT_01', false, 'SQL', 'X<=5', true}) box.execute("INSERT INTO \"test\" VALUES(5);") errinj.set("ERRINJ_WAL_IO", true) _ = box.space._ck_constraint:delete({s.id, 'CK_CONSTRAINT_01'}) @@ -149,8 +149,8 @@ s:drop() s = box.schema.create_space('test') _ = s:create_index('pk') s:format({{name='X', type='integer'}, {name='Y', type='integer'}}) -_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y'}) -_ = box.space._ck_constraint:insert({s.id, 'Xgreater10', false, 'SQL', 'X > 10'}) +_ = box.space._ck_constraint:insert({s.id, 'XlessY', false, 'SQL', 'X < Y', true}) +_ = box.space._ck_constraint:insert({s.id, 'Xgreater10', false, 'SQL', 'X > 10', true}) box.execute("INSERT INTO \"test\" VALUES(1, 2);") box.execute("INSERT INTO \"test\" VALUES(20, 10);") box.execute("INSERT INTO \"test\" VALUES(20, 100);") -- 2.23.0 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints 2019-10-15 11:13 ` [Tarantool-patches] [tarantool-patches] " Kirill Shcherbatov @ 2019-10-15 21:47 ` Nikita Pettik 2019-10-16 5:52 ` Konstantin Osipov 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2019-10-15 21:47 UTC (permalink / raw) To: Kirill Shcherbatov; +Cc: tarantool-patches, tarantool-patches On 15 Oct 14:13, Kirill Shcherbatov wrote: > > -> when processed data is verified and constraints validation > > is not required. For instance, during casual recovery process there's > > no need to provide any checks since data is assumed to be consistent. > Applied. > > > Also explain why we have to store this option. > > To be honest, I doubt that 'is_enable' option should be persisted.. > > At least, we can assume that it is always 'on' by default, so that > > save user from having to specify this option each time. > > Verbally discussed. To implement a behavior that Oracle users are familiar with. > It was a Kostya's point of view and it is already implemented. Verbally I said that 'somebody told me to do so' is a quite weak argument. So we either add fine rationale for this feature or remove it. Since it was not my suggestion I can't say why do we need it. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints 2019-10-15 21:47 ` Nikita Pettik @ 2019-10-16 5:52 ` Konstantin Osipov 2019-10-16 11:19 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Konstantin Osipov @ 2019-10-16 5:52 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches, tarantool-patches * Nikita Pettik <korablev@tarantool.org> [19/10/16 08:45]: > On 15 Oct 14:13, Kirill Shcherbatov wrote: > > > -> when processed data is verified and constraints validation > > > is not required. For instance, during casual recovery process there's > > > no need to provide any checks since data is assumed to be consistent. > > Applied. > > > > > Also explain why we have to store this option. > > > To be honest, I doubt that 'is_enable' option should be persisted.. > > > At least, we can assume that it is always 'on' by default, so that > > > save user from having to specify this option each time. > > > > Verbally discussed. To implement a behavior that Oracle users are familiar with. > > It was a Kostya's point of view and it is already implemented. > > Verbally I said that 'somebody told me to do so' is a quite weak argument. > So we either add fine rationale for this feature or remove it. Since it > was not my suggestion I can't say why do we need it. Nikita, if the option is not stored the following scenario is possible: - the option is turned off - data is changed so that a constraint is violated - the system is restarted while the option state is lost - there is no way (even in theory) to discover it and find that data is incorrect. If the option is persistent you can run a check when it is turned back on again. If we don't store the option, we should not wire it up in the interface and only use internally, during recovery, which would make it difficult to test the option. > -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints 2019-10-16 5:52 ` Konstantin Osipov @ 2019-10-16 11:19 ` Nikita Pettik 2019-10-16 13:50 ` Kirill Shcherbatov 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2019-10-16 11:19 UTC (permalink / raw) To: Konstantin Osipov, Kirill Shcherbatov, tarantool-patches, tarantool-patches On 16 Oct 08:52, Konstantin Osipov wrote: > * Nikita Pettik <korablev@tarantool.org> [19/10/16 08:45]: > > On 15 Oct 14:13, Kirill Shcherbatov wrote: > > > > -> when processed data is verified and constraints validation > > > > is not required. For instance, during casual recovery process there's > > > > no need to provide any checks since data is assumed to be consistent. > > > Applied. > > > > > > > Also explain why we have to store this option. > > > > To be honest, I doubt that 'is_enable' option should be persisted.. > > > > At least, we can assume that it is always 'on' by default, so that > > > > save user from having to specify this option each time. > > > > > > Verbally discussed. To implement a behavior that Oracle users are familiar with. > > > It was a Kostya's point of view and it is already implemented. > > > > Verbally I said that 'somebody told me to do so' is a quite weak argument. > > So we either add fine rationale for this feature or remove it. Since it > > was not my suggestion I can't say why do we need it. > > Nikita, if the option is not stored the following scenario is > possible: > > - the option is turned off > - data is changed so that a constraint is violated > - the system is restarted while the option state is lost > - there is no way (even in theory) to discover it and find that > data is incorrect. Thanks for the scenario, now it looks reasonable. On the other hand, if we decided to always run check constraints (both on casual and force recovery), we would easily find out that data violates contraints. Kirill, could you please extend commit message with Konstantin's explanation? > If the option is persistent you can run a check when it is turned > back on again. > > If we don't store the option, we should not wire it up in the > interface and only use internally, during recovery, which would > make it difficult to test the option. > > > > -- > Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints 2019-10-16 11:19 ` Nikita Pettik @ 2019-10-16 13:50 ` Kirill Shcherbatov 2019-10-16 18:09 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Kirill Shcherbatov @ 2019-10-16 13:50 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik, tarantool-patches > Thanks for the scenario, now it looks reasonable. On the other hand, > if we decided to always run check constraints (both on casual and force > recovery), we would easily find out that data violates contraints. > > Kirill, could you please extend commit message with Konstantin's explanation? Done. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 1/4] box: add an ability to disable CK constraints 2019-10-16 13:50 ` Kirill Shcherbatov @ 2019-10-16 18:09 ` Nikita Pettik 0 siblings, 0 replies; 18+ messages in thread From: Nikita Pettik @ 2019-10-16 18:09 UTC (permalink / raw) To: Kirill Shcherbatov; +Cc: tarantool-patches, tarantool-patches On 16 Oct 16:50, Kirill Shcherbatov wrote: > > Thanks for the scenario, now it looks reasonable. On the other hand, > > if we decided to always run check constraints (both on casual and force > > recovery), we would easily find out that data violates contraints. > > > > Kirill, could you please extend commit message with Konstantin's explanation? > > Done. Pushed to master. ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <d4002407f749fff0c1f0facb1ed4cf66b8b7edd6.1570539526.git.kshcherbatov@tarantool.org>]
* Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 2/4] sql: add an ability to disable CK constraints [not found] ` <d4002407f749fff0c1f0facb1ed4cf66b8b7edd6.1570539526.git.kshcherbatov@tarantool.org> @ 2019-10-14 16:56 ` Nikita Pettik 2019-10-15 11:13 ` [Tarantool-patches] [tarantool-patches] " Kirill Shcherbatov 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2019-10-14 16:56 UTC (permalink / raw) To: tarantool-patches; +Cc: tarantool-patches On 08 Oct 16:02, Kirill Shcherbatov wrote: > Closes #4244 > > @TarantoolBot document > Title: an ability to disable CK constraints > > Now it is possible to disable and enable ck constraints. > This option is not persistent. Outdated comment. > All ck constraints are enabled > by default when Tarantool is configured. Ck constraints checks > are not performed during standard recovery, but performed during > force_recovery - all conflicting tuples are skipped in case of > ck_constraint conflict. > > To change CK constraint "is_enabled" state, call > -- in LUA > ck_obj:enable(new_state in {true, false}) > -- in SQL > ALTER TABLE {TABLE_NAME} {EN, DIS}ABLE CHECK CONSTRAINT {CK_NAME}; > > Example: > box.space.T6.ck_constraint.ck_unnamed_T6_1:enable(false) > box.space.T6.ck_constraint.ck_unnamed_T6_1 > - space_id: 512 > is_enabled: false > name: ck_unnamed_T6_1 > expr: a < 10 > box.space.T6:insert({11}) > -- passed > box.execute("ALTER TABLE t6 ENABLE CHECK CONSTRAINT \"ck_unnamed_T6_1\"") > box.space.T6:insert({12}) > - error: 'Check constraint failed ''ck_unnamed_T6_1'': a < 10' > --- > diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c > index 765600186..500cabc5f 100644 > --- a/src/box/sql/alter.c > +++ b/src/box/sql/alter.c > @@ -79,6 +79,71 @@ tnt_error: > goto exit_rename_table; > } > > +void > +sql_alter_ck_constraint_enable(struct Parse *parse) > +{ > + struct enable_entity_def *enable_def = &parse->enable_entity_def; > + struct SrcList *src_tab = enable_def->base.entity_name; > + assert(enable_def->base.entity_type == ENTITY_TYPE_CK); > + assert(enable_def->base.alter_action == ALTER_ACTION_ENABLE); > + assert(src_tab->nSrc == 1); > + struct sql *db = parse->db; > + > + char *constraint_name = NULL; > + const char *tbl_name = src_tab->a[0].zName; > + struct space *space = space_by_name(tbl_name); > + if (space == NULL) { > + diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name); > + parse->is_aborted = true; > + goto exit_alter_ck_constraint; > + } > + > + constraint_name = sql_name_from_token(db, &enable_def->name); > + if (constraint_name == NULL) { > + parse->is_aborted = true; > + goto exit_alter_ck_constraint; > + } > + > + struct Vdbe *v = sqlGetVdbe(parse); > + if (v == NULL) > + goto exit_alter_ck_constraint; > + > + struct space *ck_space = space_by_id(BOX_CK_CONSTRAINT_ID); > + assert(ck_space != NULL); > + int cursor = parse->nTab++; > + vdbe_emit_open_cursor(parse, cursor, 0, ck_space); > + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); > + > + int key_reg = sqlGetTempRange(parse, 2); > + sqlVdbeAddOp2(v, OP_Integer, space->def->id, key_reg); > + sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0, > + sqlDbStrDup(db, constraint_name), P4_DYNAMIC); > + int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 2); > + sqlVdbeAddOp4(v, OP_SetDiag, ER_NO_SUCH_CONSTRAINT, 0, 0, > + sqlMPrintf(db, tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), > + constraint_name), P4_DYNAMIC); > + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); > + sqlVdbeJumpHere(v, addr); > + > + const int field_count = 6; > + int tuple_reg = sqlGetTempRange(parse, field_count + 1); > + for (int i = 0; i < field_count - 1; ++i) > + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i); > + sqlVdbeAddOp1(v, OP_Close, cursor); > + sqlVdbeAddOp2(v, OP_Bool, enable_def->is_enabled, > + tuple_reg + field_count - 1); > + sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, field_count, > + tuple_reg + field_count); > + sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + field_count, 0, 0, > + (char *)ck_space, P4_SPACEPTR); > + > + > + Too many new lines. > +exit_alter_ck_constraint: > + sqlDbFree(db, constraint_name); > + sqlSrcListDelete(db, src_tab); > +} > + > /* This function is used to implement the ALTER TABLE command. > * The table name in the CREATE TRIGGER statement is replaced with the third > * argument and the result returned. This is analagous to rename_table() ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 2/4] sql: add an ability to disable CK constraints 2019-10-14 16:56 ` [Tarantool-patches] [tarantool-patches] [PATCH v4 2/4] sql: " Nikita Pettik @ 2019-10-15 11:13 ` Kirill Shcherbatov 2019-10-16 18:10 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Kirill Shcherbatov @ 2019-10-15 11:13 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik; +Cc: tarantool-patches >> Now it is possible to disable and enable ck constraints. >> This option is not persistent. > > Outdated comment. Fixed. > Too many new lines. Fixed. ======================================================== Closes #4244 @TarantoolBot document Title: an ability to disable CK constraints Now it is possible to disable and enable ck constraints. All ck constraints are enabled by default when Tarantool is configured. Ck constraints checks are not performed during standard recovery, but performed during force_recovery - all conflicting tuples are skipped in case of ck_constraint conflict. To change CK constraint "is_enabled" state, call -- in LUA ck_obj:enable(new_state in {true, false}) -- in SQL ALTER TABLE {TABLE_NAME} {EN, DIS}ABLE CHECK CONSTRAINT {CK_NAME}; Example: box.space.T6.ck_constraint.ck_unnamed_T6_1:enable(false) box.space.T6.ck_constraint.ck_unnamed_T6_1 - space_id: 512 is_enabled: false name: ck_unnamed_T6_1 expr: a < 10 box.space.T6:insert({11}) -- passed box.execute("ALTER TABLE t6 ENABLE CHECK CONSTRAINT \"ck_unnamed_T6_1\"") box.space.T6:insert({12}) - error: 'Check constraint failed ''ck_unnamed_T6_1'': a < 10' --- extra/mkkeywordhash.c | 2 ++ src/box/sql/alter.c | 62 ++++++++++++++++++++++++++++++++++ src/box/sql/parse.y | 12 ++++++- src/box/sql/parse_def.h | 22 +++++++++++- src/box/sql/sqlInt.h | 10 ++++++ test/sql-tap/keyword1.test.lua | 4 ++- test/sql/checks.result | 49 +++++++++++++++++++++++++++ test/sql/checks.test.lua | 15 ++++++++ 8 files changed, 173 insertions(+), 3 deletions(-) diff --git a/extra/mkkeywordhash.c b/extra/mkkeywordhash.c index 2a59d6a61..3c3de4cca 100644 --- a/extra/mkkeywordhash.c +++ b/extra/mkkeywordhash.c @@ -89,6 +89,7 @@ static Keyword aKeywordTable[] = { { "DEFERRED", "TK_DEFERRED", false }, { "DEFERRABLE", "TK_DEFERRABLE", false }, { "DELETE", "TK_DELETE", true }, + { "DISABLE", "TK_DISABLE", false }, { "DESC", "TK_DESC", true }, { "DISTINCT", "TK_DISTINCT", true }, { "DROP", "TK_DROP", true }, @@ -203,6 +204,7 @@ static Keyword aKeywordTable[] = { { "DETERMINISTIC", "TK_STANDARD", true }, { "DOUBLE", "TK_STANDARD", true }, { "ELSEIF", "TK_STANDARD", true }, + { "ENABLE", "TK_ENABLE", false }, { "FETCH", "TK_STANDARD", true }, { "FLOAT", "TK_STANDARD", true }, { "FUNCTION", "TK_STANDARD", true }, diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index 765600186..973b420cf 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -79,6 +79,68 @@ tnt_error: goto exit_rename_table; } +void +sql_alter_ck_constraint_enable(struct Parse *parse) +{ + struct enable_entity_def *enable_def = &parse->enable_entity_def; + struct SrcList *src_tab = enable_def->base.entity_name; + assert(enable_def->base.entity_type == ENTITY_TYPE_CK); + assert(enable_def->base.alter_action == ALTER_ACTION_ENABLE); + assert(src_tab->nSrc == 1); + struct sql *db = parse->db; + + char *constraint_name = NULL; + const char *tbl_name = src_tab->a[0].zName; + struct space *space = space_by_name(tbl_name); + if (space == NULL) { + diag_set(ClientError, ER_NO_SUCH_SPACE, tbl_name); + parse->is_aborted = true; + goto exit_alter_ck_constraint; + } + + constraint_name = sql_name_from_token(db, &enable_def->name); + if (constraint_name == NULL) { + parse->is_aborted = true; + goto exit_alter_ck_constraint; + } + + struct Vdbe *v = sqlGetVdbe(parse); + if (v == NULL) + goto exit_alter_ck_constraint; + + struct space *ck_space = space_by_id(BOX_CK_CONSTRAINT_ID); + assert(ck_space != NULL); + int cursor = parse->nTab++; + vdbe_emit_open_cursor(parse, cursor, 0, ck_space); + sqlVdbeChangeP5(v, OPFLAG_SYSTEMSP); + + int key_reg = sqlGetTempRange(parse, 2); + sqlVdbeAddOp2(v, OP_Integer, space->def->id, key_reg); + sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0, + sqlDbStrDup(db, constraint_name), P4_DYNAMIC); + int addr = sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, 2); + sqlVdbeAddOp4(v, OP_SetDiag, ER_NO_SUCH_CONSTRAINT, 0, 0, + sqlMPrintf(db, tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), + constraint_name), P4_DYNAMIC); + sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); + sqlVdbeJumpHere(v, addr); + + const int field_count = 6; + int tuple_reg = sqlGetTempRange(parse, field_count + 1); + for (int i = 0; i < field_count - 1; ++i) + sqlVdbeAddOp3(v, OP_Column, cursor, i, tuple_reg + i); + sqlVdbeAddOp1(v, OP_Close, cursor); + sqlVdbeAddOp2(v, OP_Bool, enable_def->is_enabled, + tuple_reg + field_count - 1); + sqlVdbeAddOp3(v, OP_MakeRecord, tuple_reg, field_count, + tuple_reg + field_count); + sqlVdbeAddOp4(v, OP_IdxReplace, tuple_reg + field_count, 0, 0, + (char *)ck_space, P4_SPACEPTR); +exit_alter_ck_constraint: + sqlDbFree(db, constraint_name); + sqlSrcListDelete(db, src_tab); +} + /* This function is used to implement the ALTER TABLE command. * The table name in the CREATE TRIGGER statement is replaced with the third * argument and the result returned. This is analagous to rename_table() diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index ed59a875a..1d0c95fac 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -260,7 +260,7 @@ columnname(A) ::= nm(A) typedef(Y). {sqlAddColumn(pParse,&A,&Y);} CONFLICT DEFERRED END ENGINE FAIL IGNORE INITIALLY INSTEAD NO MATCH PLAN QUERY KEY OFFSET RAISE RELEASE REPLACE RESTRICT - RENAME CTIME_KW IF + RENAME CTIME_KW IF ENABLE DISABLE . %wildcard ANY. @@ -798,6 +798,10 @@ col_list_with_autoinc(A) ::= expr(Y) autoinc(I). { A = sql_expr_list_append(pParse->db, NULL, Y.pExpr); } +%type enable {bool} +enable(A) ::= ENABLE. {A = true;} +enable(A) ::= DISABLE. {A = false;} + %type sortorder {int} sortorder(A) ::= ASC. {A = SORT_ORDER_ASC;} @@ -1785,6 +1789,12 @@ cmd ::= ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { sql_drop_foreign_key(pParse); } +cmd ::= alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). { + enable_entity_def_init(&pParse->enable_entity_def, ENTITY_TYPE_CK, A, + &Z, E); + sql_alter_ck_constraint_enable(pParse); +} + //////////////////////// COMMON TABLE EXPRESSIONS //////////////////////////// %type with {With*} %type wqlist {With*} diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h index df1238b9e..2f433e4c0 100644 --- a/src/box/sql/parse_def.h +++ b/src/box/sql/parse_def.h @@ -169,7 +169,8 @@ enum entity_type { enum alter_action { ALTER_ACTION_CREATE = 0, ALTER_ACTION_DROP, - ALTER_ACTION_RENAME + ALTER_ACTION_RENAME, + ALTER_ACTION_ENABLE, }; struct alter_entity_def { @@ -181,6 +182,14 @@ struct alter_entity_def { struct SrcList *entity_name; }; +struct enable_entity_def { + struct alter_entity_def base; + /** Name of constraint to be enabled/disabled. */ + struct Token name; + /** A new state to be set for entity found. */ + bool is_enabled; +}; + struct rename_entity_def { struct alter_entity_def base; struct Token new_name; @@ -327,6 +336,17 @@ rename_entity_def_init(struct rename_entity_def *rename_def, rename_def->new_name = *new_name; } +static inline void +enable_entity_def_init(struct enable_entity_def *enable_def, + enum entity_type type, struct SrcList *parent_name, + struct Token *name, bool is_enabled) +{ + alter_entity_def_init(&enable_def->base, parent_name, type, + ALTER_ACTION_ENABLE); + enable_def->name = *name; + enable_def->is_enabled = is_enabled; +} + static inline void create_entity_def_init(struct create_entity_def *create_def, enum entity_type type, struct SrcList *parent_name, diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index 35fc81dfb..bcc099fa9 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2202,6 +2202,7 @@ struct Parse { struct drop_table_def drop_table_def; struct drop_trigger_def drop_trigger_def; struct drop_view_def drop_view_def; + struct enable_entity_def enable_entity_def; }; /** * Table def is not part of union since information @@ -3971,6 +3972,15 @@ extern int sqlPendingByte; void sql_alter_table_rename(struct Parse *parse); +/** + * Generate code to implement the "ALTER TABLE xxx ENABLE/DISABLE + * CHECK CONSTRAINT" command. + * + * @param parse Current parsing context. + */ +void +sql_alter_ck_constraint_enable(struct Parse *parse); + /** * Return the length (in bytes) of the token that begins at z[0]. * Store the token type in *type before returning. diff --git a/test/sql-tap/keyword1.test.lua b/test/sql-tap/keyword1.test.lua index 03b1054d2..571e1dcbe 100755 --- a/test/sql-tap/keyword1.test.lua +++ b/test/sql-tap/keyword1.test.lua @@ -1,6 +1,6 @@ #!/usr/bin/env tarantool test = require("sqltester") -test:plan(180) +test:plan(184) --!./tcltestrunner.lua -- 2009 January 29 @@ -44,6 +44,8 @@ local kwlist = { "query", "restrict", "raise", + "enable", + "disable" } local bannedkws = { diff --git a/test/sql/checks.result b/test/sql/checks.result index 72b1fa26f..6d584c301 100644 --- a/test/sql/checks.result +++ b/test/sql/checks.result @@ -790,6 +790,55 @@ box.space.TEST:insert({13}) box.space.TEST:drop() --- ... +-- +-- Test ENABLE/DISABLE CK constraints from SQL works. +-- +box.execute("ALTER TABLE falsch DISABLE CHECK CONSTRAINT \"some_ck\"") +--- +- null +- Space 'FALSCH' does not exist +... +box.execute("CREATE TABLE test(a INT PRIMARY KEY);"); +--- +- row_count: 1 +... +box.execute('ALTER TABLE test ADD CONSTRAINT \"some_ck\" CHECK(a < 10);') +--- +- row_count: 1 +... +box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"falsch\"") +--- +- null +- Constraint falsch does not exist +... +box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"some_ck\"") +--- +- row_count: 0 +... +assert(box.space.TEST.ck_constraint.some_ck.is_enabled == false) +--- +- true +... +box.space.TEST:insert({11}) +--- +- [11] +... +box.execute("ALTER TABLE test ENABLE CHECK CONSTRAINT \"some_ck\"") +--- +- row_count: 0 +... +assert(box.space.TEST.ck_constraint.some_ck.is_enabled == true) +--- +- true +... +box.space.TEST:insert({12}) +--- +- error: 'Check constraint failed ''some_ck'': a < 10' +... +box.execute("DROP TABLE test;") +--- +- row_count: 1 +... test_run:cmd("clear filter") --- - true diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua index 11918dced..f8dc5d363 100644 --- a/test/sql/checks.test.lua +++ b/test/sql/checks.test.lua @@ -257,4 +257,19 @@ assert(box.space.TEST.ck_constraint.CK.is_enabled == true) box.space.TEST:insert({13}) box.space.TEST:drop() +-- +-- Test ENABLE/DISABLE CK constraints from SQL works. +-- +box.execute("ALTER TABLE falsch DISABLE CHECK CONSTRAINT \"some_ck\"") +box.execute("CREATE TABLE test(a INT PRIMARY KEY);"); +box.execute('ALTER TABLE test ADD CONSTRAINT \"some_ck\" CHECK(a < 10);') +box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"falsch\"") +box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"some_ck\"") +assert(box.space.TEST.ck_constraint.some_ck.is_enabled == false) +box.space.TEST:insert({11}) +box.execute("ALTER TABLE test ENABLE CHECK CONSTRAINT \"some_ck\"") +assert(box.space.TEST.ck_constraint.some_ck.is_enabled == true) +box.space.TEST:insert({12}) +box.execute("DROP TABLE test;") + test_run:cmd("clear filter") -- 2.23.0 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 2/4] sql: add an ability to disable CK constraints 2019-10-15 11:13 ` [Tarantool-patches] [tarantool-patches] " Kirill Shcherbatov @ 2019-10-16 18:10 ` Nikita Pettik 0 siblings, 0 replies; 18+ messages in thread From: Nikita Pettik @ 2019-10-16 18:10 UTC (permalink / raw) To: Kirill Shcherbatov; +Cc: tarantool-patches, tarantool-patches On 15 Oct 14:13, Kirill Shcherbatov wrote: > >> Now it is possible to disable and enable ck constraints. > >> This option is not persistent. > > > > Outdated comment. > > Fixed. > > > Too many new lines. > > Fixed. Pushed to master. ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <f462f55eebcb13abb8a0611a4d84d7ed8b1a6b6a.1570539526.git.kshcherbatov@tarantool.org>]
[parent not found: <af095dba-bacd-e35f-9143-30ae59188697@tarantool.org>]
* Re: [Tarantool-patches] [tarantool-patches] [PATCH v4 4/4] sql: use name instead of function pointer for UDF [not found] ` <af095dba-bacd-e35f-9143-30ae59188697@tarantool.org> @ 2019-10-15 15:15 ` Nikita Pettik 2019-10-16 13:51 ` [Tarantool-patches] [tarantool-patches] " Kirill Shcherbatov 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2019-10-15 15:15 UTC (permalink / raw) To: Kirill Shcherbatov; +Cc: tarantool-patches, tarantool-patches On 15 Oct 14:13, Kirill Shcherbatov wrote: > > Could you add test case covering mentioned situation? > Yes. Appended. > > ================================================= > > This patch changes OP_Function parameters convention: now a > function's name is passed instead of pointer to the function > object. This allows to normally handle the situation, when UDF > has been deleted to the moment of the VDBE code execution. > In particular case this may happen with CK constraints that > refers to a deleted persistent function. > > Closes #4176 > --- > src/box/sql/expr.c | 17 ++++++++++++----- > src/box/sql/vdbe.c | 11 ++++++++--- > test/sql/checks.result | 28 ++++++++++++++++++++++++++++ > test/sql/checks.test.lua | 11 +++++++++++ > 4 files changed, 59 insertions(+), 8 deletions(-) > > diff --git a/test/sql/checks.result b/test/sql/checks.result > index b8bd19a84..02ee82a4b 100644 > --- a/test/sql/checks.result > +++ b/test/sql/checks.result > @@ -918,6 +918,34 @@ box.execute("DROP TABLE test;") > --- > - row_count: 1 > ... > +-- > +-- gh-4176: Can't recover if check constraint involves function. > +-- > +function myfunc(x) return x < 10 end > +--- > +... > +box.schema.func.create('MYFUNC', {param_list = {'integer'}, returns = 'integer', is_deterministic = true, exports = {'LUA', 'SQL'}}) > +--- > +... > +box.execute("CREATE TABLE t6(a INT CHECK (myfunc(a)) primary key);"); > +--- > +- row_count: 1 > +... > +myfunc = nil > +--- > +... > +box.execute("INSERT INTO t6 VALUES(11);"); > +--- > +- null > +- Procedure 'MYFUNC' is not defined Something is wrong with this test case. Firstly, it works fine even on master branch. Secondly, I've dropped myfunc = nil and got the same error (on master as well) - i.e. INSERT fails with 'procedure is not defined' error even if function is not dropped. Obviously, to make it crash on master you should drop function before insertion; to fix second problem you should uppercase function's name. > +box.execute("DROP TABLE t6") > +--- > +- row_count: 1 > +... > +box.func.MYFUNC:drop() > +--- > +... > test_run:cmd("clear filter") > --- > - true > diff --git a/test/sql/checks.test.lua b/test/sql/checks.test.lua > index 832322190..fa68920f6 100644 > --- a/test/sql/checks.test.lua > +++ b/test/sql/checks.test.lua > @@ -302,4 +302,15 @@ assert(box.space.TEST.ck_constraint.some_ck.is_enabled == true) > box.space.TEST:insert({12}) > box.execute("DROP TABLE test;") > > +-- > +-- gh-4176: Can't recover if check constraint involves function. > +-- > +function myfunc(x) return x < 10 end > +box.schema.func.create('MYFUNC', {param_list = {'integer'}, returns = 'integer', is_deterministic = true, exports = {'LUA', 'SQL'}}) > +box.execute("CREATE TABLE t6(a INT CHECK (myfunc(a)) primary key);"); > +myfunc = nil > +box.execute("INSERT INTO t6 VALUES(11);"); > +box.execute("DROP TABLE t6") > +box.func.MYFUNC:drop() > + > test_run:cmd("clear filter") > -- > 2.23.0 > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 4/4] sql: use name instead of function pointer for UDF 2019-10-15 15:15 ` [Tarantool-patches] [tarantool-patches] [PATCH v4 4/4] sql: use name instead of function pointer for UDF Nikita Pettik @ 2019-10-16 13:51 ` Kirill Shcherbatov 2019-10-16 18:08 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Kirill Shcherbatov @ 2019-10-16 13:51 UTC (permalink / raw) To: tarantool-patches, Nikita Pettik; +Cc: tarantool-patches > Something is wrong with this test case. Firstly, it works fine > even on master branch. Secondly, I've dropped myfunc = nil and > got the same error (on master as well) - i.e. INSERT fails with > 'procedure is not defined' error even if function is not dropped. > Obviously, to make it crash on master you should drop function > before insertion; to fix second problem you should uppercase function's > name. You are right. A fixed test case is below. +-- +-- gh-4176: Can't recover if check constraint involves function. +-- +function myfunc(x) return x < 10 end +box.schema.func.create('MYFUNC', {param_list = {'integer'}, returns = 'integer', is_deterministic = true, exports = {'LUA', 'SQL'}}) +box.execute("CREATE TABLE t6(a INT CHECK (myfunc(a)) primary key);"); +box.func.MYFUNC:drop() +box.execute("INSERT INTO t6 VALUES(11);"); +box.execute("DROP TABLE t6") + test_run:cmd("clear filter") ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 4/4] sql: use name instead of function pointer for UDF 2019-10-16 13:51 ` [Tarantool-patches] [tarantool-patches] " Kirill Shcherbatov @ 2019-10-16 18:08 ` Nikita Pettik 0 siblings, 0 replies; 18+ messages in thread From: Nikita Pettik @ 2019-10-16 18:08 UTC (permalink / raw) To: Kirill Shcherbatov; +Cc: tarantool-patches, tarantool-patches On 16 Oct 16:51, Kirill Shcherbatov wrote: > > Something is wrong with this test case. Firstly, it works fine > > even on master branch. Secondly, I've dropped myfunc = nil and > > got the same error (on master as well) - i.e. INSERT fails with > > 'procedure is not defined' error even if function is not dropped. > > Obviously, to make it crash on master you should drop function > > before insertion; to fix second problem you should uppercase function's > > name. > You are right. A fixed test case is below. > > +-- > +-- gh-4176: Can't recover if check constraint involves function. > +-- > +function myfunc(x) return x < 10 end Still name is not upper-cased. Fixed and pushed. > +box.schema.func.create('MYFUNC', {param_list = {'integer'}, returns = 'integer', is_deterministic = true, exports = {'LUA', 'SQL'}}) > +box.execute("CREATE TABLE t6(a INT CHECK (myfunc(a)) primary key);"); > +box.func.MYFUNC:drop() > +box.execute("INSERT INTO t6 VALUES(11);"); > +box.execute("DROP TABLE t6") > + > test_run:cmd("clear filter") ^ permalink raw reply [flat|nested] 18+ messages in thread
[parent not found: <4eb8f545449842bc4c468ccf50c494e4c44c32d6.1570539526.git.kshcherbatov@tarantool.org>]
[parent not found: <20191013125109.GA24391@atlas>]
[parent not found: <7114925b-190a-4f0d-409f-974d2e6a65dd@tarantool.org>]
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery [not found] ` <7114925b-190a-4f0d-409f-974d2e6a65dd@tarantool.org> @ 2019-10-17 13:58 ` Nikita Pettik 2019-10-17 14:12 ` Konstantin Osipov 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2019-10-17 13:58 UTC (permalink / raw) To: tarantool-patches; +Cc: tarantool-patches On 15 Oct 14:13, Kirill Shcherbatov wrote: > > Tarantool used to validate ck constraints for all spaces on each > recovery. It is not good practice, because the recovery process > should be fast and data in snapshot is typically valid. > The new practice coincides with secondary keys building practice: > we do not perform consistency checks by default, except the > force_recovery = true option is specified. > > Closes #4176 Note that I haven't pushed this patch. It is not required to fix #4176, it is rather optimization issue. And there are still things to work on them. > --- > src/box/alter.cc | 8 +++- > src/box/box.cc | 8 ++-- > src/box/ck_constraint.c | 6 ++- > src/box/ck_constraint.h | 9 ++++- > src/box/schema.cc | 3 ++ > src/box/schema.h | 9 +++++ > src/box/space.c | 22 +++++++++++ > src/box/space.h | 7 ++++ > test/sql/checks.result | 79 ++++++++++++++++++++++++++++++++++++++++ > test/sql/checks.test.lua | 30 +++++++++++++++ > 10 files changed, 173 insertions(+), 8 deletions(-) > > diff --git a/src/box/alter.cc b/src/box/alter.cc > index f8f1802d0..83d297665 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -1558,7 +1558,7 @@ RebuildCkConstraints::prepare(struct alter_space *alter) > link) { > struct ck_constraint *new_ck_constraint = > ck_constraint_new(old_ck_constraint->def, > - alter->new_space->def); > + alter->new_space->def, true); > if (new_ck_constraint == NULL) > diag_raise(); > rlist_add_entry(&ck_constraint, new_ck_constraint, link); > @@ -4716,6 +4716,8 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) > if (old_def->language == ck_def->language && > strcmp(old_def->expr_str, ck_def->expr_str) == 0) { > old_def->is_enabled = ck_def->is_enabled; > + old_ck_constraint->is_enabled = > + is_force_recovery || ck_def->is_enabled; You made is_force_recovery be global. However, there's a lot of functions taking force_recovery param. Please, refactor them: remove that argument and use instead global status. What is more, you don't set is_force_recovery to false after recovery process is finished. What is more, AFAIR we discussed replacement of memtx_recovery_state with one global state shared among all engines (instead, you added is_recovery_complete). > trigger_run_xc(&on_alter_space, space); > return; > } > @@ -4730,7 +4732,9 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) > name, "referencing space must be empty"); > } > struct ck_constraint *new_ck_constraint = > - ck_constraint_new(ck_def, space->def); > + ck_constraint_new(ck_def, space->def, > + is_recovery_complete || > + is_force_recovery); > if (new_ck_constraint == NULL) > diag_raise(); > ck_def_guard.is_active = false; > diff --git a/src/box/box.cc b/src/box/box.cc > index baf029a09..85ae78b57 100644 > --- a/src/box/box.cc > +++ b/src/box/box.cc > @@ -1692,9 +1692,9 @@ engine_init() > * in checkpoints (in enigne_foreach order), > * so it must be registered first. > */ > + is_force_recovery = cfg_geti("force_recovery"); > struct memtx_engine *memtx; > - memtx = memtx_engine_new_xc(cfg_gets("memtx_dir"), > - cfg_geti("force_recovery"), > + memtx = memtx_engine_new_xc(cfg_gets("memtx_dir"), is_force_recovery, > cfg_getd("memtx_memory"), > cfg_geti("memtx_min_tuple_size"), > cfg_geti("strip_core"), > @@ -210,7 +210,7 @@ ck_constraint_on_replace_trigger(struct trigger *trigger, void *event) > > struct ck_constraint * > ck_constraint_new(struct ck_constraint_def *ck_constraint_def, > - struct space_def *space_def) > + struct space_def *space_def, bool is_enabled) > { > if (space_def->field_count == 0) { > diag_set(ClientError, ER_UNSUPPORTED, "Tarantool", > @@ -225,6 +225,8 @@ ck_constraint_new(struct ck_constraint_def *ck_constraint_def, > } > ck_constraint->def = NULL; > ck_constraint->stmt = NULL; > + ck_constraint->is_enabled = ck_constraint_def->is_enabled && is_enabled; > + > rlist_create(&ck_constraint->link); > struct Expr *expr = > sql_expr_compile(sql_get(), ck_constraint_def->expr_str, > diff --git a/src/box/ck_constraint.h b/src/box/ck_constraint.h > index 6761318d6..836ff2822 100644 > --- a/src/box/ck_constraint.h > +++ b/src/box/ck_constraint.h > @@ -98,6 +98,12 @@ struct ck_constraint { > * message when ck condition unsatisfied. > */ > struct sql_stmt *stmt; > + /** > + * Dispite ck_constraint_def::is_enabled property this > + * option defines a state of the runtine ck constraint > + * object and may be overridden manually. > + */ > + bool is_enabled; Duplication of is_enabled status is total mess IMHO. It is required only for recovery. Please, avoid introducing this flag. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery 2019-10-17 13:58 ` [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery Nikita Pettik @ 2019-10-17 14:12 ` Konstantin Osipov 2019-10-17 14:39 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Konstantin Osipov @ 2019-10-17 14:12 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches, tarantool-patches * Nikita Pettik <korablev@tarantool.org> [19/10/17 17:06]: > You made is_force_recovery be global. However, there's a lot of functions > taking force_recovery param. Please, refactor them: remove that argument > and use instead global status. What is more, you don't set is_force_recovery > to false after recovery process is finished. What is more, AFAIR we discussed > replacement of memtx_recovery_state with one global state shared among > all engines (instead, you added is_recovery_complete). Why do you think that now that there is a global state it would be good to use it everywhere? Besides, given that now there is is_force_recovery and is_recovery_complete, maybe have: engine_is_force_recovery() engine_is_recovery_complete() declared in engine.h, rather than directly access global variables? -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery 2019-10-17 14:12 ` Konstantin Osipov @ 2019-10-17 14:39 ` Nikita Pettik 2019-10-17 15:18 ` Konstantin Osipov 0 siblings, 1 reply; 18+ messages in thread From: Nikita Pettik @ 2019-10-17 14:39 UTC (permalink / raw) To: Konstantin Osipov, tarantool-patches, tarantool-patches, kshcherbatov On 17 Oct 17:12, Konstantin Osipov wrote: > * Nikita Pettik <korablev@tarantool.org> [19/10/17 17:06]: > > You made is_force_recovery be global. However, there's a lot of functions > > taking force_recovery param. Please, refactor them: remove that argument > > and use instead global status. What is more, you don't set is_force_recovery > > to false after recovery process is finished. What is more, AFAIR we discussed > > replacement of memtx_recovery_state with one global state shared among > > all engines (instead, you added is_recovery_complete). > > Why do you think that now that there is a global state it would be > good to use it everywhere? Em, I don't see any pros of duplicating global state by passing it as a parameter to functions. Meanwile using local version of force_recovery may turn out to be confusing (at least for developers looking into code). > Besides, given that now there is is_force_recovery and > is_recovery_complete, maybe have: > > engine_is_force_recovery() > engine_is_recovery_complete() > > declared in engine.h, rather than directly access global > variables? It's OK. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery 2019-10-17 14:39 ` Nikita Pettik @ 2019-10-17 15:18 ` Konstantin Osipov 2019-10-17 16:28 ` Nikita Pettik 0 siblings, 1 reply; 18+ messages in thread From: Konstantin Osipov @ 2019-10-17 15:18 UTC (permalink / raw) To: Nikita Pettik; +Cc: tarantool-patches, tarantool-patches * Nikita Pettik <korablev@tarantool.org> [19/10/17 17:41]: > Em, I don't see any pros of duplicating global state by passing it > as a parameter to functions. Meanwile using local version of force_recovery > may turn out to be confusing (at least for developers looking into code). Historically xlog.force_recovery is not the same as global force_recovery. The logic is a bit more tricky. There used to be panic_on_wal_error and panic_on_snap_error, and traces of it are still in place. -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery 2019-10-17 15:18 ` Konstantin Osipov @ 2019-10-17 16:28 ` Nikita Pettik 0 siblings, 0 replies; 18+ messages in thread From: Nikita Pettik @ 2019-10-17 16:28 UTC (permalink / raw) To: Konstantin Osipov; +Cc: tarantool-patches, tarantool-patches On 17 Oct 18:18, Konstantin Osipov wrote: > * Nikita Pettik <korablev@tarantool.org> [19/10/17 17:41]: > > Em, I don't see any pros of duplicating global state by passing it > > as a parameter to functions. Meanwile using local version of force_recovery > > may turn out to be confusing (at least for developers looking into code). > > Historically xlog.force_recovery is not the same as global > force_recovery. The logic is a bit more tricky. There used to be > panic_on_wal_error and panic_on_snap_error, and traces of it are > still in place. Ok, then I mean "refactor all places where global is_force_recover is the same as argument force_recovery". ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2019-10-17 16:28 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1570539526.git.kshcherbatov@tarantool.org>
[not found] ` <8232b0466f3878280a9ad35cb08f437610a36486.1570539526.git.kshcherbatov@tarantool.org>
2019-10-14 16:49 ` [Tarantool-patches] [tarantool-patches] [PATCH v4 1/4] box: add an ability to disable CK constraints Nikita Pettik
2019-10-15 11:13 ` [Tarantool-patches] [tarantool-patches] " Kirill Shcherbatov
2019-10-15 21:47 ` Nikita Pettik
2019-10-16 5:52 ` Konstantin Osipov
2019-10-16 11:19 ` Nikita Pettik
2019-10-16 13:50 ` Kirill Shcherbatov
2019-10-16 18:09 ` Nikita Pettik
[not found] ` <d4002407f749fff0c1f0facb1ed4cf66b8b7edd6.1570539526.git.kshcherbatov@tarantool.org>
2019-10-14 16:56 ` [Tarantool-patches] [tarantool-patches] [PATCH v4 2/4] sql: " Nikita Pettik
2019-10-15 11:13 ` [Tarantool-patches] [tarantool-patches] " Kirill Shcherbatov
2019-10-16 18:10 ` Nikita Pettik
[not found] ` <f462f55eebcb13abb8a0611a4d84d7ed8b1a6b6a.1570539526.git.kshcherbatov@tarantool.org>
[not found] ` <af095dba-bacd-e35f-9143-30ae59188697@tarantool.org>
2019-10-15 15:15 ` [Tarantool-patches] [tarantool-patches] [PATCH v4 4/4] sql: use name instead of function pointer for UDF Nikita Pettik
2019-10-16 13:51 ` [Tarantool-patches] [tarantool-patches] " Kirill Shcherbatov
2019-10-16 18:08 ` Nikita Pettik
[not found] ` <4eb8f545449842bc4c468ccf50c494e4c44c32d6.1570539526.git.kshcherbatov@tarantool.org>
[not found] ` <20191013125109.GA24391@atlas>
[not found] ` <7114925b-190a-4f0d-409f-974d2e6a65dd@tarantool.org>
2019-10-17 13:58 ` [Tarantool-patches] [tarantool-patches] Re: [PATCH v4 3/4] box: do not evaluate ck constraints on recovery Nikita Pettik
2019-10-17 14:12 ` Konstantin Osipov
2019-10-17 14:39 ` Nikita Pettik
2019-10-17 15:18 ` Konstantin Osipov
2019-10-17 16:28 ` Nikita Pettik
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox