[Tarantool-patches] [PATCH v2 2/3] box: make constraint operations transactional

Roman Khabibov roman.habibov at tarantool.org
Tue Dec 10 15:49:00 MSK 2019



> On Dec 7, 2019, at 19:35, Vladislav Shpilevoy <v.shpilevoy at tarantool.org> wrote:
> 
> Thanks for the fixes!
> 
> See 9 comments below.
> 
>>> 14. Talking of the super mess with on_replace_dd_index, and
>>> how tight it is now with all these struggling about replacing
>>> constraints, and on_commit/on_rollback. I think, that this is
>>> a dead end. Too complex.
>>> 
>>> How about introducing a couple of new AlterSpaceOp classes?
>>> AddConstraint, and DropConstraint. AlterSpaceOp looks good
>>> because
>>> - it will allow you to not set on_commit/on_rollback triggers
>>> manually - it already provides these methods;
>>> - you will be able to move this huge pieces of code out of
>>> on_replace_dd_index and other on_replace_*;
>>> - it is consistent with MoveIndex/DropIndex/etc classes.
>>> 
>>> All you will need to do is to add to on_replace_*
>>> (void) new AddConstraint(), and (void) new DropConstraint.
>>> And you will keep (void) new MoveConstraints().
>>> 
>>> Also that should eliminate code duplication between different
>>> on_replace_* functions about adding/dropping constraints.
>> +/**
>> + * CreateConstraintDef - add a new constraint def to the space.
>> + */
>> +class CreateConstraintDef: public AlterSpaceOp
>> +{
>> +private:
>> +	struct constraint_def *new_constraint_def;
>> +public:
>> +	CreateConstraintDef(struct alter_space *alter,
>> +			    struct constraint_def *def)
>> +		:AlterSpaceOp(alter), new_constraint_def(def)
>> +	{}
>> +	virtual void alter(struct alter_space *alter);
>> +	virtual void rollback(struct alter_space *alter);
>> +};
>> +
>> +void
>> +CreateConstraintDef::alter(struct alter_space *alter)
>> +{
>> +	assert(new_constraint_def != NULL);
>> +	if (space_put_constraint(alter->old_space, new_constraint_def) != 0)
>> +		panic("Can't recover after constraint alter (out of memory)");
> 
> 1. Wow, why panic? Alter can safely fail. It it is not a commit or
> rollback trigger. Maybe it can't return an error, but it can throw
> it.
> 
> (Although I would like to be able to panic on malloc fails, but it
> does not seem to happen in the nearest future.)
+void
+CreateConstraintDef::alter(struct alter_space *alter)
+{
+	assert(new_constraint_def != NULL);
+	if (space_put_constraint(alter->old_space, new_constraint_def) != 0)
+		diag_raise();
+}

>> +}
>> +
>> +void
>> +CreateConstraintDef::rollback(struct alter_space *alter)
>> +{
>> +	assert(space_pop_constraint(alter->new_space,
>> +				    new_constraint_def->name) ==
>> +	       new_constraint_def);
> 
> 2. This will fail in Release build mode, because
> assert arguments are not calculated in Release. So you
> delete the object, but it is still in the hash table.
+void
+CreateConstraintDef::rollback(struct alter_space *alter)
+{
+	struct constraint_def *constraint_def =
+		space_pop_constraint(alter->new_space,
+				     new_constraint_def->name);
+	assert(constraint_def == new_constraint_def);
+	(void) alter;
+	constraint_def_delete(new_constraint_def);
+}

>> +	(void) alter;
>> +	constraint_def_delete(new_constraint_def);
>> +}
>> +
>> 
>> I know, that I can combine these ifs to the one, but I did so, because I believe that
>> it is more readable.
> 
> 3. Yes, here I agree.
> 
>> 
>> +		struct index_def *old_def = old_index->def;
>> +		/*
>> +		 * We put a new name either an index is becoming
>> +		 * unique (i.e. constraint), or when an unique
>> +		 * index's name is under change.
>> +		 */
>> +		if (!old_def->opts.is_unique && index_def->opts.is_unique &&
>> +		    create_index_as_constraint(alter, index_def) != 0)
>> +			return -1;
>> +		if (old_def->opts.is_unique && index_def->opts.is_unique &&
>> +		    strcmp(index_def->name, old_def->name) != 0 &&
>> +		    (create_index_as_constraint(alter, index_def) != 0 ||
>> +		     drop_index_as_constraint(alter, old_def) != 0))
>> +			return -1;
>> +		if (old_def->opts.is_unique && !index_def->opts.is_unique &&
>> +		    drop_index_as_constraint(alter, old_def) != 0)
>> +			return -1;
>> 
>> 
>> commit c54d2e89d660d2f0b07f1e6917327131362a6568
>> Author: Roman Khabibov <roman.habibov 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.
> 
> 4. That is a good comment. (But typo: beacuse -> because.)
Done.
>> 
>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index bef25b605..c03bad589 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc> @@ -2510,18 +2649,23 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
>> 	if (old_index == NULL && new_tuple != NULL) {
>> 		if (alter_space_move_indexes(alter, 0, iid))
>> 			return -1;
>> +		struct index_def *def =
>> +			index_def_new_from_tuple(new_tuple, old_space);
>> +		if (def == NULL)
>> +			return -1;
>> +		if (def->opts.is_unique &&
>> +		    create_index_as_constraint(alter, def) != 0) {
>> +			index_def_delete(def);
>> +			return -1;
>> +		}
>> 		CreateIndex *create_index;
>> 		try {
>> 			create_index = new CreateIndex(alter);
>> 		} catch (Exception *e) {
>> 			return -1;
> 
> 5. 'def' leaks here.
@ -2510,18 +2669,24 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 	if (old_index == NULL && new_tuple != NULL) {
 		if (alter_space_move_indexes(alter, 0, iid))
 			return -1;
+		struct index_def *def =
+			index_def_new_from_tuple(new_tuple, old_space);
+		if (def == NULL)
+			return -1;
+		if (def->opts.is_unique &&
+		    create_index_as_constraint(alter, def) != 0) {
+			index_def_delete(def);
+			return -1;
+		}
 		CreateIndex *create_index;
 		try {
 			create_index = new CreateIndex(alter);
 		} catch (Exception *e) {
+			index_def_delete(def);
 			return -1;
 		}

>> 		}
>> -		create_index->new_index_def =
>> -			index_def_new_from_tuple(new_tuple, old_space);
>> -		if (create_index->new_index_def == NULL)
>> -			return -1;
>> -		index_def_update_optionality(create_index->new_index_def,
>> -					     alter->new_min_field_count);
>> +		create_index->new_index_def = def;
>> +		index_def_update_optionality(def, alter->new_min_field_count);
>> 	}
>> 	/* Case 3 and 4: check if we need to rebuild index data. */
>> 	if (old_index != NULL && new_tuple != NULL) {
>> @@ -2531,6 +2675,23 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
>> 			return -1;
>> 		auto index_def_guard =
>> 			make_scoped_guard([=] { index_def_delete(index_def); });
>> +		struct index_def *old_def = old_index->def;
>> +		/*
>> +		 * We put a new name either an index is becoming
> 
> 6. 'either' -> 'when either’.
+		/*
+		 * We put a new name when either an index is
+		 * becoming unique (i.e. constraint), or when an
+		 * unique index's name is under change.
+		 */

>> +		 * unique (i.e. constraint), or when an unique
>> +		 * index's name is under change.
>> +		 */
>> +		if (!old_def->opts.is_unique && index_def->opts.is_unique &&
>> +		    create_index_as_constraint(alter, index_def) != 0)
>> +			return -1;
>> +		if (old_def->opts.is_unique && index_def->opts.is_unique &&
>> +		    strcmp(index_def->name, old_def->name) != 0 &&
>> +		    (create_index_as_constraint(alter, index_def) != 0 ||
>> +		     drop_index_as_constraint(alter, old_def) != 0))
>> +			return -1;
>> +		if (old_def->opts.is_unique && !index_def->opts.is_unique &&
>> +		    drop_index_as_constraint(alter, old_def) != 0)
>> +			return -1;
>> 		/*
>> 		 * To detect which key parts are optional,
>> 		 * min_field_count is required. But> @@ -5219,6 +5407,16 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
>> 						      fk);
>> 			if (on_rollback == NULL)
>> 				return -1;
>> +			struct constraint_def *constr_def =
>> +				constraint_def_new(child_space->def->id,
>> +						   CONSTRAINT_TYPE_FK,
>> +						   fk_def->name);
>> +			if (constr_def == NULL)
>> +				return -1;
>> +			if (space_put_constraint(child_space, constr_def) != 0) {
>> +				constraint_def_delete(constr_def);
>> +				return -1;
>> +			}
> 
> 7. Sequence constraint_def_new -> space_put_constraint is repeated 4
> times. I think this is worth wrapping into a function. The same about
> space_constraint_def_by_name -> space_pop_constraint ->
> constraint_def_delete.
+/**
+ * Put the node of a ck/fk constraint to the constraint hash table
+ * of @a space.
+ *
+ * This function is needed to wrap the duplicated piece of code
+ * inside the trigger functions below.
+ */
+static int
+create_fk_ck_as_constraint(struct space *space, enum constraint_type type,
+			   const char *name)
+{
+	assert(type == CONSTRAINT_TYPE_CK || type == CONSTRAINT_TYPE_FK);
+	struct constraint_def *constraint_def =
+		constraint_def_new(space->def->id, type, name);
+	if (constraint_def == NULL)
+		return -1;
+	if (space_put_constraint(space, constraint_def) != 0) {
+		constraint_def_delete(constraint_def);
+		return -1;
+	}
+	return 0;
+}
+
+/**
+ * Drop the node of a ck/fk constraint from the constraint hash
+ * table of @a space.
+ *
+ * This function is needed to wrap the duplicated piece of code
+ * inside the trigger functions below.
+ */
+static void
+drop_fk_ck_as_constraint(struct space *space, const char *name)
+{
+	struct constraint_def *constraint_def =
+		space_constraint_def_by_name(space, name);
+	assert(constraint_def != NULL);
+	assert(constraint_def->type == CONSTRAINT_TYPE_CK ||
+	       constraint_def->type == CONSTRAINT_TYPE_FK);
+	space_pop_constraint(space, name);
+	constraint_def_delete(constraint_def);
+}
+

>> 			txn_stmt_on_rollback(stmt, on_rollback);
>> 			fk_constraint_set_mask(fk,
>> 					       &parent_space->fk_constraint_mask,
>> diff --git a/test/sql/constraint.result b/test/sql/constraint.result
>> new file mode 100644
>> index 000000000..ba262182b
>> --- /dev/null
>> +++ b/test/sql/constraint.result
>> @@ -0,0 +1,190 @@
>> +-- test-run result file version 2
>> +test_run = require('test_run').new()
>> + | ---
>> + | ...
>> +engine = test_run:get_cfg('engine')
>> + | ---
>> + | ...
>> +box.execute('pragma sql_default_engine=\''..engine..'\'')
>> + | ---
>> + | - row_count: 0
>> + | ...
>> +test_run:cmd("setopt delimiter ';'")
> 
> 8. Please, don't. There is only one small place where you need
> multiple lines. You can either use ';' just for it, or use /.
> 
>> diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
>> new file mode 100755
>> index 000000000..50162e47d
>> --- /dev/null
>> +++ b/test/sql/constraint.test.lua
> 
> 9. Your test leaves lots of not deleted data. Please, do
> cleanup.
Done.

commit c90d7935b56c4603303bac45a7ebeb9af03eef3d
Author: Roman Khabibov <roman.habibov at tarantool.org>
Date:   Wed Oct 23 15:54:16 2019 +0300

    box: make constraint operations transactional
    
    Put constraint names into the space's hash table and drop them on
    replace in corresponding system spaces (_index, _fk_constraint,
    _ck_constraint).
    
    Closes #3503
    
    @TarantoolBot document
    Title: Table constraints in SQL
    
    SQL:
    According to ANSI SQL, table constraint is one of the following
    entities: PRIMARY KEY, UNIQUE, FOREIGN KEY, CHECK. Every
    constraint have its own name passed by user or automatically
    generated. And these names must be unique within one table/space.
    Naming in SQL is case-insensitive, so "CONSTRAINT c" and
    "CONSTRAINT C" are the same. For example, you will get error, if
    you try to:
    
    CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0),
                     CONSTRAINT c PRIMARY KEY (i));
    
    or
    
    CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY);
    ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);');
    
    The same is for any other constraint types.
    
    Lua/box:
    
    You also can create/drop table constraints from box. See
    space_object:create_check_constraint() and space_object:create_index()
    (if an index is unique). Naming in box is case-sensitive, so 'c' and
    'C' are not the same (see naming policy). For example, an unique
    index is a constraint, but a non-unique index is not. So, a non-unique
    index can have the same name with a check/foreign key constraint
    within one space:
    
    box.execute('CREATE TABLE t2 (i INT PRIMARY KEY);');
    box.execute('CREATE INDEX e ON t2(i);');
    box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);');
    
    But, if you try to:
    
    box.space.T2.index.E:alter({unique = true});
    
    You will get error, because index 'e' is becoming unique.

