[Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors

[Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors

[Roman Khabibov]

Improve error message about constraint name duplicate and remove
it from sql.

Follow up #3503
---
 src/box/alter.cc             | 59 +++++++++++++++++++++++++++---------
 src/box/errcode.h            |  2 +-
 src/box/sql/build.c          | 20 ------------
 test/sql-tap/alter2.test.lua |  4 +--
 test/sql/checks.result       |  2 +-
 test/sql/clear.result        |  6 ++--
 test/sql/constraint.result   | 24 +++++++--------
 7 files changed, 64 insertions(+), 53 deletions(-)

diff --git a/src/box/alter.cc b/src/box/alter.cc
index c12279e65..e60c2827b 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2449,6 +2449,47 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
 	return false;
 }
 
+/**
+ * Just return string with constraint type to print it in error
+ * message.
+ */
+static inline const char *
+cosntraint_type_str(struct constraint_def *def) {
+	switch (def->type) {
+	case CONSTRAINT_TYPE_PK:
+		return "PRIMARY KEY";
+	case CONSTRAINT_TYPE_UNIQUE:
+		return "UNIQUE";
+	case CONSTRAINT_TYPE_FK:
+		return "FOREIGN KEY";
+	case CONSTRAINT_TYPE_CK:
+		return "CHECK";
+	}
+}
+
+/**
+ * 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 true  Constraint exists.
+ * @retval false Doesn't exist.
+ */
+static inline bool
+space_constraint_exists(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,
+			 cosntraint_type_str(def), name, space_name(space));
+		return true;
+	}
+	return false;
+}
+
 /**
  * Put the node of an unique index to the constraint hash table of
  * @a space.
@@ -2462,10 +2503,8 @@ create_index_as_constraint(struct alter_space *alter,
 {
 	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);
+	if (space_constraint_exists(space, def->name))
 		return -1;
-	}
 	struct constraint_def *constr_def =
 		constraint_def_new(space->def->id, def->iid == 0 ?
 				   CONSTRAINT_TYPE_PK : CONSTRAINT_TYPE_UNIQUE,
@@ -5348,13 +5387,8 @@ 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);
+			if (space_constraint_exists(child_space, 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,
@@ -5663,13 +5697,10 @@ 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);
+		else if (space_constraint_exists(space, name))
 			return -1;
-		}
 		if (space_add_ck_constraint(space, new_ck_constraint) != 0)
 			return -1;
 		ck_guard.is_active = false;
diff --git a/src/box/errcode.h b/src/box/errcode.h
index c660b1c70..2b922a363 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -222,7 +222,7 @@ struct errcode_record {
 	/*167 */_(ER_CREATE_FK_CONSTRAINT,	"Failed to create foreign key constraint '%s': %s") \
 	/*168 */_(ER_DROP_FK_CONSTRAINT,	"Failed to drop foreign key constraint '%s': %s") \
 	/*169 */_(ER_NO_SUCH_CONSTRAINT,	"Constraint %s does not exist") \
-	/*170 */_(ER_CONSTRAINT_EXISTS,		"Constraint %s already exists") \
+	/*170 */_(ER_CONSTRAINT_EXISTS,		"%s constraint %s already exists within space %s") \
 	/*171 */_(ER_SQL_TYPE_MISMATCH,		"Type mismatch: can not convert %s to %s") \
 	/*172 */_(ER_ROWID_OVERFLOW,            "Rowid is overflowed: too many entries in ephemeral space") \
 	/*173 */_(ER_DROP_COLLATION,		"Can't drop collation %s : %s") \
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 51cd7ce63..ce4629aed 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -994,14 +994,6 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 	sqlVdbeAddOp2(v, OP_Bool, true, ck_constraint_reg + 5);
 	sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 6,
 		      ck_constraint_reg + 6);
-	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS),
-					    ck_def->name);
-	if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
-					      ck_constraint_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
-					      false, OP_NoConflict) != 0)
-		return;
 	sqlVdbeAddOp2(v, OP_SInsert, BOX_CK_CONSTRAINT_ID,
 		      ck_constraint_reg + 6);
 	VdbeComment((v, "Create CK constraint %s", ck_def->name));
@@ -1053,18 +1045,6 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 		sqlVdbeAddOp2(vdbe, OP_Integer, fk->parent_id,
 				  constr_tuple_reg + 2);
 	}
-	/*
-	 * Lets check that constraint with this name hasn't
-	 * been created before.
-	 */
-	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), name_copy);
-	if (vdbe_emit_halt_with_presence_test(parse_context,
-					      BOX_FK_CONSTRAINT_ID, 0,
-					      constr_tuple_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
-					      false, OP_NoConflict) != 0)
-		return;
 	sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
 	sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0,
 			  fk_constraint_match_strs[fk->match], P4_STATIC);
diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
index e0bd60727..e4603e579 100755
--- a/test/sql-tap/alter2.test.lua
+++ b/test/sql-tap/alter2.test.lua
@@ -246,7 +246,7 @@ test:do_catchsql_test(
         ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY (a) REFERENCES child;
     ]], {
         -- <alter2-5.1>
-        1, "Constraint FK already exists"
+        1, "Duplicate key exists in unique index 'primary' in space '_fk_constraint'"
         -- </alter2-5.1>
     })
 
@@ -278,7 +278,7 @@ test:do_catchsql_test(
     "alter2-6.2",
     [[
         ALTER TABLE t1 ADD CONSTRAINT ck CHECK(id > 0);
-    ]], { 1, "Constraint CK already exists" })
+    ]], { 1, "Duplicate key exists in unique index 'primary' in space '_ck_constraint'" })
 
 -- Make sure that CHECK constraint can be created only on empty space.
 --
diff --git a/test/sql/checks.result b/test/sql/checks.result
index a952b2b70..b9e9dfbcd 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -260,7 +260,7 @@ s:drop()
 box.execute("CREATE TABLE T2(ID INT PRIMARY KEY, CONSTRAINT CK1 CHECK(ID > 0), CONSTRAINT CK1 CHECK(ID < 0))")
 ---
 - null
-- Constraint CK1 already exists
+- Duplicate key exists in unique index 'primary' in space '_ck_constraint'
 ...
 box.space.T2
 ---
diff --git a/test/sql/clear.result b/test/sql/clear.result
index afa6520e8..816612da6 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -177,7 +177,7 @@ box.execute("DROP TABLE zoobar")
 box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 CHECK(id < 0));")
 ---
 - null
