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.
next prev parent 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