diff --git a/src/box/alter.cc b/src/box/alter.cc
index bef25b605..d5e836ae7 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -57,6 +57,7 @@
 #include "version.h"
 #include "sequence.h"
 #include "sql.h"
+#include "constraint_def.h"
 
 /* {{{ Auxiliary functions and methods. */
 
@@ -1033,10 +1034,13 @@ alter_space_rollback(struct trigger *trigger, void * /* event */)
 	space_fill_index_map(alter->old_space);
 	space_fill_index_map(alter->new_space);
 	/*
-	 * Don't forget about space triggers and foreign keys.
+	 * Don't forget about space triggers, foreign keys and
+	 * constraints.
 	 */
 	space_swap_triggers(alter->new_space, alter->old_space);
 	space_swap_fk_constraints(alter->new_space, alter->old_space);
+	SWAP(alter->new_space->constraints,
+	     alter->old_space->constraints);
 	space_cache_replace(alter->new_space, alter->old_space);
 	alter_space_delete(alter);
 	return 0;
@@ -1143,10 +1147,13 @@ alter_space_do(struct txn_stmt *stmt, struct alter_space *alter)
 	space_fill_index_map(alter->old_space);
 	space_fill_index_map(alter->new_space);
 	/*
-	 * Don't forget about space triggers and foreign keys.
+	 * Don't forget about space triggers, foreign keys and
+	 * constraints.
 	 */
 	space_swap_triggers(alter->new_space, alter->old_space);
 	space_swap_fk_constraints(alter->new_space, alter->old_space);
