From: Roman Khabibov <roman.habibov@tarantool.org> To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 2/3] box: make constraint operations transactional Date: Tue, 10 Dec 2019 15:49:00 +0300 [thread overview] Message-ID: <721EA98D-2EC6-4C4C-97B0-6BCEBE889EDA@tarantool.org> (raw) In-Reply-To: <f982e884-1835-22ff-4237-3ac001965bda@tarantool.org> > On Dec 7, 2019, at 19:35, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote: > > Thanks for the fixes! > > See 9 comments below. > >>> 14. Talking of the super mess with on_replace_dd_index, and >>> how tight it is now with all these struggling about replacing >>> constraints, and on_commit/on_rollback. I think, that this is >>> a dead end. Too complex. >>> >>> How about introducing a couple of new AlterSpaceOp classes? >>> AddConstraint, and DropConstraint. AlterSpaceOp looks good >>> because >>> - it will allow you to not set on_commit/on_rollback triggers >>> manually - it already provides these methods; >>> - you will be able to move this huge pieces of code out of >>> on_replace_dd_index and other on_replace_*; >>> - it is consistent with MoveIndex/DropIndex/etc classes. >>> >>> All you will need to do is to add to on_replace_* >>> (void) new AddConstraint(), and (void) new DropConstraint. >>> And you will keep (void) new MoveConstraints(). >>> >>> Also that should eliminate code duplication between different >>> on_replace_* functions about adding/dropping constraints. >> +/** >> + * CreateConstraintDef - add a new constraint def to the space. >> + */ >> +class CreateConstraintDef: public AlterSpaceOp >> +{ >> +private: >> + struct constraint_def *new_constraint_def; >> +public: >> + CreateConstraintDef(struct alter_space *alter, >> + struct constraint_def *def) >> + :AlterSpaceOp(alter), new_constraint_def(def) >> + {} >> + virtual void alter(struct alter_space *alter); >> + virtual void rollback(struct alter_space *alter); >> +}; >> + >> +void >> +CreateConstraintDef::alter(struct alter_space *alter) >> +{ >> + assert(new_constraint_def != NULL); >> + if (space_put_constraint(alter->old_space, new_constraint_def) != 0) >> + panic("Can't recover after constraint alter (out of memory)"); > > 1. Wow, why panic? Alter can safely fail. It it is not a commit or > rollback trigger. Maybe it can't return an error, but it can throw > it. > > (Although I would like to be able to panic on malloc fails, but it > does not seem to happen in the nearest future.) +void +CreateConstraintDef::alter(struct alter_space *alter) +{ + assert(new_constraint_def != NULL); + if (space_put_constraint(alter->old_space, new_constraint_def) != 0) + diag_raise(); +} >> +} >> + >> +void >> +CreateConstraintDef::rollback(struct alter_space *alter) >> +{ >> + assert(space_pop_constraint(alter->new_space, >> + new_constraint_def->name) == >> + new_constraint_def); > > 2. This will fail in Release build mode, because > assert arguments are not calculated in Release. So you > delete the object, but it is still in the hash table. +void +CreateConstraintDef::rollback(struct alter_space *alter) +{ + struct constraint_def *constraint_def = + space_pop_constraint(alter->new_space, + new_constraint_def->name); + assert(constraint_def == new_constraint_def); + (void) alter; + constraint_def_delete(new_constraint_def); +} >> + (void) alter; >> + constraint_def_delete(new_constraint_def); >> +} >> + >> >> I know, that I can combine these ifs to the one, but I did so, because I believe that >> it is more readable. > > 3. Yes, here I agree. > >> >> + struct index_def *old_def = old_index->def; >> + /* >> + * We put a new name either an index is becoming >> + * unique (i.e. constraint), or when an unique >> + * index's name is under change. >> + */ >> + if (!old_def->opts.is_unique && index_def->opts.is_unique && >> + create_index_as_constraint(alter, index_def) != 0) >> + return -1; >> + if (old_def->opts.is_unique && index_def->opts.is_unique && >> + strcmp(index_def->name, old_def->name) != 0 && >> + (create_index_as_constraint(alter, index_def) != 0 || >> + drop_index_as_constraint(alter, old_def) != 0)) >> + return -1; >> + if (old_def->opts.is_unique && !index_def->opts.is_unique && >> + drop_index_as_constraint(alter, old_def) != 0) >> + return -1; >> >> >> commit c54d2e89d660d2f0b07f1e6917327131362a6568 >> Author: Roman Khabibov <roman.habibov@tarantool.org> >> Date: Wed Oct 23 15:54:16 2019 +0300 >> >> sql: make constraint operations transactional >> >> Put constraint names into the space's hash table and drop them on >> replace in corresponding system spaces (_index, _fk_constraint, >> _ck_constraint). >> >> Closes #3503 >> >> @TarantoolBot document >> Title: Table constraints in SQL >> >> SQL: >> According to ANSI SQL, table constraint is one of the following >> entities: PRIMARY KEY, UNIQUE, FOREIGN KEY, CHECK. Every >> constraint have its own name passed by user or automatically >> generated. And these names must be unique within one table/space. >> Naming in SQL is case-insensitive, so "CONSTRAINT c" and >> "CONSTRAINT C" are the same. For example, you will get error, if >> you try to: >> >> CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0), >> CONSTRAINT c PRIMARY KEY (i)); >> >> or >> >> CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY); >> ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);'); >> >> The same is for any other constraint types. >> >> Lua/box: >> >> You also can create/drop table constraints from box. See >> space_object:create_check_constraint() and space_object:create_index() >> (if an index is unique). Naming in box is case-sensitive, so 'c' and >> 'C' are not the same (see naming policy). For example, an unique >> index is a constraint, but a non-unique index is not. So, a non-unique >> index can have the same name with a check/foreign key constraint >> within one space: >> >> box.execute('CREATE TABLE t2 (i INT PRIMARY KEY);'); >> box.execute('CREATE INDEX e ON t2(i);'); >> box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);'); >> >> But, if you try to: >> >> box.space.T2.index.E:alter({unique = true}); >> >> You will get error, beacuse index 'e' is becoming unique. > > 4. That is a good comment. (But typo: beacuse -> because.) Done. >> >> diff --git a/src/box/alter.cc b/src/box/alter.cc >> index bef25b605..c03bad589 100644 >> --- a/src/box/alter.cc >> +++ b/src/box/alter.cc> @@ -2510,18 +2649,23 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) >> if (old_index == NULL && new_tuple != NULL) { >> if (alter_space_move_indexes(alter, 0, iid)) >> return -1; >> + struct index_def *def = >> + index_def_new_from_tuple(new_tuple, old_space); >> + if (def == NULL) >> + return -1; >> + if (def->opts.is_unique && >> + create_index_as_constraint(alter, def) != 0) { >> + index_def_delete(def); >> + return -1; >> + } >> CreateIndex *create_index; >> try { >> create_index = new CreateIndex(alter); >> } catch (Exception *e) { >> return -1; > > 5. 'def' leaks here. @ -2510,18 +2669,24 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) if (old_index == NULL && new_tuple != NULL) { if (alter_space_move_indexes(alter, 0, iid)) return -1; + struct index_def *def = + index_def_new_from_tuple(new_tuple, old_space); + if (def == NULL) + return -1; + if (def->opts.is_unique && + create_index_as_constraint(alter, def) != 0) { + index_def_delete(def); + return -1; + } CreateIndex *create_index; try { create_index = new CreateIndex(alter); } catch (Exception *e) { + index_def_delete(def); return -1; } >> } >> - create_index->new_index_def = >> - index_def_new_from_tuple(new_tuple, old_space); >> - if (create_index->new_index_def == NULL) >> - return -1; >> - index_def_update_optionality(create_index->new_index_def, >> - alter->new_min_field_count); >> + create_index->new_index_def = def; >> + index_def_update_optionality(def, alter->new_min_field_count); >> } >> /* Case 3 and 4: check if we need to rebuild index data. */ >> if (old_index != NULL && new_tuple != NULL) { >> @@ -2531,6 +2675,23 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) >> return -1; >> auto index_def_guard = >> make_scoped_guard([=] { index_def_delete(index_def); }); >> + struct index_def *old_def = old_index->def; >> + /* >> + * We put a new name either an index is becoming > > 6. 'either' -> 'when either’. + /* + * We put a new name when either an index is + * becoming unique (i.e. constraint), or when an + * unique index's name is under change. + */ >> + * unique (i.e. constraint), or when an unique >> + * index's name is under change. >> + */ >> + if (!old_def->opts.is_unique && index_def->opts.is_unique && >> + create_index_as_constraint(alter, index_def) != 0) >> + return -1; >> + if (old_def->opts.is_unique && index_def->opts.is_unique && >> + strcmp(index_def->name, old_def->name) != 0 && >> + (create_index_as_constraint(alter, index_def) != 0 || >> + drop_index_as_constraint(alter, old_def) != 0)) >> + return -1; >> + if (old_def->opts.is_unique && !index_def->opts.is_unique && >> + drop_index_as_constraint(alter, old_def) != 0) >> + return -1; >> /* >> * To detect which key parts are optional, >> * min_field_count is required. But> @@ -5219,6 +5407,16 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event) >> fk); >> if (on_rollback == NULL) >> return -1; >> + struct constraint_def *constr_def = >> + constraint_def_new(child_space->def->id, >> + CONSTRAINT_TYPE_FK, >> + fk_def->name); >> + if (constr_def == NULL) >> + return -1; >> + if (space_put_constraint(child_space, constr_def) != 0) { >> + constraint_def_delete(constr_def); >> + return -1; >> + } > > 7. Sequence constraint_def_new -> space_put_constraint is repeated 4 > times. I think this is worth wrapping into a function. The same about > space_constraint_def_by_name -> space_pop_constraint -> > constraint_def_delete. +/** + * Put the node of a ck/fk constraint to the constraint hash table + * of @a space. + * + * This function is needed to wrap the duplicated piece of code + * inside the trigger functions below. + */ +static int +create_fk_ck_as_constraint(struct space *space, enum constraint_type type, + const char *name) +{ + assert(type == CONSTRAINT_TYPE_CK || type == CONSTRAINT_TYPE_FK); + struct constraint_def *constraint_def = + constraint_def_new(space->def->id, type, name); + if (constraint_def == NULL) + return -1; + if (space_put_constraint(space, constraint_def) != 0) { + constraint_def_delete(constraint_def); + return -1; + } + return 0; +} + +/** + * Drop the node of a ck/fk constraint from the constraint hash + * table of @a space. + * + * This function is needed to wrap the duplicated piece of code + * inside the trigger functions below. + */ +static void +drop_fk_ck_as_constraint(struct space *space, const char *name) +{ + struct constraint_def *constraint_def = + space_constraint_def_by_name(space, name); + assert(constraint_def != NULL); + assert(constraint_def->type == CONSTRAINT_TYPE_CK || + constraint_def->type == CONSTRAINT_TYPE_FK); + space_pop_constraint(space, name); + constraint_def_delete(constraint_def); +} + >> txn_stmt_on_rollback(stmt, on_rollback); >> fk_constraint_set_mask(fk, >> &parent_space->fk_constraint_mask, >> diff --git a/test/sql/constraint.result b/test/sql/constraint.result >> new file mode 100644 >> index 000000000..ba262182b >> --- /dev/null >> +++ b/test/sql/constraint.result >> @@ -0,0 +1,190 @@ >> +-- test-run result file version 2 >> +test_run = require('test_run').new() >> + | --- >> + | ... >> +engine = test_run:get_cfg('engine') >> + | --- >> + | ... >> +box.execute('pragma sql_default_engine=\''..engine..'\'') >> + | --- >> + | - row_count: 0 >> + | ... >> +test_run:cmd("setopt delimiter ';'") > > 8. Please, don't. There is only one small place where you need > multiple lines. You can either use ';' just for it, or use /. > >> diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua >> new file mode 100755 >> index 000000000..50162e47d >> --- /dev/null >> +++ b/test/sql/constraint.test.lua > > 9. Your test leaves lots of not deleted data. Please, do > cleanup. Done. commit c90d7935b56c4603303bac45a7ebeb9af03eef3d Author: Roman Khabibov <roman.habibov@tarantool.org> Date: Wed Oct 23 15:54:16 2019 +0300 box: make constraint operations transactional Put constraint names into the space's hash table and drop them on replace in corresponding system spaces (_index, _fk_constraint, _ck_constraint). Closes #3503 @TarantoolBot document Title: Table constraints in SQL SQL: According to ANSI SQL, table constraint is one of the following entities: PRIMARY KEY, UNIQUE, FOREIGN KEY, CHECK. Every constraint have its own name passed by user or automatically generated. And these names must be unique within one table/space. Naming in SQL is case-insensitive, so "CONSTRAINT c" and "CONSTRAINT C" are the same. For example, you will get error, if you try to: CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0), CONSTRAINT c PRIMARY KEY (i)); or CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY); ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);'); The same is for any other constraint types. Lua/box: You also can create/drop table constraints from box. See space_object:create_check_constraint() and space_object:create_index() (if an index is unique). Naming in box is case-sensitive, so 'c' and 'C' are not the same (see naming policy). For example, an unique index is a constraint, but a non-unique index is not. So, a non-unique index can have the same name with a check/foreign key constraint within one space: box.execute('CREATE TABLE t2 (i INT PRIMARY KEY);'); box.execute('CREATE INDEX e ON t2(i);'); box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);'); But, if you try to: box.space.T2.index.E:alter({unique = true}); You will get error, because index 'e' is becoming unique. diff --git a/src/box/alter.cc b/src/box/alter.cc index bef25b605..d5e836ae7 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -57,6 +57,7 @@ #include "version.h" #include "sequence.h" #include "sql.h" +#include "constraint_def.h" /* {{{ Auxiliary functions and methods. */ @@ -1033,10 +1034,13 @@ alter_space_rollback(struct trigger *trigger, void * /* event */) space_fill_index_map(alter->old_space); space_fill_index_map(alter->new_space); /* - * Don't forget about space triggers and foreign keys. + * Don't forget about space triggers, foreign keys and + * constraints. */ space_swap_triggers(alter->new_space, alter->old_space); space_swap_fk_constraints(alter->new_space, alter->old_space); + SWAP(alter->new_space->constraints, + alter->old_space->constraints); space_cache_replace(alter->new_space, alter->old_space); alter_space_delete(alter); return 0; @@ -1143,10 +1147,13 @@ alter_space_do(struct txn_stmt *stmt, struct alter_space *alter) space_fill_index_map(alter->old_space); space_fill_index_map(alter->new_space); /* - * Don't forget about space triggers and foreign keys. + * Don't forget about space triggers, foreign keys and + * constraints. */ space_swap_triggers(alter->new_space, alter->old_space); space_swap_fk_constraints(alter->new_space, alter->old_space); + SWAP(alter->new_space->constraints, + alter->old_space->constraints); /* * The new space is ready. Time to update the space * cache with it. @@ -1764,6 +1771,85 @@ MoveCkConstraints::rollback(struct alter_space *alter) space_swap_ck_constraint(alter->new_space, alter->old_space); } +/** + * CreateConstraintDef - add a new constraint def to the space. + */ +class CreateConstraintDef: public AlterSpaceOp +{ +private: + struct constraint_def *new_constraint_def; +public: + CreateConstraintDef(struct alter_space *alter, + struct constraint_def *def) + :AlterSpaceOp(alter), new_constraint_def(def) + {} + virtual void alter(struct alter_space *alter); + virtual void rollback(struct alter_space *alter); +}; + +void +CreateConstraintDef::alter(struct alter_space *alter) +{ + assert(new_constraint_def != NULL); + if (space_put_constraint(alter->old_space, new_constraint_def) != 0) + diag_raise(); +} + +void +CreateConstraintDef::rollback(struct alter_space *alter) +{ + struct constraint_def *constraint_def = + space_pop_constraint(alter->new_space, + new_constraint_def->name); + assert(constraint_def == new_constraint_def); + (void) alter; + constraint_def_delete(new_constraint_def); +} + +/** DropConstraintDef - drop a constraint def from the space. */ +class DropConstraintDef: public AlterSpaceOp +{ +private: + struct constraint_def *old_constraint_def; + const char *name; +public: + DropConstraintDef(struct alter_space *alter, const char *name) + :AlterSpaceOp(alter), old_constraint_def(NULL), name(name) + {} + virtual void alter(struct alter_space *alter); + virtual void commit(struct alter_space *alter , int64_t signature); + virtual void rollback(struct alter_space *alter); +}; + +void +DropConstraintDef::alter(struct alter_space *alter) +{ + assert(name != NULL); + old_constraint_def = + space_constraint_def_by_name(alter->old_space, name); + assert(old_constraint_def != NULL); + space_pop_constraint(alter->old_space, name); +} + +void +DropConstraintDef::commit(struct alter_space *alter, int64_t signature) +{ + (void) alter; + (void) signature; + assert(old_constraint_def != NULL); + constraint_def_delete(old_constraint_def); +} + +void +DropConstraintDef::rollback(struct alter_space *alter) +{ + assert(old_constraint_def != NULL); + if (space_put_constraint(alter->new_space, old_constraint_def) != 0) { + panic("Can't recover after constraint drop rollback (out of " + "memory)"); + } +} + /* }}} */ /** @@ -2363,6 +2449,76 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid) return false; } +/** + * Check if constraint with @a name exists within @a space. Call + * diag_set() if it is so. + * + * @param space Space to find constraint within. + * @param name Constraint name. + * + * @retval -1 Constraint exists. + * @retval 0 Doesn't exist. + */ +static inline int +check_existence(struct space *space, const char *name) +{ + struct constraint_def *def = space_constraint_def_by_name(space, name); + if (def != NULL) { + diag_set(ClientError, ER_CONSTRAINT_EXISTS, name); + return -1; + } + return 0; +} + +/** + * Put the node of an unique index to the constraint hash table of + * @a space. + * + * This function is needed to wrap the duplicated piece of code + * inside on_replace_dd_index(). + */ +static int +create_index_as_constraint(struct alter_space *alter, + const struct index_def *def) +{ + assert(def->opts.is_unique); + struct space *space = alter->old_space; + if (check_existence(space, def->name) != 0) + return -1; + struct constraint_def *constr_def = + constraint_def_new(space->def->id, def->iid == 0 ? + CONSTRAINT_TYPE_PK : CONSTRAINT_TYPE_UNIQUE, + def->name); + if (constr_def == NULL) + return -1; + try { + (void) new CreateConstraintDef(alter, constr_def); + } catch (Exception *e) { + constraint_def_delete(constr_def); + return -1; + } + return 0; +} + +/** + * Drop the node of an unique index from the constraint hash table + * of @a space. + * + * This function is needed to wrap the duplicated piece of code + * inside on_replace_dd_index(). + */ +static int +drop_index_as_constraint(struct alter_space *alter, const struct index_def *def) +{ + assert(def->opts.is_unique); + try { + (void) new DropConstraintDef(alter, def->name); + } catch (Exception *e) { + return -1; + } + return 0; +} + /** * Just like with _space, 3 major cases: * @@ -2500,6 +2656,9 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) } if (alter_space_move_indexes(alter, 0, iid) != 0) return -1; + if (old_index->def->opts.is_unique && + drop_index_as_constraint(alter, old_index->def) != 0) + return -1; try { (void) new DropIndex(alter, old_index); } catch (Exception *e) { @@ -2510,18 +2669,24 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) if (old_index == NULL && new_tuple != NULL) { if (alter_space_move_indexes(alter, 0, iid)) return -1; + struct index_def *def = + index_def_new_from_tuple(new_tuple, old_space); + if (def == NULL) + return -1; + if (def->opts.is_unique && + create_index_as_constraint(alter, def) != 0) { + index_def_delete(def); + return -1; + } CreateIndex *create_index; try { create_index = new CreateIndex(alter); } catch (Exception *e) { + index_def_delete(def); return -1; } - create_index->new_index_def = - index_def_new_from_tuple(new_tuple, old_space); - if (create_index->new_index_def == NULL) - return -1; - index_def_update_optionality(create_index->new_index_def, - alter->new_min_field_count); + create_index->new_index_def = def; + index_def_update_optionality(def, alter->new_min_field_count); } /* Case 3 and 4: check if we need to rebuild index data. */ if (old_index != NULL && new_tuple != NULL) { @@ -2531,6 +2696,23 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event) return -1; auto index_def_guard = make_scoped_guard([=] { index_def_delete(index_def); }); + struct index_def *old_def = old_index->def; + /* + * We put a new name when either an index is + * becoming unique (i.e. constraint), or when an + * unique index's name is under change. + */ + if (!old_def->opts.is_unique && index_def->opts.is_unique && + create_index_as_constraint(alter, index_def) != 0) + return -1; + if (old_def->opts.is_unique && index_def->opts.is_unique && + strcmp(index_def->name, old_def->name) != 0 && + (create_index_as_constraint(alter, index_def) != 0 || + drop_index_as_constraint(alter, old_def) != 0)) + return -1; + if (old_def->opts.is_unique && !index_def->opts.is_unique && + drop_index_as_constraint(alter, old_def) != 0) + return -1; /* * To detect which key parts are optional, * min_field_count is required. But @@ -4984,6 +5166,48 @@ space_reset_fk_constraint_mask(struct space *space) } } +/** + * Put the node of a ck/fk constraint to the constraint hash table + * of @a space. + * + * This function is needed to wrap the duplicated piece of code + * inside the trigger functions below. + */ +static int +create_fk_ck_as_constraint(struct space *space, enum constraint_type type, + const char *name) +{ + assert(type == CONSTRAINT_TYPE_CK || type == CONSTRAINT_TYPE_FK); + struct constraint_def *constraint_def = + constraint_def_new(space->def->id, type, name); + if (constraint_def == NULL) + return -1; + if (space_put_constraint(space, constraint_def) != 0) { + constraint_def_delete(constraint_def); + return -1; + } + return 0; +} + +/** + * Drop the node of a ck/fk constraint from the constraint hash + * table of @a space. + * + * This function is needed to wrap the duplicated piece of code + * inside the trigger functions below. + */ +static void +drop_fk_ck_as_constraint(struct space *space, const char *name) +{ + struct constraint_def *constraint_def = + space_constraint_def_by_name(space, name); + assert(constraint_def != NULL); + assert(constraint_def->type == CONSTRAINT_TYPE_CK || + constraint_def->type == CONSTRAINT_TYPE_FK); + space_pop_constraint(space, name); + constraint_def_delete(constraint_def); +} + /** * On rollback of creation we remove FK constraint from DD, i.e. * from parent's and child's lists of constraints and @@ -4996,8 +5220,11 @@ on_create_fk_constraint_rollback(struct trigger *trigger, void *event) struct fk_constraint *fk = (struct fk_constraint *)trigger->data; rlist_del_entry(fk, in_parent_space); rlist_del_entry(fk, in_child_space); + struct space *child = space_by_id(fk->def->child_id); + assert(child != NULL); + drop_fk_ck_as_constraint(child, fk->def->name); space_reset_fk_constraint_mask(space_by_id(fk->def->parent_id)); - space_reset_fk_constraint_mask(space_by_id(fk->def->child_id)); + space_reset_fk_constraint_mask(child); fk_constraint_delete(fk); return 0; } @@ -5029,8 +5256,14 @@ on_drop_fk_constraint_rollback(struct trigger *trigger, void *event) struct fk_constraint *old_fk = (struct fk_constraint *)trigger->data; struct space *parent = space_by_id(old_fk->def->parent_id); struct space *child = space_by_id(old_fk->def->child_id); + assert(child != NULL && parent != NULL); rlist_add_entry(&child->child_fk_constraint, old_fk, in_child_space); rlist_add_entry(&parent->parent_fk_constraint, old_fk, in_parent_space); + if (create_fk_ck_as_constraint(child, CONSTRAINT_TYPE_FK, + old_fk->def->name) != 0) { + panic("Can't recover after FK constraint drop rollback (out of " + "memory)"); + } fk_constraint_set_mask(old_fk, &child->fk_constraint_mask, FIELD_LINK_CHILD); fk_constraint_set_mask(old_fk, &parent->fk_constraint_mask, @@ -5210,15 +5443,21 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event) fk->def = fk_def; fk->index_id = fk_index->def->iid; if (old_tuple == NULL) { - rlist_add_entry(&child_space->child_fk_constraint, - fk, in_child_space); - rlist_add_entry(&parent_space->parent_fk_constraint, - fk, in_parent_space); + if (check_existence(child_space, fk_def->name) != 0) + return -1; struct trigger *on_rollback = txn_alter_trigger_new(on_create_fk_constraint_rollback, fk); if (on_rollback == NULL) return -1; + if (create_fk_ck_as_constraint(child_space, + CONSTRAINT_TYPE_FK, + fk_def->name) != 0) + return -1; + rlist_add_entry(&child_space->child_fk_constraint, + fk, in_child_space); + rlist_add_entry(&parent_space->parent_fk_constraint, + fk, in_parent_space); txn_stmt_on_rollback(stmt, on_rollback); fk_constraint_set_mask(fk, &parent_space->fk_constraint_mask, @@ -5279,6 +5518,7 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event) old_fk); if (on_rollback == NULL) return -1; + drop_fk_ck_as_constraint(child_space, fk_def->name); txn_stmt_on_rollback(stmt, on_rollback); space_reset_fk_constraint_mask(child_space); space_reset_fk_constraint_mask(parent_space); @@ -5344,9 +5584,10 @@ on_create_ck_constraint_rollback(struct trigger *trigger, void * /* event */) assert(ck != NULL); struct space *space = space_by_id(ck->def->space_id); assert(space != NULL); - assert(space_ck_constraint_by_name(space, ck->def->name, - strlen(ck->def->name)) != NULL); + const char *name = ck->def->name; + assert(space_ck_constraint_by_name(space, name, strlen(name)) != NULL); space_remove_ck_constraint(space, ck); + drop_fk_ck_as_constraint(space, name); ck_constraint_delete(ck); if (trigger_run(&on_alter_space, space) != 0) return -1; @@ -5371,9 +5612,10 @@ on_drop_ck_constraint_rollback(struct trigger *trigger, void * /* event */) assert(ck != NULL); struct space *space = space_by_id(ck->def->space_id); assert(space != NULL); - assert(space_ck_constraint_by_name(space, ck->def->name, - strlen(ck->def->name)) == NULL); - if (space_add_ck_constraint(space, ck) != 0) + const char *name = ck->def->name; + assert(space_ck_constraint_by_name(space, name, strlen(name)) == NULL); + if (space_add_ck_constraint(space, ck) != 0 || + create_fk_ck_as_constraint(space, CONSTRAINT_TYPE_CK, name) != 0) panic("Can't recover after CK constraint drop rollback"); if (trigger_run(&on_alter_space, space) != 0) return -1; @@ -5459,7 +5701,8 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) 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) { + bool is_insert = old_ck_constraint == NULL; + if (!is_insert) { struct ck_constraint_def *old_def = old_ck_constraint->def; assert(old_def->space_id == ck_def->space_id); @@ -5491,10 +5734,15 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) auto ck_guard = make_scoped_guard([=] { ck_constraint_delete(new_ck_constraint); }); - if (old_ck_constraint != NULL) - rlist_del_entry(old_ck_constraint, link); - if (space_add_ck_constraint(space, new_ck_constraint) != 0) + if (is_insert && check_existence(space, name) != 0) + return -1; + if (space_add_ck_constraint(space, new_ck_constraint) != 0 || + (is_insert && + create_fk_ck_as_constraint(space, CONSTRAINT_TYPE_CK, + name) != 0)) return -1; + if (!is_insert) + rlist_del_entry(old_ck_constraint, link); ck_guard.is_active = false; if (old_tuple != NULL) { on_rollback->data = old_ck_constraint; @@ -5516,6 +5764,7 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event) struct ck_constraint *old_ck_constraint = space_ck_constraint_by_name(space, name, name_len); assert(old_ck_constraint != NULL); + drop_fk_ck_as_constraint(space, old_ck_constraint->def->name); space_remove_ck_constraint(space, old_ck_constraint); on_commit->data = old_ck_constraint; on_commit->run = on_drop_ck_constraint_commit; diff --git a/test/sql/constraint.result b/test/sql/constraint.result new file mode 100644 index 000000000..a3dd7d531 --- /dev/null +++ b/test/sql/constraint.result @@ -0,0 +1,214 @@ +-- test-run result file version 2 +test_run = require('test_run').new() + | --- + | ... +engine = test_run:get_cfg('engine') + | --- + | ... +box.execute('pragma sql_default_engine=\''..engine..'\'') + | --- + | - row_count: 0 + | ... + +-- +-- Check a constraint name for duplicate within a single +-- <CREATE TABLE> statement. +-- +box.execute('CREATE TABLE t1 (i INT PRIMARY KEY);') + | --- + | - row_count: 1 + | ... +test_run:cmd("setopt delimiter ';'"); + | --- + | - true + | ... +box.execute([[CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0), + CONSTRAINT c PRIMARY KEY (i));]]); + | --- + | - null + | - Constraint C already exists + | ... +box.execute([[CREATE TABLE t2 (i INT, + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i), + CONSTRAINT c PRIMARY KEY (i));]]); + | --- + | - null + | - Constraint C already exists + | ... +box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY, + CONSTRAINT c CHECK (i > 0), + CONSTRAINT c UNIQUE (i));]]); + | --- + | - null + | - Constraint C already exists + | ... +box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY, + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i), + CONSTRAINT c UNIQUE (i));]]); + | --- + | - null + | - Constraint C already exists + | ... +box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY, + CONSTRAINT c CHECK (i > 0), + CONSTRAINT c CHECK (i < 0));]]); + | --- + | - null + | - Constraint C already exists + | ... +box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY, + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i), + CONSTRAINT c CHECK (i > 0));]]); + | --- + | - null + | - Constraint C already exists + | ... +box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY CONSTRAINT c REFERENCES t1(i), + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i))]]); + | --- + | - null + | - Constraint C already exists + | ... +test_run:cmd("setopt delimiter ''"); + | --- + | - true + | ... + +-- +-- Check a constraint name for duplicate using <ALTER TABLE> +-- statement. +-- +box.execute('CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY);') + | --- + | - row_count: 1 + | ... +box.execute('ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);') + | --- + | - null + | - Constraint C already exists + | ... +box.execute('ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);') + | --- + | - null + | - Index 'C' already exists in space 'T2' + | ... +box.execute('ALTER TABLE t2 ADD CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i);') + | --- + | - null + | - Constraint C already exists + | ... + +-- +-- Make sure that a constraint's name isn't kept after the +-- constraint drop. +-- +box.execute('CREATE UNIQUE INDEX d ON t2(i);') + | --- + | - row_count: 1 + | ... +box.execute('DROP INDEX d ON t2;') + | --- + | - row_count: 1 + | ... +box.execute('ALTER TABLE t2 ADD CONSTRAINT d CHECK(i > 0);') + | --- + | - row_count: 1 + | ... +box.space.T2.ck_constraint.D:drop() + | --- + | ... +box.execute('ALTER TABLE t2 ADD CONSTRAINT d FOREIGN KEY(i) REFERENCES t1(i);') + | --- + | - row_count: 1 + | ... +box.execute('ALTER TABLE t2 DROP CONSTRAINT d;') + | --- + | - row_count: 1 + | ... + +-- +-- The same inside a transaction. +-- +test_run:cmd("setopt delimiter ';'"); + | --- + | - true + | ... +box.begin() +box.execute('CREATE UNIQUE INDEX d ON t2(i);') +box.execute('DROP INDEX d ON t2;') +box.execute('ALTER TABLE t2 ADD CONSTRAINT d CHECK(i > 0);') +box.space.T2.ck_constraint.D:drop() +box.execute('ALTER TABLE t2 ADD CONSTRAINT d FOREIGN KEY(i) REFERENCES t1(i);') +box.execute('ALTER TABLE t2 DROP CONSTRAINT d;') +box.commit(); + | --- + | ... +test_run:cmd("setopt delimiter ''"); + | --- + | - true + | ... + +-- +-- Make sure, that altering of an index name affect to its record +-- in the space's constraint hash table. +-- +box.execute('CREATE UNIQUE INDEX d ON t2(i);') + | --- + | - row_count: 1 + | ... +box.space.T2.index.D:alter({name = 'E'}) + | --- + | ... +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') + | --- + | - null + | - Constraint E already exists + | ... + +-- +-- Make sure, that altering of an index uniqueness puts/drops +-- its name to/from the space's constraint hash table. +-- +box.space.T2.index.E:alter({unique = false}) + | --- + | ... +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') + | --- + | - row_count: 1 + | ... +box.space.T2.index.E:alter({unique = true}) + | --- + | - error: Constraint E already exists + | ... +box.space.T2.ck_constraint.E:drop() + | --- + | ... +box.space.T2.index.E:alter({unique = true}) + | --- + | ... +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') + | --- + | - null + | - Constraint E already exists + | ... + +-- Alter name and uniqueness of an unique index simultaneously. +box.space.T2.index.E:alter({name = 'D', unique = false}) + | --- + | ... +box.execute('CREATE UNIQUE INDEX e ON t2(i);') + | --- + | - row_count: 1 + | ... + +-- +-- Cleanup. +-- +box.execute('DROP TABLE t2;') + | --- + | - row_count: 1 + | ... +box.execute('DROP TABLE t1;') + | --- + | - row_count: 1 + | ... diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua new file mode 100755 index 000000000..e321b87aa --- /dev/null +++ b/test/sql/constraint.test.lua @@ -0,0 +1,93 @@ +test_run = require('test_run').new() +engine = test_run:get_cfg('engine') +box.execute('pragma sql_default_engine=\''..engine..'\'') + +-- +-- Check a constraint name for duplicate within a single +-- <CREATE TABLE> statement. +-- +box.execute('CREATE TABLE t1 (i INT PRIMARY KEY);') +test_run:cmd("setopt delimiter ';'"); +box.execute([[CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0), + CONSTRAINT c PRIMARY KEY (i));]]); +box.execute([[CREATE TABLE t2 (i INT, + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i), + CONSTRAINT c PRIMARY KEY (i));]]); +box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY, + CONSTRAINT c CHECK (i > 0), + CONSTRAINT c UNIQUE (i));]]); +box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY, + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i), + CONSTRAINT c UNIQUE (i));]]); +box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY, + CONSTRAINT c CHECK (i > 0), + CONSTRAINT c CHECK (i < 0));]]); +box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY, + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i), + CONSTRAINT c CHECK (i > 0));]]); +box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY CONSTRAINT c REFERENCES t1(i), + CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i))]]); +test_run:cmd("setopt delimiter ''"); + +-- +-- Check a constraint name for duplicate using <ALTER TABLE> +-- statement. +-- +box.execute('CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY);') +box.execute('ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);') +box.execute('ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);') +box.execute('ALTER TABLE t2 ADD CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i);') + +-- +-- Make sure that a constraint's name isn't kept after the +-- constraint drop. +-- +box.execute('CREATE UNIQUE INDEX d ON t2(i);') +box.execute('DROP INDEX d ON t2;') +box.execute('ALTER TABLE t2 ADD CONSTRAINT d CHECK(i > 0);') +box.space.T2.ck_constraint.D:drop() +box.execute('ALTER TABLE t2 ADD CONSTRAINT d FOREIGN KEY(i) REFERENCES t1(i);') +box.execute('ALTER TABLE t2 DROP CONSTRAINT d;') + +-- +-- The same inside a transaction. +-- +test_run:cmd("setopt delimiter ';'"); +box.begin() +box.execute('CREATE UNIQUE INDEX d ON t2(i);') +box.execute('DROP INDEX d ON t2;') +box.execute('ALTER TABLE t2 ADD CONSTRAINT d CHECK(i > 0);') +box.space.T2.ck_constraint.D:drop() +box.execute('ALTER TABLE t2 ADD CONSTRAINT d FOREIGN KEY(i) REFERENCES t1(i);') +box.execute('ALTER TABLE t2 DROP CONSTRAINT d;') +box.commit(); +test_run:cmd("setopt delimiter ''"); + +-- +-- Make sure, that altering of an index name affect to its record +-- in the space's constraint hash table. +-- +box.execute('CREATE UNIQUE INDEX d ON t2(i);') +box.space.T2.index.D:alter({name = 'E'}) +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') + +-- +-- Make sure, that altering of an index uniqueness puts/drops +-- its name to/from the space's constraint hash table. +-- +box.space.T2.index.E:alter({unique = false}) +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') +box.space.T2.index.E:alter({unique = true}) +box.space.T2.ck_constraint.E:drop() +box.space.T2.index.E:alter({unique = true}) +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') + +-- Alter name and uniqueness of an unique index simultaneously. +box.space.T2.index.E:alter({name = 'D', unique = false}) +box.execute('CREATE UNIQUE INDEX e ON t2(i);') + +-- +-- Cleanup. +-- +box.execute('DROP TABLE t2;') +box.execute('DROP TABLE t1;')
next prev parent reply other threads:[~2019-12-10 12:49 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-28 18:34 [Tarantool-patches] [PATCH v2 0/2] Add constraint names hash table to space Roman Khabibov 2019-11-28 18:34 ` [Tarantool-patches] [PATCH v2 1/2] box: introduce constraint names hash table Roman Khabibov 2019-11-30 1:03 ` Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 1/3] " Roman Khabibov 2019-12-07 16:34 ` Vladislav Shpilevoy 2019-12-10 12:48 ` Roman Khabibov 2019-11-28 18:34 ` [Tarantool-patches] [PATCH v2 2/2] sql: make constraint operations transactional Roman Khabibov 2019-11-29 7:38 ` Roman Khabibov 2019-11-30 1:03 ` Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 2/3] " Roman Khabibov 2019-12-05 18:43 ` Roman Khabibov 2019-12-07 16:35 ` Vladislav Shpilevoy 2019-12-10 12:49 ` Roman Khabibov [this message] 2019-12-15 22:26 ` [Tarantool-patches] [PATCH v2 2/3] box: " Vladislav Shpilevoy 2019-12-17 15:03 ` Roman Khabibov 2019-12-28 0:18 ` Nikita Pettik 2019-12-28 11:07 ` Vladislav Shpilevoy 2019-12-29 0:07 ` Nikita Pettik 2019-12-29 15:51 ` Vladislav Shpilevoy 2019-12-29 22:28 ` Nikita Pettik 2019-12-29 22:35 ` Vladislav Shpilevoy 2019-12-30 11:12 ` Sergey Ostanevich 2019-12-30 12:05 ` Nikita Pettik 2019-12-21 20:54 ` Sergey Ostanevich 2019-12-22 14:59 ` Vladislav Shpilevoy 2019-12-24 12:06 ` Roman Khabibov 2019-11-30 1:03 ` [Tarantool-patches] [PATCH v2 0/2] Add constraint names hash table to space Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 0/3] " Roman Khabibov 2019-12-07 16:34 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=721EA98D-2EC6-4C4C-97B0-6BCEBE889EDA@tarantool.org \ --to=roman.habibov@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --cc=v.shpilevoy@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 2/3] box: make constraint operations transactional' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox