From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 4A08446971A for ; Wed, 4 Dec 2019 19:23:36 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3594.4.19\)) From: Roman Khabibov In-Reply-To: <77b2090c-4bfd-c42c-e744-3395eef2e8a3@tarantool.org> Date: Wed, 4 Dec 2019 19:23:34 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <4BB41329-C9B8-4206-9424-BE5DE3280EF7@tarantool.org> References: <1b936f83be688cbbc8a1625be61a2841809b5633.1574965971.git.roman.habibov@tarantool.org> <77b2090c-4bfd-c42c-e744-3395eef2e8a3@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH v2 2/3] sql: make constraint operations transactional List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Cc: Vladislav Shpilevoy > On Nov 30, 2019, at 04:03, Vladislav Shpilevoy = wrote: >=20 > Thanks for the patch! >=20 > The commit below (or at least the commit message) was outdated. > I see a different version on the branch. Below I copypaste the > branch's version, and review it. >=20 > See 14 comments below. >=20 >> Put constraint names into the space's hash table and drop them on >> replace in correspondig system spaces (_index, _fk_constraint, >=20 > 1. correspondig -> corresponding Done. >> _ck_constraint). >>=20 >> Closes #3503 >>=20 >> @TarantoolBot document >> Title: Constraints >> Now, names of constraints must be unique within one space. >=20 > 2. Now, imagine that you are a technical writer. You don't > run and test Tarantool every day. And you don't know what, > how, when, and where do we support. What are the problems, > features, plans. Your task is simple - you take a ticket > from tarantool/doc, and transform it into something that is > ok to be placed on the site. Into a good English text, with > rich details, with omission of not needed information. You > may fix some of the old documentation affected by that ticket. > You may add pictures, diagrams. >=20 > And then imagine you got such a ticket as you provided here. > Sorry, but this is just nothing. That is like if I would ask > you to 'fix an assertion in box'. What assertion? Where in > the box? What is a reproducer? >=20 > You need to explain, what do you mean as constraints; what > if uniqueness is violated; does letter case matter; what are > examples; does language matter (Lua, SQL, binary) etc. Done. >>=20 >> diff --git a/src/box/alter.cc b/src/box/alter.cc >> index 4a3241a79..4bb8b8556 100644 >> --- a/src/box/alter.cc >> +++ b/src/box/alter.cc >> @@ -1765,6 +1766,38 @@ MoveCkConstraints::rollback(struct alter_space = *alter) >> space_swap_ck_constraint(alter->new_space, alter->old_space); >> } >>=20 >> +/** >> + * Move constraint names hash table from old space to a new one. >> + */ >> +class MoveConstraints: public AlterSpaceOp >> +{ >> + inline void space_swap_constraint(struct space *old_space, >> + struct space *new_space); >> +public: >> + MoveConstraints(struct alter_space *alter) : AlterSpaceOp(alter) = {} >> + virtual void alter(struct alter_space *alter); >> + virtual void rollback(struct alter_space *alter); >> +}; >> + >> +inline void >> +MoveConstraints::space_swap_constraint(struct space *old_space, >> + struct space *new_space) >> +{ >> + SWAP(new_space->constraint_names, old_space->constraint_names); >=20 > 3. So this is not inlined as a I asked. Perhaps that was > misunderstanding. I meant literally inline it. Not add > 'inline' keyword. That is a one-liner function, which calls > another one liner. Even with the same number of arguments. > What is a point of having it? It will never even be extended, > and does not make the code more compact. Isn=E2=80=99t it better to SWAP() them in alter_space_do() and = alter_space_rollback() without new class adding? +/** + * Move constraint names hash table from old space to a new one. + */ +class MoveConstraintDefs: public AlterSpaceOp +{ +public: + MoveConstraintDefs(struct alter_space *alter) : = AlterSpaceOp(alter) {} + virtual void alter(struct alter_space *alter); + virtual void rollback(struct alter_space *alter); +}; + +void +MoveConstraintDefs::alter(struct alter_space *alter) +{ + SWAP(alter->new_space->constraint_names, + alter->old_space->constraint_names); +} + +void +MoveConstraintDefs::rollback(struct alter_space *alter) +{ + SWAP(alter->new_space->constraint_names, + alter->old_space->constraint_names); +} + >> +} >> + >> +void >> +MoveConstraints::alter(struct alter_space *alter) >> +{ >> + space_swap_constraint(alter->old_space, alter->new_space); >> +} >> + >> +void >> +MoveConstraints::rollback(struct alter_space *alter) >> +{ >> + space_swap_constraint(alter->new_space, alter->old_space); >> +} >> @@ -2341,6 +2375,150 @@ index_is_used_by_fk_constraint(struct rlist = *fk_list, uint32_t iid) >> + >> +/** >> + * Put the node of an unique index to the constraint hash table of >> + * @a space and create trigger on rollback. >> + * >> + * This function is needed to wrap the duplicated piece of code >> + * inside on_replace_dd_index(). >> + * >> + * @param stmt Statement. >> + * @param space Space. >> + * @param def Index def. >> + * >> + * @retval 0 Success. >> + * @retval -1 Constraint already exists or out of memory. >=20 > 4. Once again - please, don't write comments just for > comments. 'stmt Statement.' really does not make anything > clearer or easier to understand. Either drop these > useless formal @param @retval, or provide more *useful* > details about these arguments. Although I doubt, that > it is needed. The function is trivial and is not public > anyway. The same about other similar places. >=20 > The comment's part before first @param is good, keep it. >=20 >> + */ >> +static int >> +create_index_as_constraint(struct txn_stmt *stmt, struct space = *space, >> + struct index_def *def) { >=20 > 5. I propose you to make it `const struct index_def *`. >=20 >> + assert(def->opts.is_unique); >> + if (space_constraint_def_by_name(space, def->name) !=3D NULL) { >> + diag_set(ClientError, ER_CONSTRAINT_EXISTS, def->name); +/** + * 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 =3D alter->old_space; + if (space_constraint_def_by_name(space, def->name) !=3D NULL) { + diag_set(ClientError, ER_CONSTRAINT_EXISTS, def->name); + return -1; + } + struct constraint_def *constr_def =3D + constraint_def_new(space->def->id, def->iid =3D=3D 0 ? + CONSTRAINT_TYPE_PK : = CONSTRAINT_TYPE_UNIQUE, + def->name); + if (constr_def =3D=3D NULL) + return -1; + (void) new CreateConstraintDef(alter, constr_def); + return 0; +} + > 6. AFAIR, I asked to add constraint type into the error message, = didn't I? > Why didn't you add it? I have done the commit to the patchset, but IMO, I jumped out of the = frying pan into the fire. Because of, e.g.: box.execute("CREATE TABLE t2(id INT PRIMARY KEY, CONSTRAINT fk1 FOREIGN = KEY(id) REFERENCES t2, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t2);") --- - null -- Constraint FK1 already exists +- Duplicate key exists in unique index 'primary' in space = =E2=80=98_fk_constraint' Tuple duplication is checked before the corresponding on_replace_dd_... >> + return -1; >> + } >> + struct constraint_def *constr_def =3D >> + constraint_def_new(space->def->id, def->iid =3D=3D 0 ? >> + CONSTRAINT_TYPE_PK : = CONSTRAINT_TYPE_UNIQUE, >> + def->name); >> + if (constr_def =3D=3D NULL) >> + return -1; >> + struct trigger *on_rollback =3D txn_alter_trigger_new(NULL, = NULL); >> + if (space_put_constraint(space, constr_def) !=3D 0 || >> + on_rollback =3D=3D NULL) { >> + constraint_def_free(constr_def); >> + return -1; >> + } >> + on_rollback->data =3D constr_def; >> + on_rollback->run =3D on_create_index_rollback; >> + txn_stmt_on_rollback(stmt, on_rollback); >> + return 0; >> +} >> + >> +/** >> + * Drop the node of an unique index from the constraint hash table >> + * of @a space and create trigger on rollback. >> + * >> + * This function is needed to wrap the duplicated piece of code >> + * inside on_replace_dd_index(). >> + * >> + * @param stmt Statement. >> + * @param space Space. >> + * @param def Index def. >> + * >> + * @retval 0 Success. >> + * @retval -1 Out of memory. >> + */ >> +static int >> +drop_index_as_constraint(struct txn_stmt *stmt, struct space *space, >> + struct index_def *def) { >> + assert(def->opts.is_unique); >> + struct constraint_def *constr_def =3D >> + space_constraint_def_by_name(space, def->name); >> + assert(constr_def !=3D NULL); >> + space_drop_constraint(space, def->name); >> + struct trigger *on_rollback =3D txn_alter_trigger_new(NULL, = NULL); >> + if (on_rollback =3D=3D NULL) >> + return -1; >=20 > 7. In case on_rollback creation is failed, you don't restore > the constraint back. >=20 >> + on_rollback->data =3D constr_def; >> + on_rollback->run =3D on_drop_index_rollback; >> + txn_stmt_on_rollback(stmt, on_rollback); >> + return 0; >> +} >> + >> /** >> * Just like with _space, 3 major cases: >> *> @@ -2469,35 +2645,111 @@ on_replace_dd_index(struct trigger * /* = trigger */, void *event) >> * Can't drop index if foreign key constraints >> * references this index. >> */ >> - if = (index_is_used_by_fk_constraint(&old_space->parent_fk_constraint, >> + if = (index_is_used_by_fk_constraint(&space->parent_fk_constraint, >> iid)) { >> - diag_set(ClientError, ER_ALTER_SPACE, >> - space_name(old_space), >> + diag_set(ClientError, ER_ALTER_SPACE, = space_name(space), >> "can not drop a referenced index"); >> return -1; >> } >> alter_space_move_indexes(alter, 0, iid); >> (void) new DropIndex(alter, old_index); >> + if (old_index->def->opts.is_unique && >> + drop_index_as_constraint(stmt, space, = old_index->def) !=3D 0) >> + return -1; >> } >> /* Case 2: create an index, if it is simply created. */ >> if (old_index =3D=3D NULL && new_tuple !=3D NULL) { >> + struct index_def *def =3D >> + index_def_new_from_tuple(new_tuple, space); >> + if (def =3D=3D NULL) >> + return -1; >> + if (def->opts.is_unique) { >> + if (create_index_as_constraint(stmt, space, def) = !=3D 0) { >> + index_def_delete(def); >> + return -1; >> + } >> + } >> alter_space_move_indexes(alter, 0, iid); >> CreateIndex *create_index =3D new CreateIndex(alter); >> - create_index->new_index_def =3D >> - index_def_new_from_tuple(new_tuple, old_space); >> - if (create_index->new_index_def =3D=3D NULL) >> - return -1; >> - = index_def_update_optionality(create_index->new_index_def, >> - = alter->new_min_field_count); >> + create_index->new_index_def =3D 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 !=3D NULL && new_tuple !=3D NULL) { >> struct index_def *index_def; >> - index_def =3D index_def_new_from_tuple(new_tuple, = old_space); >> + index_def =3D index_def_new_from_tuple(new_tuple, = space); >> if (index_def =3D=3D NULL) >> return -1; >> auto index_def_guard =3D >> make_scoped_guard([=3D] { = index_def_delete(index_def); }); >> + struct index_def *old_def =3D old_index->def; >> + /** >=20 > 8. We don't use /** inside functions. Ok. Removed. >> + * 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) { >> + if (create_index_as_constraint(stmt, space, = index_def) >=20 > 9. When you have multiple conditions, it is shorter > and easier to check them all via && instead of 'if if if ... =E2=80=98. Ok. I thought it was more obvious. >=20 > 13. I remember, you said that 'old_space' -> 'space' is going > to save some lines, but man. This rename diff is huge. This is > really comparable with the functional diff in this function. I > didn't think, that this rename will affect so much code. Please, > return old_space back. It is not worth introducing such a big > diff. Returned. >=20 > 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. >=20 > 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. >=20 > 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(). >=20 > 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) +{ + if (space_put_constraint(alter->old_space, new_constraint_def) = !=3D 0) + panic("Can't recover after constraint alter (out of = memory)"); +} + +void +CreateConstraintDef::rollback(struct alter_space *alter) +{ + /* + * Pop from new_space, because hash tables was swapped by + * MoveConstraintDefs::alter(). + */ + assert(space_pop_constraint(alter->new_space, + new_constraint_def->name) =3D=3D + new_constraint_def); + 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) +{ + old_constraint_def =3D + space_constraint_def_by_name(alter->old_space, name); + assert(old_constraint_def !=3D NULL); + space_pop_constraint(alter->old_space, name); +} + +void +DropConstraintDef::commit(struct alter_space *alter, int64_t signature) +{ + (void) alter; + (void) signature; + constraint_def_delete(old_constraint_def); +} + +void +DropConstraintDef::rollback(struct alter_space *alter) +{ + assert(old_constraint_def !=3D NULL); + /* + * Put to new_space, because hash tables was swapped by + * MoveConstraintDefs::alter(). + */ + if (space_put_constraint(alter->new_space, old_constraint_def) = !=3D 0) { + panic("Can't recover after constraint drop rollback (out = of " + "memory)"); + } +} + commit 5bad13b4756bff553d1107616aa3de3dd4bbe124 Author: Roman Khabibov Date: Wed Oct 23 15:54:16 2019 +0300 sql: make constraint operations transactional =20 Put constraint names into the space's hash table and drop them on replace in corresponding system spaces (_index, _fk_constraint, _ck_constraint). =20 Closes #3503 =20 @TarantoolBot document Title: Table constraints in SQL =20 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: =20 CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0), CONSTRAINT c PRIMARY KEY (i)); =20 or =20 CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY); ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);'); =20 The same is for any other constraint types. =20 Lua/box: =20 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: =20 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);'); =20 But, if you try to: =20 box.space.T2.index.E:alter({unique =3D true}); =20 You will get error, beacuse index 'e' is becoming unique. diff --git a/src/box/alter.cc b/src/box/alter.cc index 4a3241a79..9583bfd13 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" =20 /* {{{ Auxiliary functions and methods. */ =20 @@ -1765,6 +1766,113 @@ MoveCkConstraints::rollback(struct alter_space = *alter) space_swap_ck_constraint(alter->new_space, alter->old_space); } =20 +/** + * 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) +{ + if (space_put_constraint(alter->old_space, new_constraint_def) = !=3D 0) + panic("Can't recover after constraint alter (out of = memory)"); +} + +void +CreateConstraintDef::rollback(struct alter_space *alter) +{ + /* + * Pop from new_space, because hash tables was swapped by + * MoveConstraintDefs::alter(). + */ + assert(space_pop_constraint(alter->new_space, + new_constraint_def->name) =3D=3D + new_constraint_def); + 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) +{ + old_constraint_def =3D + space_constraint_def_by_name(alter->old_space, name); + assert(old_constraint_def !=3D NULL); + space_pop_constraint(alter->old_space, name); +} + +void +DropConstraintDef::commit(struct alter_space *alter, int64_t signature) +{ + (void) alter; + (void) signature; + constraint_def_delete(old_constraint_def); +} + +void +DropConstraintDef::rollback(struct alter_space *alter) +{ + assert(old_constraint_def !=3D NULL); + /* + * Put to new_space, because hash tables was swapped by + * MoveConstraintDefs::alter(). + */ + if (space_put_constraint(alter->new_space, old_constraint_def) = !=3D 0) { + panic("Can't recover after constraint drop rollback (out = of " + "memory)"); + } +} + +/** + * Move constraint names hash table from old space to a new one. + */ +class MoveConstraintDefs: public AlterSpaceOp +{ +public: + MoveConstraintDefs(struct alter_space *alter) : = AlterSpaceOp(alter) {} + virtual void alter(struct alter_space *alter); + virtual void rollback(struct alter_space *alter); +}; + +void +MoveConstraintDefs::alter(struct alter_space *alter) +{ + SWAP(alter->new_space->constraint_names, + alter->old_space->constraint_names); +} + +void +MoveConstraintDefs::rollback(struct alter_space *alter) +{ + SWAP(alter->new_space->constraint_names, + alter->old_space->constraint_names); +} + /* }}} */ =20 /** @@ -2307,6 +2415,7 @@ on_replace_dd_space(struct trigger * /* trigger = */, void *event) def_guard.is_active =3D false; /* Create MoveIndex ops for all space indexes. */ alter_space_move_indexes(alter, 0, = old_space->index_id_max + 1); + (void) new MoveConstraintDefs(alter); /* Remember to update schema_version. */ (void) new UpdateSchemaVersion(alter); try { @@ -2341,6 +2450,33 @@ index_is_used_by_fk_constraint(struct rlist = *fk_list, uint32_t iid) return false; } =20 +/** + * 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 =3D alter->old_space; + if (space_constraint_def_by_name(space, def->name) !=3D NULL) { + diag_set(ClientError, ER_CONSTRAINT_EXISTS, def->name); + return -1; + } + struct constraint_def *constr_def =3D + constraint_def_new(space->def->id, def->iid =3D=3D 0 ? + CONSTRAINT_TYPE_PK : = CONSTRAINT_TYPE_UNIQUE, + def->name); + if (constr_def =3D=3D NULL) + return -1; + (void) new CreateConstraintDef(alter, constr_def); + return 0; +} + /** * Just like with _space, 3 major cases: * @@ -2476,19 +2612,28 @@ on_replace_dd_index(struct trigger * /* trigger = */, void *event) "can not drop a referenced index"); return -1; } + struct index_def *def =3D old_index->def; alter_space_move_indexes(alter, 0, iid); (void) new DropIndex(alter, old_index); + if (old_index->def->opts.is_unique) + (void) new DropConstraintDef(alter, def->name); } /* Case 2: create an index, if it is simply created. */ if (old_index =3D=3D NULL && new_tuple !=3D NULL) { - alter_space_move_indexes(alter, 0, iid); - CreateIndex *create_index =3D new CreateIndex(alter); - create_index->new_index_def =3D + struct index_def *def =3D index_def_new_from_tuple(new_tuple, old_space); - if (create_index->new_index_def =3D=3D NULL) + if (def =3D=3D NULL) return -1; - = index_def_update_optionality(create_index->new_index_def, - = alter->new_min_field_count); + if (def->opts.is_unique) { + if (create_index_as_constraint(alter, def) !=3D = 0) { + index_def_delete(def); + return -1; + } + } + alter_space_move_indexes(alter, 0, iid); + CreateIndex *create_index =3D new CreateIndex(alter); + create_index->new_index_def =3D 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 !=3D NULL && new_tuple !=3D NULL) { @@ -2498,6 +2643,23 @@ on_replace_dd_index(struct trigger * /* trigger = */, void *event) return -1; auto index_def_guard =3D make_scoped_guard([=3D] { = index_def_delete(index_def); }); + struct index_def *old_def =3D 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) !=3D 0) + return -1; + if (old_def->opts.is_unique && index_def->opts.is_unique = && + strcmp(index_def->name, old_def->name) !=3D 0) { + if (create_index_as_constraint(alter, index_def) = !=3D 0) + return -1; + (void) new DropConstraintDef(alter, = old_def->name); + } + if (old_def->opts.is_unique && = !index_def->opts.is_unique) + (void) new DropConstraintDef(alter, = old_def->name); /* * To detect which key parts are optional, * min_field_count is required. But @@ -2572,6 +2734,7 @@ on_replace_dd_index(struct trigger * /* trigger = */, void *event) */ alter_space_move_indexes(alter, iid + 1, old_space->index_id_max = + 1); (void) new MoveCkConstraints(alter); + (void) new MoveConstraintDefs(alter); /* Add an op to update schema_version on commit. */ (void) new UpdateSchemaVersion(alter); try { @@ -2663,6 +2826,7 @@ on_replace_dd_truncate(struct trigger * /* trigger = */, void *event) } =20 (void) new MoveCkConstraints(alter); + (void) new MoveConstraintDefs(alter); try { alter_space_do(stmt, alter); } catch (Exception *e) { @@ -4951,8 +5115,15 @@ on_create_fk_constraint_rollback(struct trigger = *trigger, void *event) struct fk_constraint *fk =3D (struct fk_constraint = *)trigger->data; rlist_del_entry(fk, in_parent_space); rlist_del_entry(fk, in_child_space); + struct space *child =3D space_by_id(fk->def->child_id); + const char *name =3D fk->def->name; + struct constraint_def *constr_def =3D + space_constraint_def_by_name(child, name); + assert(constr_def !=3D NULL); + space_pop_constraint(child, name); + constraint_def_delete(constr_def); 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; } @@ -4986,11 +5157,24 @@ on_drop_fk_constraint_rollback(struct trigger = *trigger, void *event) struct space *child =3D space_by_id(old_fk->def->child_id); rlist_add_entry(&child->child_fk_constraint, old_fk, = in_child_space); rlist_add_entry(&parent->parent_fk_constraint, old_fk, = in_parent_space); + const char *name =3D old_fk->def->name; + struct constraint_def *constr_def =3D + constraint_def_new(child->def->id, CONSTRAINT_TYPE_FK, = name); + if (constr_def =3D=3D NULL) + goto panic; + if (space_put_constraint(child, constr_def) !=3D 0) { + constraint_def_delete(constr_def); + goto panic; + } fk_constraint_set_mask(old_fk, &child->fk_constraint_mask, FIELD_LINK_CHILD); fk_constraint_set_mask(old_fk, &parent->fk_constraint_mask, FIELD_LINK_PARENT); return 0; + +panic: + panic("Can't recover after FK constraint drop rollback (out of " + "memory)"); } =20 /** @@ -5165,15 +5349,31 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) fk->def =3D fk_def; fk->index_id =3D fk_index->def->iid; if (old_tuple =3D=3D NULL) { + if (space_constraint_def_by_name(child_space, + fk_def->name) + !=3D NULL) { + diag_set(ClientError, = ER_CONSTRAINT_EXISTS, + fk_def->name); + 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); + struct constraint_def *constr_def =3D + constraint_def_new(child_space->def->id, + CONSTRAINT_TYPE_FK, + fk_def->name); + if (constr_def =3D=3D NULL) + return -1; struct trigger *on_rollback =3D = txn_alter_trigger_new(on_create_fk_constraint_rollback, fk); - if (on_rollback =3D=3D NULL) + if (space_put_constraint(child_space, = constr_def) !=3D 0 + || on_rollback =3D=3D NULL) { + constraint_def_delete(constr_def); return -1; + } txn_stmt_on_rollback(stmt, on_rollback); fk_constraint_set_mask(fk, = &parent_space->fk_constraint_mask, @@ -5221,6 +5421,12 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) struct fk_constraint *old_fk=3D = fk_constraint_remove(&child_space->child_fk_constraint, fk_def->name); + const char *name =3D fk_def->name; + struct constraint_def *constr_def =3D + space_constraint_def_by_name(child_space, name); + assert(constr_def !=3D NULL); + space_pop_constraint(child_space, name); + constraint_def_delete(constr_def); struct trigger *on_commit =3D = txn_alter_trigger_new(on_drop_or_replace_fk_constraint_commit, old_fk); @@ -5297,9 +5503,14 @@ on_create_ck_constraint_rollback(struct trigger = *trigger, void * /* event */) assert(ck !=3D NULL); struct space *space =3D space_by_id(ck->def->space_id); assert(space !=3D NULL); - assert(space_ck_constraint_by_name(space, ck->def->name, - strlen(ck->def->name)) !=3D = NULL); + const char *name =3D ck->def->name; + assert(space_ck_constraint_by_name(space, name, strlen(name)) !=3D= NULL); space_remove_ck_constraint(space, ck); + struct constraint_def *constr_def =3D + space_constraint_def_by_name(space, name); + assert(constr_def !=3D NULL); + space_pop_constraint(space, name); + constraint_def_delete(constr_def); ck_constraint_delete(ck); if (trigger_run(&on_alter_space, space) !=3D 0) return -1; @@ -5324,13 +5535,23 @@ on_drop_ck_constraint_rollback(struct trigger = *trigger, void * /* event */) assert(ck !=3D NULL); struct space *space =3D space_by_id(ck->def->space_id); assert(space !=3D NULL); - assert(space_ck_constraint_by_name(space, ck->def->name, - strlen(ck->def->name)) =3D=3D = NULL); - if (space_add_ck_constraint(space, ck) !=3D 0) - panic("Can't recover after CK constraint drop = rollback"); + const char *name =3D ck->def->name; + assert(space_ck_constraint_by_name(space, name, strlen(name)) =3D=3D= NULL); + struct constraint_def *constr_def =3D + constraint_def_new(space->def->id, CONSTRAINT_TYPE_CK, = name); + if (constr_def =3D=3D NULL) + goto panic; + if (space_add_ck_constraint(space, ck) !=3D 0 || + space_put_constraint(space, constr_def) !=3D 0) { + constraint_def_delete(constr_def); + goto panic; + } if (trigger_run(&on_alter_space, space) !=3D 0) return -1; return 0; + +panic: + panic("Can't recover after CK constraint drop rollback"); } =20 /** Commit REPLACE check constraint. */ @@ -5443,11 +5664,27 @@ on_replace_dd_ck_constraint(struct trigger * /* = trigger*/, void *event) auto ck_guard =3D make_scoped_guard([=3D] { ck_constraint_delete(new_ck_constraint); }); - if (old_ck_constraint !=3D NULL) + if (old_ck_constraint !=3D NULL) { rlist_del_entry(old_ck_constraint, link); + } else if (space_constraint_def_by_name(space, name) !=3D = NULL) { + diag_set(ClientError, ER_CONSTRAINT_EXISTS, + name); + return -1; + } if (space_add_ck_constraint(space, new_ck_constraint) !=3D= 0) return -1; ck_guard.is_active =3D false; + if (old_ck_constraint =3D=3D NULL) { + struct constraint_def *constr_def =3D + constraint_def_new(space->def->id, + CONSTRAINT_TYPE_CK, = name); + if (constr_def =3D=3D NULL) + return -1; + if (space_put_constraint(space, constr_def) !=3D = 0) { + constraint_def_delete(constr_def); + return -1; + } + } if (old_tuple !=3D NULL) { on_rollback->data =3D old_ck_constraint; on_rollback->run =3D = on_replace_ck_constraint_rollback; @@ -5467,7 +5704,13 @@ on_replace_dd_ck_constraint(struct trigger * /* = trigger*/, void *event) return -1; struct ck_constraint *old_ck_constraint =3D space_ck_constraint_by_name(space, name, = name_len); + struct constraint_def *constr_def =3D + space_constraint_def_by_name(space, + = old_ck_constraint->def->name); + assert(constr_def !=3D NULL); assert(old_ck_constraint !=3D NULL); + space_pop_constraint(space, = old_ck_constraint->def->name); + constraint_def_delete(constr_def); space_remove_ck_constraint(space, old_ck_constraint); on_commit->data =3D old_ck_constraint; on_commit->run =3D on_drop_ck_constraint_commit; @@ -5567,6 +5810,7 @@ on_replace_dd_func_index(struct trigger *trigger, = void *event) alter_space_move_indexes(alter, index->def->iid + 1, space->index_id_max + 1); (void) new MoveCkConstraints(alter); + (void) new MoveConstraintDefs(alter); (void) new UpdateSchemaVersion(alter); try { alter_space_do(stmt, alter); 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 =3D require('test_run').new() + | --- + | ... +engine =3D test_run:get_cfg('engine') + | --- + | ... +box.execute('pragma sql_default_engine=3D\''..engine..'\'') + | --- + | - row_count: 0 + | ... +test_run:cmd("setopt delimiter ';'") + | --- + | - true + | ... + +-- +-- Check a constraint name for duplicate within a single +-- statement. +-- +box.execute('CREATE TABLE t1 (i INT PRIMARY KEY);'); + | --- + | - row_count: 1 + | ... +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 + | ... + +-- +-- Check a constraint name for duplicate using +-- 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. +-- +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(); + | --- + | ... + +-- +-- 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 =3D '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 =3D false}); + | --- + | ... +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);'); + | --- + | - row_count: 1 + | ... +box.space.T2.index.E:alter({unique =3D true}); + | --- + | - error: Constraint E already exists + | ... +box.space.T2.ck_constraint.E:drop(); + | --- + | ... +box.space.T2.index.E:alter({unique =3D 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 =3D 'D', unique =3D false}); + | --- + | ... +box.execute('CREATE UNIQUE INDEX e ON t2(i);'); + | --- + | - row_count: 1 + | ... 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 @@ -0,0 +1,84 @@ +test_run =3D require('test_run').new() +engine =3D test_run:get_cfg('engine') +box.execute('pragma sql_default_engine=3D\''..engine..'\'') +test_run:cmd("setopt delimiter ';'") + +-- +-- Check a constraint name for duplicate within a single +-- statement. +-- +box.execute('CREATE TABLE t1 (i INT PRIMARY KEY);'); +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))]]); + +-- +-- Check a constraint name for duplicate using +-- 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. +-- +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(); + +-- +-- 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 =3D '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 =3D false}); +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);'); +box.space.T2.index.E:alter({unique =3D true}); +box.space.T2.ck_constraint.E:drop(); +box.space.T2.index.E:alter({unique =3D 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 =3D 'D', unique =3D false}); +box.execute('CREATE UNIQUE INDEX e ON t2(i);');