+	SWAP(alter->new_space->constraints,
+	     alter->old_space->constraints);
 	/*
 	 * The new space is ready. Time to update the space
 	 * cache with it.
@@ -1764,6 +1771,85 @@ MoveCkConstraints::rollback(struct alter_space *alter)
 	space_swap_ck_constraint(alter->new_space, alter->old_space);
 }
 
+/**
+ * CreateConstraintDef - add a new constraint def to the space.
+ */
+class CreateConstraintDef: public AlterSpaceOp
+{
+private:
+	struct constraint_def *new_constraint_def;
+public:
+	CreateConstraintDef(struct alter_space *alter,
+			    struct constraint_def *def)
+		:AlterSpaceOp(alter), new_constraint_def(def)
+	{}
+	virtual void alter(struct alter_space *alter);
+	virtual void rollback(struct alter_space *alter);
+};
+
+void
+CreateConstraintDef::alter(struct alter_space *alter)
+{
+	assert(new_constraint_def != NULL);
+	if (space_put_constraint(alter->old_space, new_constraint_def) != 0)
+		diag_raise();
+}
+
+void
+CreateConstraintDef::rollback(struct alter_space *alter)
+{
+	struct constraint_def *constraint_def =
+		space_pop_constraint(alter->new_space,
+				     new_constraint_def->name);
+	assert(constraint_def == new_constraint_def);
+	(void) alter;
+	constraint_def_delete(new_constraint_def);
+}
+
+/** DropConstraintDef - drop a constraint def from the space. */
+class DropConstraintDef: public AlterSpaceOp
+{
+private:
+	struct constraint_def *old_constraint_def;
+	const char *name;
+public:
+	DropConstraintDef(struct alter_space *alter, const char *name)
+		:AlterSpaceOp(alter), old_constraint_def(NULL), name(name)
+	{}
+	virtual void alter(struct alter_space *alter);
+	virtual void commit(struct alter_space *alter , int64_t signature);
+	virtual void rollback(struct alter_space *alter);
+};
+
+void
+DropConstraintDef::alter(struct alter_space *alter)
+{
+	assert(name != NULL);
+	old_constraint_def =
+		space_constraint_def_by_name(alter->old_space, name);
+	assert(old_constraint_def != NULL);
+	space_pop_constraint(alter->old_space, name);
+}
+
+void
+DropConstraintDef::commit(struct alter_space *alter, int64_t signature)
+{
+	(void) alter;
+	(void) signature;
+	assert(old_constraint_def != NULL);
+	constraint_def_delete(old_constraint_def);
+}
+
+void
+DropConstraintDef::rollback(struct alter_space *alter)
+{
+	assert(old_constraint_def != NULL);
+	if (space_put_constraint(alter->new_space, old_constraint_def) != 0) {
+		panic("Can't recover after constraint drop rollback (out of "
+		      "memory)");
+	}
+}
+
 /* }}} */
 
 /**
@@ -2363,6 +2449,76 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
 	return false;
 }
 
+/**
+ * Check if constraint with @a name exists within @a space. Call
+ * diag_set() if it is so.
+ *
+ * @param space Space to find constraint within.
+ * @param name  Constraint name.
+ *
+ * @retval -1 Constraint exists.
+ * @retval 0  Doesn't exist.
+ */
+static inline int
+check_existence(struct space *space, const char *name)
+{
+	struct constraint_def *def = space_constraint_def_by_name(space, name);
+	if (def != NULL) {
+		diag_set(ClientError, ER_CONSTRAINT_EXISTS, name);
+		return -1;
+	}
+	return 0;
+}
+
+/**
+ * Put the node of an unique index to the constraint hash table of
+ * @a space.
+ *
+ * This function is needed to wrap the duplicated piece of code
+ * inside on_replace_dd_index().
+ */
+static int
+create_index_as_constraint(struct alter_space *alter,
+			   const struct index_def *def)
+{
+	assert(def->opts.is_unique);
+	struct space *space = alter->old_space;
+	if (check_existence(space, def->name) != 0)
+		return -1;
+	struct constraint_def *constr_def =
+		constraint_def_new(space->def->id, def->iid == 0 ?
+				   CONSTRAINT_TYPE_PK : CONSTRAINT_TYPE_UNIQUE,
+				   def->name);
+	if (constr_def == NULL)
+		return -1;
+	try {
+		(void) new CreateConstraintDef(alter, constr_def);
+	} catch (Exception *e) {
+		constraint_def_delete(constr_def);
+		return -1;
+	}
+	return 0;
+}
+
+/**
+ * Drop the node of an unique index from the constraint hash table
+ * of @a space.
+ *
+ * This function is needed to wrap the duplicated piece of code
+ * inside on_replace_dd_index().
+ */
+static int
+drop_index_as_constraint(struct alter_space *alter, const struct index_def *def)
+{
+	assert(def->opts.is_unique);
+	try {
+		(void) new DropConstraintDef(alter, def->name);
+	} catch (Exception *e) {
+		return -1;
+	}
+	return 0;
+}
+
 /**
  * Just like with _space, 3 major cases:
  *
@@ -2500,6 +2656,9 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 		}
 		if (alter_space_move_indexes(alter, 0, iid) != 0)
 			return -1;
+		if (old_index->def->opts.is_unique &&
+		    drop_index_as_constraint(alter, old_index->def) != 0)
+			return -1;
 		try {
 			(void) new DropIndex(alter, old_index);
 		} catch (Exception *e) {
@@ -2510,18 +2669,24 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 	if (old_index == NULL && new_tuple != NULL) {
 		if (alter_space_move_indexes(alter, 0, iid))
 			return -1;
+		struct index_def *def =
+			index_def_new_from_tuple(new_tuple, old_space);
+		if (def == NULL)
+			return -1;
+		if (def->opts.is_unique &&
+		    create_index_as_constraint(alter, def) != 0) {
+			index_def_delete(def);
+			return -1;
+		}
 		CreateIndex *create_index;
 		try {
 			create_index = new CreateIndex(alter);
 		} catch (Exception *e) {
+			index_def_delete(def);
 			return -1;
 		}
-		create_index->new_index_def =
-			index_def_new_from_tuple(new_tuple, old_space);
-		if (create_index->new_index_def == NULL)
-			return -1;
-		index_def_update_optionality(create_index->new_index_def,
-					     alter->new_min_field_count);
+		create_index->new_index_def = def;
+		index_def_update_optionality(def, alter->new_min_field_count);
 	}
 	/* Case 3 and 4: check if we need to rebuild index data. */
 	if (old_index != NULL && new_tuple != NULL) {
@@ -2531,6 +2696,23 @@ on_replace_dd_index(struct trigger * /* trigger */, void *event)
 			return -1;
 		auto index_def_guard =
 			make_scoped_guard([=] { index_def_delete(index_def); });
+		struct index_def *old_def = old_index->def;
+		/*
+		 * We put a new name when either an index is
+		 * becoming unique (i.e. constraint), or when an
+		 * unique index's name is under change.
+		 */
+		if (!old_def->opts.is_unique && index_def->opts.is_unique &&
+		    create_index_as_constraint(alter, index_def) != 0)
+			return -1;
+		if (old_def->opts.is_unique && index_def->opts.is_unique &&
+		    strcmp(index_def->name, old_def->name) != 0 &&
+		    (create_index_as_constraint(alter, index_def) != 0 ||
+		     drop_index_as_constraint(alter, old_def) != 0))
+			return -1;
+		if (old_def->opts.is_unique && !index_def->opts.is_unique &&
+		    drop_index_as_constraint(alter, old_def) != 0)
+			return -1;
 		/*
 		 * To detect which key parts are optional,
 		 * min_field_count is required. But
@@ -4984,6 +5166,48 @@ space_reset_fk_constraint_mask(struct space *space)
 	}
 }
 
+/**
+ * Put the node of a ck/fk constraint to the constraint hash table
+ * of @a space.
+ *
+ * This function is needed to wrap the duplicated piece of code
+ * inside the trigger functions below.
+ */
+static int
+create_fk_ck_as_constraint(struct space *space, enum constraint_type type,
+			   const char *name)
+{
+	assert(type == CONSTRAINT_TYPE_CK || type == CONSTRAINT_TYPE_FK);
+	struct constraint_def *constraint_def =
+		constraint_def_new(space->def->id, type, name);
+	if (constraint_def == NULL)
+		return -1;
+	if (space_put_constraint(space, constraint_def) != 0) {
+		constraint_def_delete(constraint_def);
+		return -1;
+	}
+	return 0;
+}
+
+/**
+ * Drop the node of a ck/fk constraint from the constraint hash
+ * table of @a space.
+ *
+ * This function is needed to wrap the duplicated piece of code
+ * inside the trigger functions below.
+ */
+static void
+drop_fk_ck_as_constraint(struct space *space, const char *name)
+{
+	struct constraint_def *constraint_def =
+		space_constraint_def_by_name(space, name);
+	assert(constraint_def != NULL);
+	assert(constraint_def->type == CONSTRAINT_TYPE_CK ||
+	       constraint_def->type == CONSTRAINT_TYPE_FK);
+	space_pop_constraint(space, name);
+	constraint_def_delete(constraint_def);
+}
+
 /**
  * On rollback of creation we remove FK constraint from DD, i.e.
  * from parent's and child's lists of constraints and
@@ -4996,8 +5220,11 @@ on_create_fk_constraint_rollback(struct trigger *trigger, void *event)
 	struct fk_constraint *fk = (struct fk_constraint *)trigger->data;
 	rlist_del_entry(fk, in_parent_space);
 	rlist_del_entry(fk, in_child_space);
+	struct space *child = space_by_id(fk->def->child_id);
+	assert(child != NULL);
+	drop_fk_ck_as_constraint(child, fk->def->name);
 	space_reset_fk_constraint_mask(space_by_id(fk->def->parent_id));
-	space_reset_fk_constraint_mask(space_by_id(fk->def->child_id));
+	space_reset_fk_constraint_mask(child);
 	fk_constraint_delete(fk);
 	return 0;
 }
@@ -5029,8 +5256,14 @@ on_drop_fk_constraint_rollback(struct trigger *trigger, void *event)
 	struct fk_constraint *old_fk = (struct fk_constraint *)trigger->data;
 	struct space *parent = space_by_id(old_fk->def->parent_id);
 	struct space *child = space_by_id(old_fk->def->child_id);
+	assert(child != NULL && parent != NULL);
 	rlist_add_entry(&child->child_fk_constraint, old_fk, in_child_space);
 	rlist_add_entry(&parent->parent_fk_constraint, old_fk, in_parent_space);
+	if (create_fk_ck_as_constraint(child, CONSTRAINT_TYPE_FK,
+				       old_fk->def->name) != 0) {
+		panic("Can't recover after FK constraint drop rollback (out of "
+		      "memory)");
+	}
 	fk_constraint_set_mask(old_fk, &child->fk_constraint_mask,
 			       FIELD_LINK_CHILD);
 	fk_constraint_set_mask(old_fk, &parent->fk_constraint_mask,
@@ -5210,15 +5443,21 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 		fk->def = fk_def;
 		fk->index_id = fk_index->def->iid;
 		if (old_tuple == NULL) {
-			rlist_add_entry(&child_space->child_fk_constraint,
-					fk, in_child_space);
-			rlist_add_entry(&parent_space->parent_fk_constraint,
-					fk, in_parent_space);
+			if (check_existence(child_space, fk_def->name) != 0)
+				return -1;
 			struct trigger *on_rollback =
 				txn_alter_trigger_new(on_create_fk_constraint_rollback,
 						      fk);
 			if (on_rollback == NULL)
 				return -1;
+			if (create_fk_ck_as_constraint(child_space,
+						       CONSTRAINT_TYPE_FK,
+						       fk_def->name) != 0)
+				return -1;
+			rlist_add_entry(&child_space->child_fk_constraint,
+					fk, in_child_space);
+			rlist_add_entry(&parent_space->parent_fk_constraint,
+					fk, in_parent_space);
 			txn_stmt_on_rollback(stmt, on_rollback);
 			fk_constraint_set_mask(fk,
 					       &parent_space->fk_constraint_mask,
@@ -5279,6 +5518,7 @@ on_replace_dd_fk_constraint(struct trigger * /* trigger*/, void *event)
 					      old_fk);
 		if (on_rollback == NULL)
 			return -1;
