Tarantool development patches archive
 help / color / mirror / Atom feed
From: Roman Khabibov <roman.habibov@tarantool.org>
To: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
Cc: Konstantin Osipov <kostja@tarantool.org>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors
Date: Thu, 5 Dec 2019 21:59:33 +0300	[thread overview]
Message-ID: <B4DA7969-21CC-43DB-B79B-601198877C2A@tarantool.org> (raw)
In-Reply-To: <20191204175450.GD19235@atlas>



> 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.

  reply	other threads:[~2019-12-05 18:59 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <cover.1575468493.git.roman.habibov@tarantool.org>
2019-12-04 16:23 ` Roman Khabibov
2019-12-04 16:32   ` Cyrill Gorcunov
2019-12-04 17:54   ` Konstantin Osipov
2019-12-05 18:59     ` Roman Khabibov [this message]
2019-12-05 19:13       ` Cyrill Gorcunov
2019-12-07 16:35   ` Vladislav Shpilevoy
2019-12-10 12:49     ` Roman Khabibov

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=B4DA7969-21CC-43DB-B79B-601198877C2A@tarantool.org \
    --to=roman.habibov@tarantool.org \
    --cc=kostja@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox