From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 D337F46971A for ; Thu, 5 Dec 2019 21:43:48 +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: Thu, 5 Dec 2019 21:43:46 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: 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 Please, don=E2=80=99t see the previous answer. >=20 > 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. > 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. > 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()? @@ -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->constraint_names, + alter->old_space->constraint_names); 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->constraint_names, + alter->old_space->constraint_names); /* * The new space is ready. Time to update the space * cache with it. > 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; + 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; +} + >=20 > 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_ > 8. We don't use /** inside functions. Removed. > 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) +{ + assert(new_constraint_def !=3D NULL); + 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) +{ + assert(space_pop_constraint(alter->new_space, + new_constraint_def->name) =3D=3D + 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 !=3D NULL); + 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; + assert(old_constraint_def !=3D NULL); + constraint_def_delete(old_constraint_def); +} + +void +DropConstraintDef::rollback(struct alter_space *alter) +{ + assert(old_constraint_def !=3D NULL); + if (space_put_constraint(alter->new_space, old_constraint_def) = !=3D 0) { + panic("Can't recover after constraint drop rollback (out = of " + "memory)"); + } +} + I know, that I can combine these ifs to the one, but I did so, because I = believe that it is more readable. + 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 && + (create_index_as_constraint(alter, index_def) !=3D 0 = || + drop_index_as_constraint(alter, old_def) !=3D 0)) + return -1; + if (old_def->opts.is_unique && = !index_def->opts.is_unique && + drop_index_as_constraint(alter, old_def) !=3D 0) + return -1; commit c54d2e89d660d2f0b07f1e6917327131362a6568 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 bef25b605..c03bad589 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 @@ -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->constraint_names, + alter->old_space->constraint_names); 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->constraint_names, + alter->old_space->constraint_names); /* * The new space is ready. Time to update the space * cache with it. @@ -1764,6 +1771,84 @@ 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) +{ + assert(new_constraint_def !=3D NULL); + 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) +{ + assert(space_pop_constraint(alter->new_space, + new_constraint_def->name) =3D=3D + 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 !=3D NULL); + 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; + assert(old_constraint_def !=3D NULL); + constraint_def_delete(old_constraint_def); +} + +void +DropConstraintDef::rollback(struct alter_space *alter) +{ + assert(old_constraint_def !=3D NULL); + if (space_put_constraint(alter->new_space, old_constraint_def) = !=3D 0) { + panic("Can't recover after constraint drop rollback (out = of " + "memory)"); + } +} + /* }}} */ =20 /** @@ -2363,6 +2448,57 @@ 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; + 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 +2636,9 @@ on_replace_dd_index(struct trigger * /* trigger = */, void *event) } if (alter_space_move_indexes(alter, 0, iid) !=3D 0) return -1; + if (old_index->def->opts.is_unique && + drop_index_as_constraint(alter, old_index->def) !=3D = 0) + return -1; try { (void) new DropIndex(alter, old_index); } catch (Exception *e) { @@ -2510,18 +2649,23 @@ on_replace_dd_index(struct trigger * /* trigger = */, void *event) if (old_index =3D=3D NULL && new_tuple !=3D NULL) { if (alter_space_move_indexes(alter, 0, iid)) return -1; + struct index_def *def =3D + index_def_new_from_tuple(new_tuple, old_space); + if (def =3D=3D NULL) + return -1; + if (def->opts.is_unique && + create_index_as_constraint(alter, def) !=3D 0) { + index_def_delete(def); + return -1; + } CreateIndex *create_index; try { create_index =3D new CreateIndex(alter); } catch (Exception *e) { return -1; } - 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) { @@ -2531,6 +2675,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 && + (create_index_as_constraint(alter, index_def) !=3D 0 = || + drop_index_as_constraint(alter, old_def) !=3D 0)) + return -1; + if (old_def->opts.is_unique && = !index_def->opts.is_unique && + drop_index_as_constraint(alter, old_def) !=3D 0) + return -1; /* * To detect which key parts are optional, * min_field_count is required. But @@ -4996,8 +5157,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; } @@ -5031,11 +5199,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 /** @@ -5210,6 +5391,13 @@ 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, @@ -5219,6 +5407,16 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) fk); if (on_rollback =3D=3D NULL) return -1; + 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; + if (space_put_constraint(child_space, = constr_def) !=3D 0) { + constraint_def_delete(constr_def); + return -1; + } txn_stmt_on_rollback(stmt, on_rollback); fk_constraint_set_mask(fk, = &parent_space->fk_constraint_mask, @@ -5279,6 +5477,12 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) old_fk); if (on_rollback =3D=3D NULL) return -1; + 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); txn_stmt_on_rollback(stmt, on_rollback); space_reset_fk_constraint_mask(child_space); space_reset_fk_constraint_mask(parent_space); @@ -5344,9 +5548,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; @@ -5371,13 +5580,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. */ @@ -5491,11 +5710,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; @@ -5515,7 +5750,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; 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);');