-- Constraint CK1 already exists
+- Duplicate key exists in unique index 'primary' in space '_ck_constraint'
 ...
 box.space.t1
 ---
@@ -190,7 +190,7 @@ box.space._ck_constraint:select()
 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'
 ...
 box.space.t2
 ---
@@ -207,7 +207,7 @@ box.space._fk_constraint:select()
 box.execute("CREATE TABLE t3(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 FOREIGN KEY(id) REFERENCES t3, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t3, CONSTRAINT ck1 CHECK(id < 0));")
 ---
 - null
-- Constraint CK1 already exists
+- FOREIGN KEY constraint CK1 already exists within space T3
 ...
 box.space.t1
 ---
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index ba262182b..ae2d5b915 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -26,48 +26,48 @@ box.execute([[CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0),
                                CONSTRAINT c PRIMARY KEY (i));]]);
  | ---
  | - null
- | - Constraint C already exists
+ | - PRIMARY KEY constraint C already exists within space T2
  | ...
 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
+ | - PRIMARY KEY constraint C already exists within space T2
  | ...
 box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
                                CONSTRAINT c CHECK (i > 0),
                                CONSTRAINT c UNIQUE (i));]]);
  | ---
  | - null
- | - Constraint C already exists
+ | - UNIQUE constraint C already exists within space T2
  | ...
 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
+ | - UNIQUE constraint C already exists within space T2
  | ...
 box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
                                CONSTRAINT c CHECK (i > 0),
                                CONSTRAINT c CHECK (i < 0));]]);
  | ---
  | - null
- | - Constraint C already exists
+ | - Duplicate key exists in unique index 'primary' in space '_ck_constraint'
  | ...
 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
+ | - FOREIGN KEY constraint C already exists within space T2
  | ...
 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
+ | - Duplicate key exists in unique index 'primary' in space '_fk_constraint'
  | ...
 
 --
@@ -81,7 +81,7 @@ box.execute('CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY);');
 box.execute('ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);');
  | ---
  | - null
- | - Constraint C already exists
+ | - PRIMARY KEY constraint C already exists within space T2
  | ...
 box.execute('ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);');
  | ---
@@ -91,7 +91,7 @@ box.execute('ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);');
 box.execute('ALTER TABLE t2 ADD CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i);');
  | ---
  | - null
- | - Constraint C already exists
+ | - PRIMARY KEY constraint C already exists within space T2
  | ...
 
 --
@@ -150,7 +150,7 @@ box.space.T2.index.D:alter({name = 'E'});
 box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);');
  | ---
  | - null
- | - Constraint E already exists
+ | - UNIQUE constraint E already exists within space T2
  | ...
 
 --
@@ -166,7 +166,7 @@ box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);');
  | ...
 box.space.T2.index.E:alter({unique = true});
  | ---
- | - error: Constraint E already exists
+ | - error: CHECK constraint E already exists within space T2
  | ...
 box.space.T2.ck_constraint.E:drop();
  | ---
@@ -177,7 +177,7 @@ box.space.T2.index.E:alter({unique = true});
 box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);');
  | ---
  | - null
- | - Constraint E already exists
+ | - UNIQUE constraint E already exists within space T2
  | ...
 
 -- Alter name and uniqueness of an unique index simultaneously.
-- 
2.21.0 (Apple Git-122)

Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors

[Cyrill Gorcunov]

On Wed, Dec 04, 2019 at 07:23:11PM +0300, Roman Khabibov wrote:
>  
> +/**
> + * Just return string with constraint type to print it in error
> + * message.
> + */
> +static inline const char *
> +cosntraint_type_str(struct constraint_def *def) {
> +	switch (def->type) {
> +	case CONSTRAINT_TYPE_PK:
> +		return "PRIMARY KEY";
> +	case CONSTRAINT_TYPE_UNIQUE:
> +		return "UNIQUE";
> +	case CONSTRAINT_TYPE_FK:
> +		return "FOREIGN KEY";
> +	case CONSTRAINT_TYPE_CK:
> +		return "CHECK";
> +	}
> +}

Should not here be either default case or assert()? What would
be returned if someone add a new def->type and forget about
this place?

Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors

[Konstantin Osipov]

* Roman Khabibov <roman.habibov@tarantool.org> [19/12/04 19:25]:
> + * 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 true  Constraint exists.
> + * @retval false Doesn't exist.
> + */
> +static inline bool
> +space_constraint_exists(struct space *space, const char *name)
> +{

Looks like it belongs to space.h/c


-- 
Konstantin Osipov, Moscow, Russia

Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors

[Roman Khabibov]

> On Dec 4, 2019, at 20:54, Konstantin Osipov <kostja.osipov@gmail.com> wrote:
> 
> * Roman Khabibov <roman.habibov@tarantool.org> [19/12/04 19:25]:
>> + * 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 true  Constraint exists.
>> + * @retval false Doesn't exist.
>> + */
>> +static inline bool
>> +space_constraint_exists(struct space *space, const char *name)
>> +{
> 
> Looks like it belongs to space.h/c
> 
> 
> -- 
> Konstantin Osipov, Moscow, Russia

Vlad, please, don’t see the previous.

Cyrill, I added both for sure.

Konstantin, I wrapped the piece of code used only in alter.cc and logically related to alter.cc
by this function, so I decided to keep this function in alter.cc only. Maybe, it is better to move
it to space.h/c. Let’s listen to Vlad’s opinion, then we’ll decide.

commit e12c95007f465c98646206d4e83a620b0d2aff36
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Wed Dec 4 12:38:29 2019 +0300

    box: let only box handle constraint dup errors
    
    Improve error message about constraint name duplicate and remove
    it from sql.
    
    Follow up #3503

diff --git a/src/box/alter.cc b/src/box/alter.cc
index c03bad589..a99605703 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2448,6 +2448,53 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
 	return false;
 }
 
+/**
+ * Just return string with constraint type to print it in error
+ * message.
+ */
+static inline const char *
+cosntraint_type_str(struct constraint_def *def) {
+	assert(def->type == CONSTRAINT_TYPE_PK || def->type ==
+	       CONSTRAINT_TYPE_UNIQUE || def->type == CONSTRAINT_TYPE_FK ||
+	       def->type == CONSTRAINT_TYPE_CK);
+	switch (def->type) {
+	case CONSTRAINT_TYPE_PK:
+		return "PRIMARY KEY";
+	case CONSTRAINT_TYPE_UNIQUE:
+		return "UNIQUE";
+	case CONSTRAINT_TYPE_FK:
+		return "FOREIGN KEY";
+	case CONSTRAINT_TYPE_CK:
+		return "CHECK";
+	default:
+		break;
+	}
+	return NULL;
+}
+
+/**
+ * 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 true  Constraint exists.
+ * @retval false Doesn't exist.
+ */
+static inline bool
+space_constraint_exists(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,
+			 cosntraint_type_str(def), name, space_name(space));
+		return true;
+	}
+	return false;
+}
+
 /**
  * Put the node of an unique index to the constraint hash table of
  * @a space.
@@ -2461,10 +2508,8 @@ create_index_as_constraint(struct alter_space *alter,
 {
 	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);
+	if (space_constraint_exists(space, def->name))
 		return -1;
-	}
 	struct constraint_def *constr_def =
 		constraint_def_new(space->def->id, def->iid == 0 ?
 				   CONSTRAINT_TYPE_PK : CONSTRAINT_TYPE_UNIQUE,
@@ -5391,13 +5436,8 @@ 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);
+			if (space_constraint_exists(child_space, 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,
@@ -5710,13 +5750,10 @@ 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);
+		else if (space_constraint_exists(space, name))
 			return -1;
-		}
 		if (space_add_ck_constraint(space, new_ck_constraint) != 0)
 			return -1;
 		ck_guard.is_active = false;
diff --git a/src/box/errcode.h b/src/box/errcode.h
index c660b1c70..daf32331c 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -222,7 +222,7 @@ struct errcode_record {
 	/*167 */_(ER_CREATE_FK_CONSTRAINT,	"Failed to create foreign key constraint '%s': %s") \
 	/*168 */_(ER_DROP_FK_CONSTRAINT,	"Failed to drop foreign key constraint '%s': %s") \
 	/*169 */_(ER_NO_SUCH_CONSTRAINT,	"Constraint %s does not exist") \
