[Tarantool-patches] [PATCH v2 2/3] sql: make constraint operations transactional
Roman Khabibov
roman.habibov at tarantool.org
Thu Dec 5 21:43:46 MSK 2019
Please, don’t see the previous answer.
>
> On Nov 30, 2019, at 04:03, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
>
> Thanks for the patch!
>
> 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.
>
> See 14 comments below.
>
>> Put constraint names into the space's hash table and drop them on
>> replace in correspondig system spaces (_index, _fk_constraint,
>
> 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.
>
> 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?
>
> 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’t 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.
>
> The comment's part before first @param is good, keep it.
>
>> + */
>> +static int
>> +create_index_as_constraint(struct txn_stmt *stmt, struct space *space,
>> + struct index_def *def) {
>
> 5. I propose you to make it `const struct index_def *`.
>
>> + assert(def->opts.is_unique);
>> + if (space_constraint_def_by_name(space, def->name) != 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 = alter->old_space;
+ if (space_constraint_def_by_name(space, def->name) != NULL) {
+ diag_set(ClientError, ER_CONSTRAINT_EXISTS, def->name);
+ 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;
+}
+
>
> 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 ‘_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 ... ‘.
Ok. I thought it was more obvious.
>
> 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.
>
> 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)");
+}
+
+void
+CreateConstraintDef::rollback(struct alter_space *alter)
+{
+ assert(space_pop_constraint(alter->new_space,
+ new_constraint_def->name) ==
+ 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)");
+ }
+}
+
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 = 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 at 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.
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"
/* {{{ 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->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);
}
+/**
+ * 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)");
+}
+
+void
+CreateConstraintDef::rollback(struct alter_space *alter)
+{
+ assert(space_pop_constraint(alter->new_space,
+ new_constraint_def->name) ==
+ 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 +2448,57 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
return false;
}
+/**
+ * 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 (space_constraint_def_by_name(space, def->name) != NULL) {
+ diag_set(ClientError, ER_CONSTRAINT_EXISTS, def->name);
+ 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 +2636,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 +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;
}
- 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
+ * 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
@@ -4996,8 +5157,15 @@ 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);
+ const char *name = fk->def->name;
+ struct constraint_def *constr_def =
+ space_constraint_def_by_name(child, name);
+ assert(constr_def != 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 = 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 = old_fk->def->name;
+ struct constraint_def *constr_def =
+ constraint_def_new(child->def->id, CONSTRAINT_TYPE_FK, name);
+ if (constr_def == NULL)
+ goto panic;
+ if (space_put_constraint(child, constr_def) != 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)");
}
/**
@@ -5210,6 +5391,13 @@ 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) {
+ if (space_constraint_def_by_name(child_space,
+ fk_def->name)
+ != 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 == 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;
+ }
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 == NULL)
return -1;
+ const char *name = fk_def->name;
+ struct constraint_def *constr_def =
+ space_constraint_def_by_name(child_space, name);
+ assert(constr_def != 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 != 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);
+ struct constraint_def *constr_def =
+ space_constraint_def_by_name(space, name);
+ assert(constr_def != NULL);
+ space_pop_constraint(space, name);
+ constraint_def_delete(constr_def);
ck_constraint_delete(ck);
if (trigger_run(&on_alter_space, space) != 0)
return -1;
@@ -5371,13 +5580,23 @@ 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)
- panic("Can't recover after CK constraint drop rollback");
+ const char *name = ck->def->name;
+ assert(space_ck_constraint_by_name(space, name, strlen(name)) == NULL);
+ struct constraint_def *constr_def =
+ constraint_def_new(space->def->id, CONSTRAINT_TYPE_CK, name);
+ if (constr_def == NULL)
+ goto panic;
+ if (space_add_ck_constraint(space, ck) != 0 ||
+ space_put_constraint(space, constr_def) != 0) {
+ constraint_def_delete(constr_def);
+ goto panic;
+ }
if (trigger_run(&on_alter_space, space) != 0)
return -1;
return 0;
+
+panic:
+ panic("Can't recover after CK constraint drop rollback");
}
/** Commit REPLACE check constraint. */
@@ -5491,11 +5710,27 @@ 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)
+ if (old_ck_constraint != NULL) {
rlist_del_entry(old_ck_constraint, link);
+ } else if (space_constraint_def_by_name(space, name) != NULL) {
+ diag_set(ClientError, ER_CONSTRAINT_EXISTS,
+ name);
+ return -1;
+ }
if (space_add_ck_constraint(space, new_ck_constraint) != 0)
return -1;
ck_guard.is_active = false;
+ if (old_ck_constraint == NULL) {
+ struct constraint_def *constr_def =
+ constraint_def_new(space->def->id,
+ CONSTRAINT_TYPE_CK, name);
+ if (constr_def == NULL)
+ return -1;
+ if (space_put_constraint(space, constr_def) != 0) {
+ constraint_def_delete(constr_def);
+ return -1;
+ }
+ }
if (old_tuple != NULL) {
on_rollback->data = old_ck_constraint;
on_rollback->run = 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 =
space_ck_constraint_by_name(space, name, name_len);
+ struct constraint_def *constr_def =
+ space_constraint_def_by_name(space,
+ old_ck_constraint->def->name);
+ assert(constr_def != NULL);
assert(old_ck_constraint != 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 = 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..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 ';'")
+ | ---
+ | - true
+ | ...
+
+--
+-- Check a constraint name for duplicate within a single
+-- <CREATE TABLE> 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 <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.
+--
+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 = '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
+ | ...
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 = require('test_run').new()
+engine = test_run:get_cfg('engine')
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+test_run:cmd("setopt delimiter ';'")
+
+--
+-- Check a constraint name for duplicate within a single
+-- <CREATE TABLE> 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 <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.
+--
+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 = '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);');
More information about the Tarantool-patches
mailing list