+		drop_fk_ck_as_constraint(child_space, fk_def->name);
 		txn_stmt_on_rollback(stmt, on_rollback);
 		space_reset_fk_constraint_mask(child_space);
 		space_reset_fk_constraint_mask(parent_space);
@@ -5344,9 +5584,10 @@ on_create_ck_constraint_rollback(struct trigger *trigger, void * /* event */)
 	assert(ck != NULL);
 	struct space *space = space_by_id(ck->def->space_id);
 	assert(space != NULL);
-	assert(space_ck_constraint_by_name(space, ck->def->name,
-					   strlen(ck->def->name)) != NULL);
+	const char *name = ck->def->name;
+	assert(space_ck_constraint_by_name(space, name, strlen(name)) != NULL);
 	space_remove_ck_constraint(space, ck);
+	drop_fk_ck_as_constraint(space, name);
 	ck_constraint_delete(ck);
 	if (trigger_run(&on_alter_space, space) != 0)
 		return -1;
@@ -5371,9 +5612,10 @@ on_drop_ck_constraint_rollback(struct trigger *trigger, void * /* event */)
 	assert(ck != NULL);
 	struct space *space = space_by_id(ck->def->space_id);
 	assert(space != NULL);
-	assert(space_ck_constraint_by_name(space, ck->def->name,
-					   strlen(ck->def->name)) == NULL);
-	if (space_add_ck_constraint(space, ck) != 0)
+	const char *name = ck->def->name;
+	assert(space_ck_constraint_by_name(space, name, strlen(name)) == NULL);
+	if (space_add_ck_constraint(space, ck) != 0 ||
+	    create_fk_ck_as_constraint(space, CONSTRAINT_TYPE_CK, name) != 0)
 		panic("Can't recover after CK constraint drop rollback");
 	if (trigger_run(&on_alter_space, space) != 0)
 		return -1;