-	/*170 */_(ER_CONSTRAINT_EXISTS,		"Constraint %s already exists") \
+	/*170 */_(ER_CONSTRAINT_EXISTS,		"%s constraint '%s' already exists within space '%s'") \
 	/*171 */_(ER_SQL_TYPE_MISMATCH,		"Type mismatch: can not convert %s to %s") \
 	/*172 */_(ER_ROWID_OVERFLOW,            "Rowid is overflowed: too many entries in ephemeral space") \
 	/*173 */_(ER_DROP_COLLATION,		"Can't drop collation %s : %s") \
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 51cd7ce63..ce4629aed 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -994,14 +994,6 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 	sqlVdbeAddOp2(v, OP_Bool, true, ck_constraint_reg + 5);
 	sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 6,
 		      ck_constraint_reg + 6);
-	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS),
-					    ck_def->name);
-	if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
-					      ck_constraint_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
-					      false, OP_NoConflict) != 0)
-		return;
 	sqlVdbeAddOp2(v, OP_SInsert, BOX_CK_CONSTRAINT_ID,
 		      ck_constraint_reg + 6);
 	VdbeComment((v, "Create CK constraint %s", ck_def->name));
@@ -1053,18 +1045,6 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 		sqlVdbeAddOp2(vdbe, OP_Integer, fk->parent_id,
 				  constr_tuple_reg + 2);
 	}
-	/*
-	 * Lets check that constraint with this name hasn't
-	 * been created before.
-	 */
-	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), name_copy);
-	if (vdbe_emit_halt_with_presence_test(parse_context,
-					      BOX_FK_CONSTRAINT_ID, 0,
-					      constr_tuple_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
-					      false, OP_NoConflict) != 0)
-		return;
 	sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
 	sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0,
 			  fk_constraint_match_strs[fk->match], P4_STATIC);
diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
index e0bd60727..e4603e579 100755
--- a/test/sql-tap/alter2.test.lua
+++ b/test/sql-tap/alter2.test.lua
@@ -246,7 +246,7 @@ test:do_catchsql_test(
         ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY (a) REFERENCES child;
     ]], {
         -- <alter2-5.1>
-        1, "Constraint FK already exists"
+        1, "Duplicate key exists in unique index 'primary' in space '_fk_constraint'"
         -- </alter2-5.1>
     })
 
@@ -278,7 +278,7 @@ test:do_catchsql_test(
     "alter2-6.2",
     [[
         ALTER TABLE t1 ADD CONSTRAINT ck CHECK(id > 0);
-    ]], { 1, "Constraint CK already exists" })
+    ]], { 1, "Duplicate key exists in unique index 'primary' in space '_ck_constraint'" })
 
 -- Make sure that CHECK constraint can be created only on empty space.
 --
diff --git a/test/sql/checks.result b/test/sql/checks.result
index a952b2b70..b9e9dfbcd 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -260,7 +260,7 @@ s:drop()
 box.execute("CREATE TABLE T2(ID INT PRIMARY KEY, CONSTRAINT CK1 CHECK(ID > 0), CONSTRAINT CK1 CHECK(ID < 0))")
 ---
 - null
-- Constraint CK1 already exists
+- Duplicate key exists in unique index 'primary' in space '_ck_constraint'
 ...
 box.space.T2
 ---
diff --git a/test/sql/clear.result b/test/sql/clear.result
index afa6520e8..68baa9320 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -177,7 +177,7 @@ box.execute("DROP TABLE zoobar")
 box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 CHECK(id < 0));")
 ---
 - null
-- Constraint CK1 already exists
+- Duplicate key exists in unique index 'primary' in space '_ck_constraint'
 ...
 box.space.t1
 ---
@@ -190,7 +190,7 @@ box.space._ck_constraint:select()
 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'
 ...
 box.space.t2
 ---
@@ -207,7 +207,7 @@ box.space._fk_constraint:select()
 box.execute("CREATE TABLE t3(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 FOREIGN KEY(id) REFERENCES t3, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t3, CONSTRAINT ck1 CHECK(id < 0));")
 ---
 - null
-- Constraint CK1 already exists
+- FOREIGN KEY constraint 'CK1' already exists within space 'T3'
 ...
 box.space.t1
 ---
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index ba262182b..63562aaf0 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -26,48 +26,48 @@ box.execute([[CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0),
                                CONSTRAINT c PRIMARY KEY (i));]]);
  | ---
  | - null
- | - Constraint C already exists
+ | - PRIMARY KEY constraint 'C' already exists within space 'T2'
  | ...
 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
