From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng2.m.smailru.net (smtpng2.m.smailru.net [94.100.179.3]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 25E0846971A for ; Thu, 5 Dec 2019 21:59:35 +0300 (MSK) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 13.0 \(3594.4.19\)) From: Roman Khabibov In-Reply-To: <20191204175450.GD19235@atlas> Date: Thu, 5 Dec 2019 21:59:33 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <0218889f41f954a10bbe36c6d0c43f32fa7dd4b4.1575468493.git.roman.habibov@tarantool.org> <20191204175450.GD19235@atlas> Subject: Re: [Tarantool-patches] [PATCH v2 3/3] box: let only box handle constraint dup errors List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vladislav Shpilevoy Cc: Konstantin Osipov , tarantool-patches@dev.tarantool.org > On Dec 4, 2019, at 20:54, Konstantin Osipov = wrote: >=20 > * Roman Khabibov [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) >> +{ >=20 > Looks like it belongs to space.h/c >=20 >=20 > --=20 > Konstantin Osipov, Moscow, Russia Vlad, please, don=E2=80=99t 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=E2=80=99s listen to Vlad=E2=80=99s opinion, then = we=E2=80=99ll decide. commit e12c95007f465c98646206d4e83a620b0d2aff36 Author: Roman Khabibov Date: Wed Dec 4 12:38:29 2019 +0300 box: let only box handle constraint dup errors =20 Improve error message about constraint name duplicate and remove it from sql. =20 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; } =20 +/** + * 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 =3D=3D CONSTRAINT_TYPE_PK || def->type =3D=3D + CONSTRAINT_TYPE_UNIQUE || def->type =3D=3D = CONSTRAINT_TYPE_FK || + def->type =3D=3D 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 =3D + space_constraint_def_by_name(space, name); + if (def !=3D 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 =3D alter->old_space; - if (space_constraint_def_by_name(space, def->name) !=3D NULL) { - diag_set(ClientError, ER_CONSTRAINT_EXISTS, def->name); + if (space_constraint_exists(space, def->name)) return -1; - } struct constraint_def *constr_def =3D constraint_def_new(space->def->id, def->iid =3D=3D 0 ? CONSTRAINT_TYPE_PK : = CONSTRAINT_TYPE_UNIQUE, @@ -5391,13 +5436,8 @@ on_replace_dd_fk_constraint(struct trigger * /* = trigger*/, void *event) fk->def =3D fk_def; fk->index_id =3D fk_index->def->iid; if (old_tuple =3D=3D NULL) { - if (space_constraint_def_by_name(child_space, - fk_def->name) - !=3D 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 =3D make_scoped_guard([=3D] { ck_constraint_delete(new_ck_constraint); }); - if (old_ck_constraint !=3D NULL) { + if (old_ck_constraint !=3D NULL) rlist_del_entry(old_ck_constraint, link); - } else if (space_constraint_def_by_name(space, name) !=3D = 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) !=3D= 0) return -1; ck_guard.is_active =3D 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 =3D - 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) !=3D = 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 =3D - 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) !=3D = 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; ]], { -- - 1, "Constraint FK already exists" + 1, "Duplicate key exists in unique index 'primary' in space = '_fk_constraint'" -- }) =20 @@ -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'" }) =20 -- 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' | ... =20 -- @@ -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' | ... =20 -- @@ -150,7 +150,7 @@ box.space.T2.index.D:alter({name =3D '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' | ... =20 -- @@ -166,7 +166,7 @@ box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i = > 0);'); | ... box.space.T2.index.E:alter({unique =3D 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 =3D 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' | ... =20 -- Alter name and uniqueness of an unique index simultaneously.