@@ -5459,7 +5701,8 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 		const char *name = ck_def->name;
 		struct ck_constraint *old_ck_constraint =
 			space_ck_constraint_by_name(space, name, strlen(name));
-		if (old_ck_constraint != NULL) {
+		bool is_insert = old_ck_constraint == NULL;
+		if (!is_insert) {
 			struct ck_constraint_def *old_def =
 						old_ck_constraint->def;
 			assert(old_def->space_id == ck_def->space_id);
@@ -5491,10 +5734,15 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 		auto ck_guard = make_scoped_guard([=] {
 			ck_constraint_delete(new_ck_constraint);
 		});
-		if (old_ck_constraint != NULL)
-			rlist_del_entry(old_ck_constraint, link);
-		if (space_add_ck_constraint(space, new_ck_constraint) != 0)
+		if (is_insert && check_existence(space, name) != 0)
+			return -1;
+		if (space_add_ck_constraint(space, new_ck_constraint) != 0 ||
+		    (is_insert &&
+		     create_fk_ck_as_constraint(space, CONSTRAINT_TYPE_CK,
+						name) != 0))
 			return -1;
+		if (!is_insert)
+			rlist_del_entry(old_ck_constraint, link);
 		ck_guard.is_active = false;
 		if (old_tuple != NULL) {
 			on_rollback->data = old_ck_constraint;
@@ -5516,6 +5764,7 @@ on_replace_dd_ck_constraint(struct trigger * /* trigger*/, void *event)
 		struct ck_constraint *old_ck_constraint =
 			space_ck_constraint_by_name(space, name, name_len);
 		assert(old_ck_constraint != NULL);
+		drop_fk_ck_as_constraint(space, old_ck_constraint->def->name);
 		space_remove_ck_constraint(space, old_ck_constraint);
 		on_commit->data = old_ck_constraint;
 		on_commit->run = on_drop_ck_constraint_commit;
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
new file mode 100644
index 000000000..a3dd7d531
--- /dev/null
+++ b/test/sql/constraint.result
@@ -0,0 +1,214 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+engine = test_run:get_cfg('engine')
+ | ---
+ | ...
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+ | ---
+ | - row_count: 0
+ | ...
+
+--
+-- Check a constraint name for duplicate within a single
+-- <CREATE TABLE> statement.
+--
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY);')
+ | ---
+ | - row_count: 1
+ | ...
+test_run:cmd("setopt delimiter ';'");
+ | ---
+ | - true
+ | ...
+box.execute([[CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0),
+                               CONSTRAINT c PRIMARY KEY (i));]]);
+ | ---
+ | - null
+ | - Constraint C already exists
+ | ...
+box.execute([[CREATE TABLE t2 (i INT,
+                               CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i),
+                               CONSTRAINT c PRIMARY KEY (i));]]);
+ | ---
+ | - null
+ | - Constraint C already exists
+ | ...
+box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
+                               CONSTRAINT c CHECK (i > 0),
+                               CONSTRAINT c UNIQUE (i));]]);
+ | ---
+ | - null
+ | - Constraint C already exists
+ | ...
+box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
+                               CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i),
+                               CONSTRAINT c UNIQUE (i));]]);
+ | ---
+ | - null
+ | - Constraint C already exists
+ | ...
+box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
+                               CONSTRAINT c CHECK (i > 0),
+                               CONSTRAINT c CHECK (i < 0));]]);
+ | ---
+ | - null
+ | - Constraint C already exists
+ | ...
+box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
+                               CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i),
+                               CONSTRAINT c CHECK (i > 0));]]);
+ | ---
+ | - null
+ | - Constraint C already exists
+ | ...
+box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY CONSTRAINT c REFERENCES t1(i),
+                               CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i))]]);
+ | ---
+ | - null
+ | - Constraint C already exists
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+--
+-- Check a constraint name for duplicate using <ALTER TABLE>
+-- statement.
+--
+box.execute('CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);')
+ | ---
+ | - null
+ | - Constraint C already exists
+ | ...
+box.execute('ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);')
+ | ---
+ | - null
+ | - Index 'C' already exists in space 'T2'
+ | ...
+box.execute('ALTER TABLE t2 ADD CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i);')
+ | ---
+ | - null
+ | - Constraint C already exists
+ | ...
+
+--
+-- Make sure that a constraint's name isn't kept after the
+-- constraint drop.
+--
+box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('DROP INDEX d ON t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 ADD CONSTRAINT d CHECK(i > 0);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.ck_constraint.D:drop()
+ | ---
+ | ...
+box.execute('ALTER TABLE t2 ADD CONSTRAINT d FOREIGN KEY(i) REFERENCES t1(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('ALTER TABLE t2 DROP CONSTRAINT d;')
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- The same inside a transaction.
+--
+test_run:cmd("setopt delimiter ';'");
+ | ---
+ | - true
+ | ...
+box.begin()
+box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+box.execute('DROP INDEX d ON t2;')
+box.execute('ALTER TABLE t2 ADD CONSTRAINT d CHECK(i > 0);')
+box.space.T2.ck_constraint.D:drop()
+box.execute('ALTER TABLE t2 ADD CONSTRAINT d FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT d;')
+box.commit();
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+
+--
+-- Make sure, that altering of an index name affect to its record
+-- in the space's constraint hash table.
+--
+box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.D:alter({name = 'E'})
+ | ---
+ | ...
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+ | ---
+ | - null
+ | - Constraint E already exists
+ | ...
+
+--
+-- Make sure, that altering of an index uniqueness puts/drops
+-- its name to/from the space's constraint hash table.
+--
+box.space.T2.index.E:alter({unique = false})
+ | ---
+ | ...
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+ | ---
+ | - row_count: 1
+ | ...
+box.space.T2.index.E:alter({unique = true})
+ | ---
+ | - error: Constraint E already exists
+ | ...
+box.space.T2.ck_constraint.E:drop()
+ | ---
+ | ...
+box.space.T2.index.E:alter({unique = true})
+ | ---
+ | ...
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+ | ---
+ | - null
+ | - Constraint E already exists
+ | ...
+
+-- Alter name and uniqueness of an unique index simultaneously.
+box.space.T2.index.E:alter({name = 'D', unique = false})
+ | ---
+ | ...
+box.execute('CREATE UNIQUE INDEX e ON t2(i);')
+ | ---
+ | - row_count: 1
+ | ...
+
+--
+-- Cleanup.
+--
+box.execute('DROP TABLE t2;')
+ | ---
+ | - row_count: 1
+ | ...
+box.execute('DROP TABLE t1;')
+ | ---
+ | - row_count: 1
+ | ...
diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua
new file mode 100755
index 000000000..e321b87aa
--- /dev/null
+++ b/test/sql/constraint.test.lua
@@ -0,0 +1,93 @@
+test_run = require('test_run').new()
+engine = test_run:get_cfg('engine')
+box.execute('pragma sql_default_engine=\''..engine..'\'')
+
+--
+-- Check a constraint name for duplicate within a single
+-- <CREATE TABLE> statement.
+--
+box.execute('CREATE TABLE t1 (i INT PRIMARY KEY);')
+test_run:cmd("setopt delimiter ';'");
+box.execute([[CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0),
+                               CONSTRAINT c PRIMARY KEY (i));]]);
+box.execute([[CREATE TABLE t2 (i INT,
+                               CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i),
+                               CONSTRAINT c PRIMARY KEY (i));]]);
+box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
+                               CONSTRAINT c CHECK (i > 0),
+                               CONSTRAINT c UNIQUE (i));]]);
+box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
+                               CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i),
+                               CONSTRAINT c UNIQUE (i));]]);
+box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
+                               CONSTRAINT c CHECK (i > 0),
+                               CONSTRAINT c CHECK (i < 0));]]);
+box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
+                               CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i),
+                               CONSTRAINT c CHECK (i > 0));]]);
+box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY CONSTRAINT c REFERENCES t1(i),
+                               CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i))]]);
+test_run:cmd("setopt delimiter ''");
+
+--
+-- Check a constraint name for duplicate using <ALTER TABLE>
+-- statement.
+--
+box.execute('CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY);')
+box.execute('ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);')
+box.execute('ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);')
+box.execute('ALTER TABLE t2 ADD CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i);')
+
+--
+-- Make sure that a constraint's name isn't kept after the
+-- constraint drop.
+--
+box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+box.execute('DROP INDEX d ON t2;')
+box.execute('ALTER TABLE t2 ADD CONSTRAINT d CHECK(i > 0);')
+box.space.T2.ck_constraint.D:drop()
+box.execute('ALTER TABLE t2 ADD CONSTRAINT d FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT d;')
+
+--
+-- The same inside a transaction.
+--
+test_run:cmd("setopt delimiter ';'");
+box.begin()
+box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+box.execute('DROP INDEX d ON t2;')
+box.execute('ALTER TABLE t2 ADD CONSTRAINT d CHECK(i > 0);')
+box.space.T2.ck_constraint.D:drop()
+box.execute('ALTER TABLE t2 ADD CONSTRAINT d FOREIGN KEY(i) REFERENCES t1(i);')
+box.execute('ALTER TABLE t2 DROP CONSTRAINT d;')
+box.commit();
+test_run:cmd("setopt delimiter ''");
+
+--
+-- Make sure, that altering of an index name affect to its record
+-- in the space's constraint hash table.
+--
+box.execute('CREATE UNIQUE INDEX d ON t2(i);')
+box.space.T2.index.D:alter({name = 'E'})
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+
+--
+-- Make sure, that altering of an index uniqueness puts/drops
+-- its name to/from the space's constraint hash table.
+--
+box.space.T2.index.E:alter({unique = false})
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+box.space.T2.index.E:alter({unique = true})
+box.space.T2.ck_constraint.E:drop()
+box.space.T2.index.E:alter({unique = true})
+box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
+
+-- Alter name and uniqueness of an unique index simultaneously.
+box.space.T2.index.E:alter({name = 'D', unique = false})
+box.execute('CREATE UNIQUE INDEX e ON t2(i);')
+
+--
+-- Cleanup.
+--
+box.execute('DROP TABLE t2;')
+box.execute('DROP TABLE t1;')



More information about the Tarantool-patches mailing list