+ | - PRIMARY KEY constraint 'C' already exists within space 'T2'
  | ...
 box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
                                CONSTRAINT c CHECK (i > 0),
                                CONSTRAINT c UNIQUE (i));]]);
  | ---
  | - null
- | - Constraint C already exists
+ | - UNIQUE constraint 'C' already exists within space 'T2'
  | ...
 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
+ | - UNIQUE constraint 'C' already exists within space 'T2'
  | ...
 box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
                                CONSTRAINT c CHECK (i > 0),
                                CONSTRAINT c CHECK (i < 0));]]);
  | ---
  | - null
- | - Constraint C already exists
+ | - Duplicate key exists in unique index 'primary' in space '_ck_constraint'
  | ...
 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
+ | - FOREIGN KEY constraint 'C' already exists within space 'T2'
  | ...
 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
+ | - Duplicate key exists in unique index 'primary' in space '_fk_constraint'
  | ...
 
 --
@@ -81,7 +81,7 @@ box.execute('CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY);');
 box.execute('ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);');
  | ---
  | - null
- | - Constraint C already exists
+ | - PRIMARY KEY constraint 'C' already exists within space 'T2'
  | ...
 box.execute('ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);');
  | ---
@@ -91,7 +91,7 @@ box.execute('ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);');
 box.execute('ALTER TABLE t2 ADD CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i);');
  | ---
  | - null
- | - Constraint C already exists
+ | - PRIMARY KEY constraint 'C' already exists within space 'T2'
  | ...
 
 --
@@ -150,7 +150,7 @@ box.space.T2.index.D:alter({name = 'E'});
 box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);');
  | ---
  | - null
- | - Constraint E already exists
+ | - UNIQUE constraint 'E' already exists within space 'T2'
  | ...
 
 --
@@ -166,7 +166,7 @@ box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);');
  | ...
 box.space.T2.index.E:alter({unique = true});
  | ---
- | - error: Constraint E already exists
+ | - error: CHECK constraint 'E' already exists within space 'T2'
  | ...
 box.space.T2.ck_constraint.E:drop();
  | ---
@@ -177,7 +177,7 @@ box.space.T2.index.E:alter({unique = true});
 box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);');
  | ---
  | - null
- | - Constraint E already exists
+ | - UNIQUE constraint 'E' already exists within space 'T2'
  | ...
 
 -- Alter name and uniqueness of an unique index simultaneously.

Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors

[Cyrill Gorcunov]

On Thu, Dec 05, 2019 at 09:59:33PM +0300, Roman Khabibov wrote:
> 
> Vlad, please, don’t see the previous.
> 
> Cyrill, I added both for sure.

Thanks!

> +/**
> + * Just return string with constraint type to print it in error
> + * message.
> + */
> +static inline const char *
> +cosntraint_type_str(struct constraint_def *def) {
> +	assert(def->type == CONSTRAINT_TYPE_PK || def->type ==
> +	       CONSTRAINT_TYPE_UNIQUE || def->type == CONSTRAINT_TYPE_FK ||
> +	       def->type == CONSTRAINT_TYPE_CK);
> +	switch (def->type) {
> +	case CONSTRAINT_TYPE_PK:
> +		return "PRIMARY KEY";
> +	case CONSTRAINT_TYPE_UNIQUE:
> +		return "UNIQUE";
> +	case CONSTRAINT_TYPE_FK:
> +		return "FOREIGN KEY";
> +	case CONSTRAINT_TYPE_CK:
> +		return "CHECK";
> +	default:
> +		break;
> +	}
> +	return NULL;
> +}

This is a way better than it was. To be honest I'm not strong
in the whole context of the series, but when I need to provide
some symbolic name I do something like

/**
 * popen_state_str - get process state string representation
 * @state:	process state
 */
const char *
popen_state_str(unsigned int state)
{
	/*
	 * A process may be in a number of states,
	 * running/sleeping/disk sleep/stopped and etc
	 * but we are interested only if it is alive
	 * or dead (via plain exit or kill signal).
	 *
	 * Thus while it present in a system and not
	 * yet reaped we call it "alive".
	 *
	 * Note this is API for lua, so change with
	 * caution if needed.
	 */
	static const char * state_str[POPEN_STATE_MAX] = {
		[POPEN_STATE_NONE]	= "none",
		[POPEN_STATE_RUNNING]	= "alive",
		[POPEN_STATE_EXITED]	= "exited",
		[POPEN_STATE_SIGNALED]	= "signaled",
	};

	return state < POPEN_STATE_MAX ? state_str[state] : "unknown";
}

but note that I know the number of entries so it might not fit
your needs thus I'm fine with your current code. Thanks!

Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors

[Vladislav Shpilevoy]

Thanks for the fixes!

See 4 comments below.

> diff --git a/src/box/alter.cc b/src/box/alter.cc
> index c12279e65..e60c2827b 100644
> --- a/src/box/alter.cc
> +++ b/src/box/alter.cc
> @@ -2449,6 +2449,47 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
>  	return false;
>  }
>  
> +/**
> + * Just return string with constraint type to print it in error
> + * message.
> + */
> +static inline const char *
> +cosntraint_type_str(struct constraint_def *def) {
> +	switch (def->type) {
> +	case CONSTRAINT_TYPE_PK:
> +		return "PRIMARY KEY";
> +	case CONSTRAINT_TYPE_UNIQUE:
> +		return "UNIQUE";
> +	case CONSTRAINT_TYPE_FK:
> +		return "FOREIGN KEY";
> +	case CONSTRAINT_TYPE_CK:
> +		return "CHECK";
> +	}
> +}

1. Please, use the way it is implemented for other
enums - add const char *constraint_type_strs[] array
and get a string via constraint_type_strs[type].

> +
> +/**
> + * 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 true  Constraint exists.
> + * @retval false Doesn't exist.
> + */
> +static inline bool
> +space_constraint_exists(struct space *space, const char *name)

2. Please, rename to 'check_existence()'. When you write 'exists',
it looks like a simple getter method, which does not change anything.
Your method does - it changes diagnostics area. And then it will
return int: 0 or -1, as usual.

> +{
> +	struct constraint_def *def =
> +		space_constraint_def_by_name(space, name);
> +	if (def != NULL) {
> +		diag_set(ClientError, ER_CONSTRAINT_EXISTS,
> +			 cosntraint_type_str(def), name, space_name(space));
> +		return true;
> +	}
> +	return false;
> +}
> @@ -5348,13 +5387,8 @@ 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);
> +			if (space_constraint_exists(child_space, fk_def->name))
>  				return -1;
> -			}

3. I propose you to introduce space_constraint_exists in the previous
commit so as here the diff will be smaller.

>  			rlist_add_entry(&child_space->child_fk_constraint,
>  					fk, in_child_space);
>  			rlist_add_entry(&parent_space->parent_fk_constraint,> diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
> index e0bd60727..e4603e579 100755
> --- a/test/sql-tap/alter2.test.lua
> +++ b/test/sql-tap/alter2.test.lua
> @@ -246,7 +246,7 @@ test:do_catchsql_test(
>          ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY (a) REFERENCES child;
>      ]], {
>          -- <alter2-5.1>
> -        1, "Constraint FK already exists"
> +        1, "Duplicate key exists in unique index 'primary' in space '_fk_constraint'"

4. Well, maybe I was wrong, and we should keep vdbe_emit_halt_with_presence_test.
It was added exactly to avoid such ugly error messages.

But the problem is that ER_CONSTRAINT_EXISTS expects multiple arguments, which
can't be passed through vdbe_emit_halt_with_presence_test.

I propose you to turn ER_CONSTRAINT_EXISTS into ER_FK_EXISTS in
vdbe_emit_fk_constraint_create, and into ER_CK_EXISTS in
vdbe_emit_ck_constraint_create. ER_CK/FK_EXISTS will have only
one parameter.

>          -- </alter2-5.1>
>      })
>  

Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors

[Roman Khabibov]

> On Dec 7, 2019, at 19:35, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Thanks for the fixes!
> 
> See 4 comments below.
> 
>> diff --git a/src/box/alter.cc b/src/box/alter.cc
>> index c12279e65..e60c2827b 100644
>> --- a/src/box/alter.cc
>> +++ b/src/box/alter.cc
>> @@ -2449,6 +2449,47 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
>> 	return false;
>> }
>> 
>> +/**
>> + * Just return string with constraint type to print it in error
>> + * message.
>> + */
>> +static inline const char *
>> +cosntraint_type_str(struct constraint_def *def) {
>> +	switch (def->type) {
>> +	case CONSTRAINT_TYPE_PK:
>> +		return "PRIMARY KEY";
>> +	case CONSTRAINT_TYPE_UNIQUE:
>> +		return "UNIQUE";
>> +	case CONSTRAINT_TYPE_FK:
>> +		return "FOREIGN KEY";
>> +	case CONSTRAINT_TYPE_CK:
>> +		return "CHECK";
>> +	}
>> +}
> 
> 1. Please, use the way it is implemented for other
> enums - add const char *constraint_type_strs[] array
> and get a string via constraint_type_strs[type].
+/**
+ * Just return string with constraint type to print it in an error
+ * message.
+ */
+static inline const char *
+constraint_type_str(struct constraint_def *def)
+{
+	static const char *type_str[CONSTRAINT_TYPE_MAX] = {
+		[CONSTRAINT_TYPE_PK]		= "PRIMARY KEY",
+		[CONSTRAINT_TYPE_UNIQUE]	= "UNIQUE",
+		[CONSTRAINT_TYPE_FK]		= "FOREIGN KEY",
+		[CONSTRAINT_TYPE_CK]		= "CHECK",
+	};
+
+	return def->type <= CONSTRAINT_TYPE_MAX ? type_str[def->type] :
+		"UNKNOWN";
+}
+

>> +
>> +/**
>> + * 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 true  Constraint exists.
>> + * @retval false Doesn't exist.
>> + */
>> +static inline bool
>> +space_constraint_exists(struct space *space, const char *name)
> 
> 2. Please, rename to 'check_existence()'. When you write 'exists',
> it looks like a simple getter method, which does not change anything.
> Your method does - it changes diagnostics area. And then it will
> return int: 0 or -1, as usual.
 /**
  * Check if constraint with @a name exists within @a space. Call
  * diag_set() if it is so.
@@ -2464,7 +2482,8 @@ 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);
+		diag_set(ClientError, ER_CONSTRAINT_EXISTS,
+			 constraint_type_str(def), name, space_name(space));
 		return -1;
 	}

>> +{
>> +	struct constraint_def *def =
>> +		space_constraint_def_by_name(space, name);
>> +	if (def != NULL) {
>> +		diag_set(ClientError, ER_CONSTRAINT_EXISTS,
>> +			 cosntraint_type_str(def), name, space_name(space));
>> +		return true;
>> +	}
>> +	return false;
>> +}
>> @@ -5348,13 +5387,8 @@ 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);
>> +			if (space_constraint_exists(child_space, fk_def->name))
>> 				return -1;
>> -			}
> 
> 3. I propose you to introduce space_constraint_exists in the previous
> commit so as here the diff will be smaller.

>> 			rlist_add_entry(&child_space->child_fk_constraint,
>> 					fk, in_child_space);
>> 			rlist_add_entry(&parent_space->parent_fk_constraint,> diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
>> index e0bd60727..e4603e579 100755
>> --- a/test/sql-tap/alter2.test.lua
>> +++ b/test/sql-tap/alter2.test.lua
>> @@ -246,7 +246,7 @@ test:do_catchsql_test(
>>         ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY (a) REFERENCES child;
>>     ]], {
>>         -- <alter2-5.1>
>> -        1, "Constraint FK already exists"
>> +        1, "Duplicate key exists in unique index 'primary' in space '_fk_constraint'"
> 
> 4. Well, maybe I was wrong, and we should keep vdbe_emit_halt_with_presence_test.
> It was added exactly to avoid such ugly error messages.
> 
> But the problem is that ER_CONSTRAINT_EXISTS expects multiple arguments, which
> can't be passed through vdbe_emit_halt_with_presence_test.
> 
> I propose you to turn ER_CONSTRAINT_EXISTS into ER_FK_EXISTS in
> vdbe_emit_fk_constraint_create, and into ER_CK_EXISTS in
> vdbe_emit_ck_constraint_create. ER_CK/FK_EXISTS will have only
> one parameter.
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 51cd7ce63..556df7786 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -584,7 +584,7 @@ trim_space_snprintf(char *wptr, const char *str, uint32_t str_len)
 static void
 vdbe_emit_ck_constraint_create(struct Parse *parser,
 			       const struct ck_constraint_def *ck_def,
-			       uint32_t reg_space_id);
+			       uint32_t reg_space_id, const char *space_name);
 
 void
 sql_create_check_contraint(struct Parse *parser)
@@ -664,7 +664,8 @@ sql_create_check_contraint(struct Parse *parser)
 		struct Vdbe *v = sqlGetVdbe(parser);
 		sqlVdbeAddOp2(v, OP_Integer, space->def->id,
 			      space_id_reg);
-		vdbe_emit_ck_constraint_create(parser, ck_def, space_id_reg);
+		vdbe_emit_ck_constraint_create(parser, ck_def, space_id_reg,
+					       space->def->name);
 		assert(sqlVdbeGetOp(v, v->nOp - 1)->opcode == OP_SInsert);
 		sqlVdbeCountChanges(v);
 		sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
@@ -973,7 +974,7 @@ emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id)
 static void
 vdbe_emit_ck_constraint_create(struct Parse *parser,
 			       const struct ck_constraint_def *ck_def,
-			       uint32_t reg_space_id)
+			       uint32_t reg_space_id, const char *space_name)
 {
 	struct sql *db = parser->db;
 	struct Vdbe *v = sqlGetVdbe(parser);
@@ -995,12 +996,13 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 	sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 6,
 		      ck_constraint_reg + 6);
 	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS),
-					    ck_def->name);
+		tt_sprintf(tnt_errcode_desc(ER_CK_CONSTRAINT_EXISTS),
+					    ck_def->name, space_name);
 	if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
 					      ck_constraint_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
-					      false, OP_NoConflict) != 0)
+					      ER_CK_CONSTRAINT_EXISTS,
+					      error_msg, false,
+					      OP_NoConflict) != 0)
 		return;
 	sqlVdbeAddOp2(v, OP_SInsert, BOX_CK_CONSTRAINT_ID,
 		      ck_constraint_reg + 6);
@@ -1017,7 +1019,8 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
  */
 static void
 vdbe_emit_fk_constraint_create(struct Parse *parse_context,
-			       const struct fk_constraint_def *fk)
+			       const struct fk_constraint_def *fk,
+			       const char *space_name)
 {
 	assert(parse_context != NULL);
 	assert(fk != NULL);
@@ -1058,12 +1061,14 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 	 * been created before.
 	 */
 	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), name_copy);
+		tt_sprintf(tnt_errcode_desc(ER_FK_CONSTRAINT_EXISTS),
+			   name_copy, space_name);
 	if (vdbe_emit_halt_with_presence_test(parse_context,
 					      BOX_FK_CONSTRAINT_ID, 0,
 					      constr_tuple_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
-					      false, OP_NoConflict) != 0)
+					      ER_FK_CONSTRAINT_EXISTS,
+					      error_msg, false,
+					      OP_NoConflict) != 0)
 		return;
 	sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
 	sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0,
@@ -1272,13 +1277,13 @@ sqlEndTable(struct Parse *pParse)
 			fk_def->parent_id = reg_space_id;
 		}
 		fk_def->child_id = reg_space_id;
-		vdbe_emit_fk_constraint_create(pParse, fk_def);
+		vdbe_emit_fk_constraint_create(pParse, fk_def, space_name_copy);
 	}
 	struct ck_constraint_parse *ck_parse;
 	rlist_foreach_entry(ck_parse, &pParse->create_table_def.new_check,
 			    link) {
 		vdbe_emit_ck_constraint_create(pParse, ck_parse->ck_def,
-					       reg_space_id);
+					       reg_space_id, space_name_copy);
 	}
 }
 
@@ -1959,7 +1964,8 @@ sql_create_foreign_key(struct Parse *parse_context)
 					  struct fk_constraint_parse, link);
 		fk_parse->fk_def = fk_def;
 	} else {
-		vdbe_emit_fk_constraint_create(parse_context, fk_def);
+		vdbe_emit_fk_constraint_create(parse_context, fk_def,
+					       child_space->def->name);
 	}


commit b0ab5c12e9fa3ab78a747294164b81fabd3b42f0
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Wed Dec 4 12:38:29 2019 +0300

    box: let only box handle constraint dup errors
    
    Improve error message about constraint name duplicate and remove
    it from sql.
    
    Follow up #3503

diff --git a/src/box/alter.cc b/src/box/alter.cc
index d5e836ae7..b1ef7ec4f 100644
--- a/src/box/alter.cc
+++ b/src/box/alter.cc
@@ -2449,6 +2449,24 @@ index_is_used_by_fk_constraint(struct rlist *fk_list, uint32_t iid)
 	return false;
 }
 
+/**
+ * Just return string with constraint type to print it in an error
+ * message.
+ */
+static inline const char *
+constraint_type_str(struct constraint_def *def)
+{
+	static const char *type_str[CONSTRAINT_TYPE_MAX] = {
+		[CONSTRAINT_TYPE_PK]		= "PRIMARY KEY",
+		[CONSTRAINT_TYPE_UNIQUE]	= "UNIQUE",
+		[CONSTRAINT_TYPE_FK]		= "FOREIGN KEY",
+		[CONSTRAINT_TYPE_CK]		= "CHECK",
+	};
+
+	return def->type <= CONSTRAINT_TYPE_MAX ? type_str[def->type] :
+		"UNKNOWN";
+}
+
 /**
  * Check if constraint with @a name exists within @a space. Call
  * diag_set() if it is so.
@@ -2464,7 +2482,8 @@ 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);
+		diag_set(ClientError, ER_CONSTRAINT_EXISTS,
+			 constraint_type_str(def), name, space_name(space));
 		return -1;
 	}
 	return 0;
diff --git a/src/box/errcode.h b/src/box/errcode.h
index c660b1c70..658f64e9b 100644
--- a/src/box/errcode.h
+++ b/src/box/errcode.h
@@ -222,7 +222,7 @@ struct errcode_record {
 	/*167 */_(ER_CREATE_FK_CONSTRAINT,	"Failed to create foreign key constraint '%s': %s") \
 	/*168 */_(ER_DROP_FK_CONSTRAINT,	"Failed to drop foreign key constraint '%s': %s") \
 	/*169 */_(ER_NO_SUCH_CONSTRAINT,	"Constraint %s does not exist") \
-	/*170 */_(ER_CONSTRAINT_EXISTS,		"Constraint %s already exists") \
+	/*170 */_(ER_CONSTRAINT_EXISTS,		"%s constraint '%s' already exists within space '%s'") \
 	/*171 */_(ER_SQL_TYPE_MISMATCH,		"Type mismatch: can not convert %s to %s") \
 	/*172 */_(ER_ROWID_OVERFLOW,            "Rowid is overflowed: too many entries in ephemeral space") \
 	/*173 */_(ER_DROP_COLLATION,		"Can't drop collation %s : %s") \
@@ -258,6 +258,8 @@ struct errcode_record {
 	/*203 */_(ER_BOOTSTRAP_READONLY,	"Trying to bootstrap a local read-only instance as master") \
 	/*204 */_(ER_SQL_FUNC_WRONG_RET_COUNT,	"SQL expects exactly one argument returned from %s, got %d")\
 	/*205 */_(ER_FUNC_INVALID_RETURN_TYPE,	"Function '%s' returned value of invalid type: expected %s got %s") \
+	/*206 */_(ER_CK_CONSTRAINT_EXISTS,	"CHECK constraint '%s' already exists within space '%s'") \
+	/*207 */_(ER_FK_CONSTRAINT_EXISTS,	"FOREIGN KEY constraint '%s' already exists within space '%s'") \
 
 /*
  * !IMPORTANT! Please follow instructions at start of the file
diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 51cd7ce63..556df7786 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -584,7 +584,7 @@ trim_space_snprintf(char *wptr, const char *str, uint32_t str_len)
 static void
 vdbe_emit_ck_constraint_create(struct Parse *parser,
 			       const struct ck_constraint_def *ck_def,
-			       uint32_t reg_space_id);
+			       uint32_t reg_space_id, const char *space_name);
 
 void
 sql_create_check_contraint(struct Parse *parser)
@@ -664,7 +664,8 @@ sql_create_check_contraint(struct Parse *parser)
 		struct Vdbe *v = sqlGetVdbe(parser);
 		sqlVdbeAddOp2(v, OP_Integer, space->def->id,
 			      space_id_reg);
-		vdbe_emit_ck_constraint_create(parser, ck_def, space_id_reg);
+		vdbe_emit_ck_constraint_create(parser, ck_def, space_id_reg,
+					       space->def->name);
 		assert(sqlVdbeGetOp(v, v->nOp - 1)->opcode == OP_SInsert);
 		sqlVdbeCountChanges(v);
 		sqlVdbeChangeP5(v, OPFLAG_NCHANGE);
@@ -973,7 +974,7 @@ emitNewSysSpaceSequenceRecord(Parse *pParse, int reg_space_id, int reg_seq_id)
 static void
 vdbe_emit_ck_constraint_create(struct Parse *parser,
 			       const struct ck_constraint_def *ck_def,
-			       uint32_t reg_space_id)
+			       uint32_t reg_space_id, const char *space_name)
 {
 	struct sql *db = parser->db;
 	struct Vdbe *v = sqlGetVdbe(parser);
@@ -995,12 +996,13 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
 	sqlVdbeAddOp3(v, OP_MakeRecord, ck_constraint_reg, 6,
 		      ck_constraint_reg + 6);
 	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS),
-					    ck_def->name);
+		tt_sprintf(tnt_errcode_desc(ER_CK_CONSTRAINT_EXISTS),
+					    ck_def->name, space_name);
 	if (vdbe_emit_halt_with_presence_test(parser, BOX_CK_CONSTRAINT_ID, 0,
 					      ck_constraint_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
-					      false, OP_NoConflict) != 0)
+					      ER_CK_CONSTRAINT_EXISTS,
+					      error_msg, false,
+					      OP_NoConflict) != 0)
 		return;
 	sqlVdbeAddOp2(v, OP_SInsert, BOX_CK_CONSTRAINT_ID,
 		      ck_constraint_reg + 6);
@@ -1017,7 +1019,8 @@ vdbe_emit_ck_constraint_create(struct Parse *parser,
  */
 static void
 vdbe_emit_fk_constraint_create(struct Parse *parse_context,
-			       const struct fk_constraint_def *fk)
+			       const struct fk_constraint_def *fk,
+			       const char *space_name)
 {
 	assert(parse_context != NULL);
 	assert(fk != NULL);
@@ -1058,12 +1061,14 @@ vdbe_emit_fk_constraint_create(struct Parse *parse_context,
 	 * been created before.
 	 */
 	const char *error_msg =
-		tt_sprintf(tnt_errcode_desc(ER_CONSTRAINT_EXISTS), name_copy);
+		tt_sprintf(tnt_errcode_desc(ER_FK_CONSTRAINT_EXISTS),
+			   name_copy, space_name);
 	if (vdbe_emit_halt_with_presence_test(parse_context,
 					      BOX_FK_CONSTRAINT_ID, 0,
 					      constr_tuple_reg, 2,
-					      ER_CONSTRAINT_EXISTS, error_msg,
-					      false, OP_NoConflict) != 0)
+					      ER_FK_CONSTRAINT_EXISTS,
+					      error_msg, false,
+					      OP_NoConflict) != 0)
 		return;
 	sqlVdbeAddOp2(vdbe, OP_Bool, fk->is_deferred, constr_tuple_reg + 3);
 	sqlVdbeAddOp4(vdbe, OP_String8, 0, constr_tuple_reg + 4, 0,
@@ -1272,13 +1277,13 @@ sqlEndTable(struct Parse *pParse)
 			fk_def->parent_id = reg_space_id;
 		}
 		fk_def->child_id = reg_space_id;
-		vdbe_emit_fk_constraint_create(pParse, fk_def);
+		vdbe_emit_fk_constraint_create(pParse, fk_def, space_name_copy);
 	}
 	struct ck_constraint_parse *ck_parse;
 	rlist_foreach_entry(ck_parse, &pParse->create_table_def.new_check,
 			    link) {
 		vdbe_emit_ck_constraint_create(pParse, ck_parse->ck_def,
-					       reg_space_id);
+					       reg_space_id, space_name_copy);
 	}
 }
 
@@ -1959,7 +1964,8 @@ sql_create_foreign_key(struct Parse *parse_context)
 					  struct fk_constraint_parse, link);
 		fk_parse->fk_def = fk_def;
 	} else {
-		vdbe_emit_fk_constraint_create(parse_context, fk_def);
+		vdbe_emit_fk_constraint_create(parse_context, fk_def,
+					       child_space->def->name);
 	}
 
 exit_create_fk:
diff --git a/test/box/misc.result b/test/box/misc.result
index d2a20307a..c343726da 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -554,6 +554,8 @@ t;
   203: box.error.BOOTSTRAP_READONLY
   204: box.error.SQL_FUNC_WRONG_RET_COUNT
   205: box.error.FUNC_INVALID_RETURN_TYPE
+  206: box.error.CK_CONSTRAINT_EXISTS
+  207: box.error.FK_CONSTRAINT_EXISTS
 ...
 test_run:cmd("setopt delimiter ''");
 ---
diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua
index e0bd60727..f1b4ff660 100755
--- a/test/sql-tap/alter2.test.lua
+++ b/test/sql-tap/alter2.test.lua
@@ -246,7 +246,7 @@ test:do_catchsql_test(
         ALTER TABLE child ADD CONSTRAINT fk FOREIGN KEY (a) REFERENCES child;
     ]], {
         -- <alter2-5.1>
-        1, "Constraint FK already exists"
+        1, "FOREIGN KEY constraint 'FK' already exists within space 'CHILD'"
         -- </alter2-5.1>
     })
 
@@ -278,7 +278,7 @@ test:do_catchsql_test(
     "alter2-6.2",
     [[
         ALTER TABLE t1 ADD CONSTRAINT ck CHECK(id > 0);
-    ]], { 1, "Constraint CK already exists" })
+    ]], { 1, "CHECK constraint 'CK' already exists within space 'T1'" })
 
 -- Make sure that CHECK constraint can be created only on empty space.
 --
diff --git a/test/sql/checks.result b/test/sql/checks.result
index a952b2b70..afd7c3d25 100644
--- a/test/sql/checks.result
+++ b/test/sql/checks.result
@@ -260,7 +260,7 @@ s:drop()
 box.execute("CREATE TABLE T2(ID INT PRIMARY KEY, CONSTRAINT CK1 CHECK(ID > 0), CONSTRAINT CK1 CHECK(ID < 0))")
 ---
 - null
-- Constraint CK1 already exists
+- CHECK constraint 'CK1' already exists within space 'T2'
 ...
 box.space.T2
 ---
diff --git a/test/sql/clear.result b/test/sql/clear.result
index afa6520e8..90070811a 100644
--- a/test/sql/clear.result
+++ b/test/sql/clear.result
@@ -177,7 +177,7 @@ box.execute("DROP TABLE zoobar")
 box.execute("CREATE TABLE t1(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 CHECK(id < 0));")
 ---
 - null
-- Constraint CK1 already exists
+- CHECK constraint 'CK1' already exists within space 'T1'
 ...
 box.space.t1
 ---
@@ -190,7 +190,7 @@ box.space._ck_constraint:select()
 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
+- FOREIGN KEY constraint 'FK1' already exists within space 'T2'
 ...
 box.space.t2
 ---
@@ -207,7 +207,7 @@ box.space._fk_constraint:select()
 box.execute("CREATE TABLE t3(id INT PRIMARY KEY, CONSTRAINT ck1 CHECK(id > 0), CONSTRAINT ck1 FOREIGN KEY(id) REFERENCES t3, CONSTRAINT fk1 FOREIGN KEY(id) REFERENCES t3, CONSTRAINT ck1 CHECK(id < 0));")
 ---
 - null
-- Constraint CK1 already exists
+- FOREIGN KEY constraint 'CK1' already exists within space 'T3'
 ...
 box.space.t1
 ---
diff --git a/test/sql/constraint.result b/test/sql/constraint.result
index a3dd7d531..7383acdac 100644
--- a/test/sql/constraint.result
+++ b/test/sql/constraint.result
@@ -26,48 +26,48 @@ box.execute([[CREATE TABLE t2 (i INT, CONSTRAINT c CHECK (i > 0),
                                CONSTRAINT c PRIMARY KEY (i));]]);
  | ---
  | - null
- | - Constraint C already exists
+ | - PRIMARY KEY constraint 'C' already exists within space 'T2'
  | ...
 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
+ | - PRIMARY KEY constraint 'C' already exists within space 'T2'
  | ...
 box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
                                CONSTRAINT c CHECK (i > 0),
                                CONSTRAINT c UNIQUE (i));]]);
  | ---
  | - null
- | - Constraint C already exists
+ | - UNIQUE constraint 'C' already exists within space 'T2'
  | ...
 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
+ | - UNIQUE constraint 'C' already exists within space 'T2'
  | ...
 box.execute([[CREATE TABLE t2 (i INT PRIMARY KEY,
                                CONSTRAINT c CHECK (i > 0),
                                CONSTRAINT c CHECK (i < 0));]]);
  | ---
  | - null
- | - Constraint C already exists
+ | - CHECK constraint 'C' already exists within space 'T2'
  | ...
 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
+ | - FOREIGN KEY constraint 'C' already exists within space 'T2'
  | ...
 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
+ | - FOREIGN KEY constraint 'C' already exists within space 'T2'
  | ...
 test_run:cmd("setopt delimiter ''");
  | ---
@@ -85,7 +85,7 @@ box.execute('CREATE TABLE t2 (i INT CONSTRAINT c PRIMARY KEY);')
 box.execute('ALTER TABLE t2 ADD CONSTRAINT c CHECK(i > 0);')
  | ---
  | - null
- | - Constraint C already exists
+ | - PRIMARY KEY constraint 'C' already exists within space 'T2'
  | ...
 box.execute('ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);')
  | ---
@@ -95,7 +95,7 @@ box.execute('ALTER TABLE t2 ADD CONSTRAINT c UNIQUE(i);')
 box.execute('ALTER TABLE t2 ADD CONSTRAINT c FOREIGN KEY(i) REFERENCES t1(i);')
  | ---
  | - null
- | - Constraint C already exists
+ | - PRIMARY KEY constraint 'C' already exists within space 'T2'
  | ...
 
 --
@@ -162,7 +162,7 @@ box.space.T2.index.D:alter({name = 'E'})
 box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
  | ---
  | - null
- | - Constraint E already exists
+ | - UNIQUE constraint 'E' already exists within space 'T2'
  | ...
 
 --
@@ -178,7 +178,7 @@ box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
  | ...
 box.space.T2.index.E:alter({unique = true})
  | ---
- | - error: Constraint E already exists
+ | - error: CHECK constraint 'E' already exists within space 'T2'
  | ...
 box.space.T2.ck_constraint.E:drop()
  | ---
@@ -189,7 +189,7 @@ box.space.T2.index.E:alter({unique = true})
 box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);')
  | ---
  | - null
- | - Constraint E already exists
+ | - UNIQUE constraint 'E' already exists within space 'T2'
  | ...
 
 -- Alter name and uniqueness of an unique index simultaneously.