From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng1.m.smailru.net (smtpng1.m.smailru.net [94.100.181.251]) (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 CC91B46970E for ; Fri, 24 Jan 2020 17:21:37 +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: <20200113170045.GD7851@tarantool.org> Date: Fri, 24 Jan 2020 17:21:36 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: <17741BF6-ADCA-4B87-BD6C-955DAF009EC7@tarantool.org> References: <20200109101502.4158-1-roman.habibov@tarantool.org> <20200113170045.GD7851@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH 2/2] sql: support constraint drop List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikita Pettik Cc: tarantool-patches@dev.tarantool.org Hi! Thanks for the review. > On Jan 13, 2020, at 20:00, Nikita Pettik = wrote: >=20 > On 09 Jan 13:15, Roman Khabibov wrote: >> diff --git a/src/box/errcode.h b/src/box/errcode.h >> index 3065b1948..9ba42dfb4 100644 >> --- a/src/box/errcode.h >> +++ b/src/box/errcode.h >> @@ -221,7 +221,7 @@ struct errcode_record { >> /*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not = exist") \ >> /*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") \ >> + /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does = not exist in space '%s'") \ >=20 > Why did you change error (in scope of this patch)? In the previous patch about constraint_id, I did about the same with = ER_CONSTRAINT_EXISTS. Now I decided that changing the error also applies to this patch and it = is too small to be a separate patch. >> diff --git a/src/box/sql/build.c b/src/box/sql/build.c >> index bc50ecbfa..7c4b5760e 100644 >> --- a/src/box/sql/build.c >> +++ b/src/box/sql/build.c >> @@ -1469,6 +1469,29 @@ vdbe_emit_stat_space_clear(struct Parse = *parse, const char *stat_table_name, >> sql_table_delete_from(parse, src_list, where); >> } >>=20 >> +/** >> + * Generate VDBE program to remove entry with @a index_id and @a >> + * space_id from _index space. >> + */ >> +static void >> +vdbe_emit_index_drop(struct Parse *parse_context, uint32_t index_id, >> + uint32_t space_id) >> +{ >=20 > Instead of passing values of space/index ids, I'd rather pass = registers > holding these values. In most cases it allows to avoid duplicating > the same intstruction (which stores id in the given register). Done. In the separate patch. >> + struct Vdbe *v =3D sqlGetVdbe(parse_context); >> + assert(v !=3D NULL); >> + int record_reg =3D ++parse_context->nMem; >> + int space_id_reg =3D ++parse_context->nMem; >> + int index_id_reg =3D ++parse_context->nMem; >> + sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg); >> + sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg); >> + sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg); >> + sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg); >> + if (index_id =3D=3D 0) >> + VdbeComment((v, "Remove primary key", index_id)); >> + else >> + VdbeComment((v, "Remove secondary index iid =3D %u", = index_id)); >=20 > Simply without branching: "Remove index with iid =3D %u=E2=80=9D Done. >> +} >> + >> /** >> * Generate VDBE program to remove entry from _fk_constraint space. >> * >> @@ -1488,17 +1511,6 @@ vdbe_emit_fk_constraint_drop(struct Parse = *parse_context, char *constraint_name, >> sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name, >> P4_DYNAMIC); >> sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1); >> - const char *error_msg =3D >> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), >> - constraint_name); >> - if (vdbe_emit_halt_with_presence_test(parse_context, >> - BOX_FK_CONSTRAINT_ID, 0, >> - key_reg, 2, = ER_NO_SUCH_CONSTRAINT, >> - error_msg, false, >> - OP_Found) !=3D 0) { >> - sqlDbFree(parse_context->db, constraint_name); >> - return; >> - } >=20 > Why did you drop this check? >=20 >> sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2); >> sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + = 2); >> VdbeComment((vdbe, "Delete FK constraint %s", constraint_name)); >> @@ -1523,12 +1535,6 @@ vdbe_emit_ck_constraint_drop(struct Parse = *parser, const char *ck_name, >> sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg); >> sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0, >> sqlDbStrDup(db, ck_name), P4_DYNAMIC); >> - const char *error_msg =3D >> - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), = ck_name); >> - if (vdbe_emit_halt_with_presence_test(parser, = BOX_CK_CONSTRAINT_ID, 0, >> - key_reg, 2, = ER_NO_SUCH_CONSTRAINT, >> - error_msg, false, = OP_Found) !=3D 0) >> - return; >=20 > Same question. I dropped these checks, because I thought that the data are consistent. = These two functions are called when: 1) DROP TABLE. If the constraint exist in struct space, then the = corresponding tuple exists in _ck/_fk system space too. Therefore this error can not occur = in box. 2) ALTER TABLE DROP CONSTRAINT. Analogically, I check the constraint for = existence in sql_drop_constraint() and throw error on parsing level. >> sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2); >> sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2); >> VdbeComment((v, "Delete CK constraint %s", ck_name)); >> @@ -1617,7 +1623,6 @@ sql_code_drop_table(struct Parse = *parse_context, struct space *space, >> */ >> int idx_rec_reg =3D ++parse_context->nMem; >> int space_id_reg =3D ++parse_context->nMem; >> - int index_id_reg =3D ++parse_context->nMem; >> int space_id =3D space->def->id; >> sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg); >> sqlVdbeAddOp1(v, OP_CheckViewReferences, space_id_reg); >> @@ -1680,23 +1685,12 @@ sql_code_drop_table(struct Parse = *parse_context, struct space *space, >> * secondary exist. >> */ >> for (uint32_t i =3D 1; i < index_count; ++i) { >> - sqlVdbeAddOp2(v, OP_Integer, >> - = space->index[i]->def->iid, >> - index_id_reg); >> - sqlVdbeAddOp3(v, OP_MakeRecord, >> - space_id_reg, 2, = idx_rec_reg); >> - sqlVdbeAddOp2(v, OP_SDelete, = BOX_INDEX_ID, >> - idx_rec_reg); >> - VdbeComment((v, >> - "Remove secondary index iid = =3D %u", >> - = space->index[i]->def->iid)); >> + vdbe_emit_index_drop(parse_context, >> + = space->index[i]->def->iid, >> + space_id); >=20 > I'd better move this refactoring to a separate patch. Done. >> } >> } >> - sqlVdbeAddOp2(v, OP_Integer, 0, index_id_reg); >> - sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, >> - idx_rec_reg); >> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, idx_rec_reg); >> - VdbeComment((v, "Remove primary index")); >> + vdbe_emit_index_drop(parse_context, 0, space_id); >> } >> /* Delete records about the space from the _truncate. */ >> sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 1, idx_rec_reg); >> @@ -2050,28 +2044,61 @@ fk_constraint_change_defer_mode(struct Parse = *parse_context, bool is_deferred) >> is_deferred; >> } >>=20 >> +/** >> + * Emit code to drop the entry from _index or _ck_contstraint or >> + * _fk_constraint space corresponding with the constraint type. >> + */ >> void >> -sql_drop_foreign_key(struct Parse *parse_context) >> +sql_drop_constraint(struct Parse *parse_context) >> { >> - struct drop_entity_def *drop_def =3D = &parse_context->drop_fk_def.base; >> - assert(drop_def->base.entity_type =3D=3D ENTITY_TYPE_FK); >> + struct drop_entity_def *drop_def =3D >> + &parse_context->drop_constraint_def.base; >> + assert(drop_def->base.entity_type =3D=3D = ENTITY_TYPE_CONSTRAINT); >> assert(drop_def->base.alter_action =3D=3D ALTER_ACTION_DROP); >> const char *table_name =3D = drop_def->base.entity_name->a[0].zName; >> assert(table_name !=3D NULL); >> - struct space *child =3D space_by_name(table_name); >> - if (child =3D=3D NULL) { >> + struct space *space =3D space_by_name(table_name); >> + if (space =3D=3D NULL) { >> diag_set(ClientError, ER_NO_SUCH_SPACE, table_name); >> parse_context->is_aborted =3D true; >> return; >> } >> - char *constraint_name =3D >> - sql_name_from_token(parse_context->db, &drop_def->name); >> - if (constraint_name =3D=3D NULL) { >> + char *name =3D sql_name_from_token(parse_context->db, = &drop_def->name); >> + if (name =3D=3D NULL) { >> + parse_context->is_aborted =3D true; >> + return; >> + } >> + struct constraint_id *id =3D space_find_constraint_id(space, = name); >> + if (id =3D=3D NULL) { >> + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, = table_name); >> parse_context->is_aborted =3D true; >> return; >> } >> - vdbe_emit_fk_constraint_drop(parse_context, constraint_name, >> - child->def->id); >> + uint32_t space_id =3D space->def->id; >> + assert(id->type =3D=3D CONSTRAINT_TYPE_PK || id->type =3D=3D >> + CONSTRAINT_TYPE_UNIQUE || id->type =3D=3D = CONSTRAINT_TYPE_FK || >> + id->type =3D=3D CONSTRAINT_TYPE_CK); >=20 > Instead of such huge checks iterating over all possible variants, > enum can be extended with _MAX last element (see enum index_type for > example) and check becomes quite simple: assert(id->type < = contraint_type_MAX); Ok, but I thought that this assert is needed to prevent the following = situation. For example, I have added new value to enum constraint_type, and I = forgot to extend this switch-case construction. Then I will get unreachable(). >> + switch (id->type) { >> + case CONSTRAINT_TYPE_PK: >> + case CONSTRAINT_TYPE_UNIQUE: { >> + uint32_t index_id =3D box_index_id_by_name(space_id, = name, >> + strlen(name)); >> + /* >> + * We have already sure, that this index exists, >> + * so we don't check index_id for BOX_ID_NIL. >> + */ >=20 > Then add assert(index_id !=3D BOX_ID_NIL); >> + vdbe_emit_index_drop(parse_context, index_id, space_id); >> + break; >> + } >> + case CONSTRAINT_TYPE_FK: >> + vdbe_emit_fk_constraint_drop(parse_context, name, = space_id); >> + break; >> + case CONSTRAINT_TYPE_CK: >> + vdbe_emit_ck_constraint_drop(parse_context, name, = space_id); >> + break; >> + default: >> + unreachable(); >> + } >> /* >> * We account changes to row count only if drop of >> * foreign keys take place in a separate +/** + * Emit code to drop the entry from _index or _ck_contstraint or + * _fk_constraint space corresponding with the constraint type. + */ void -sql_drop_foreign_key(struct Parse *parse_context) +sql_drop_constraint(struct Parse *parse) { - struct drop_entity_def *drop_def =3D = &parse_context->drop_fk_def.base; - assert(drop_def->base.entity_type =3D=3D ENTITY_TYPE_FK); + struct drop_entity_def *drop_def =3D = &parse->drop_constraint_def.base; + assert(drop_def->base.entity_type =3D=3D = ENTITY_TYPE_CONSTRAINT); assert(drop_def->base.alter_action =3D=3D ALTER_ACTION_DROP); const char *table_name =3D = drop_def->base.entity_name->a[0].zName; assert(table_name !=3D NULL); - struct space *child =3D space_by_name(table_name); - if (child =3D=3D NULL) { + struct space *space =3D space_by_name(table_name); + if (space =3D=3D NULL) { diag_set(ClientError, ER_NO_SUCH_SPACE, table_name); - parse_context->is_aborted =3D true; + parse->is_aborted =3D true; return; } - char *constraint_name =3D - sql_name_from_token(parse_context->db, &drop_def->name); - if (constraint_name =3D=3D NULL) { - parse_context->is_aborted =3D true; + char *name =3D sql_name_from_token(parse->db, &drop_def->name); + if (name =3D=3D NULL) { + parse->is_aborted =3D true; return; } - vdbe_emit_fk_constraint_drop(parse_context, constraint_name, - child->def->id); + struct constraint_id *id =3D space_find_constraint_id(space, = name); + if (id =3D=3D NULL) { + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, = table_name); + parse->is_aborted =3D true; + return; + } + uint32_t space_id =3D space->def->id; + assert(id->type < constraint_type_MAX); + switch (id->type) { + case CONSTRAINT_TYPE_PK: + case CONSTRAINT_TYPE_UNIQUE: { + uint32_t index_id =3D box_index_id_by_name(space_id, = name, + strlen(name)); + /* + * We have already sure, that this index exists, + * so we don't check index_id for BOX_ID_NIL. + */ + assert(index_id !=3D BOX_ID_NIL); + int space_id_reg =3D ++parse->nMem; + int index_id_reg =3D ++parse->nMem; + struct Vdbe *v =3D sqlGetVdbe(parse); + assert(v !=3D NULL); + sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg); + vdbe_emit_index_drop(parse, index_id, index_id_reg, + space_id_reg); + break; + } + case CONSTRAINT_TYPE_FK: + vdbe_emit_fk_constraint_drop(parse, name, space_id); + break; + case CONSTRAINT_TYPE_CK: + vdbe_emit_ck_constraint_drop(parse, name, space_id); + break; + default: + unreachable(); + } /* * We account changes to row count only if drop of - * foreign keys take place in a separate - * ALTER TABLE DROP CONSTRAINT statement, since whole - * DROP TABLE always returns 1 (one) as a row count. + * constraints take place in a separate ALTER TABLE DROP + * CONSTRAINT statement, since whole DROP TABLE always + * returns 1 (one) as a row count. */ - struct Vdbe *v =3D sqlGetVdbe(parse_context); + struct Vdbe *v =3D sqlGetVdbe(parse); sqlVdbeCountChanges(v); sqlVdbeChangeP5(v, OPFLAG_NCHANGE); } >> @@ -2704,18 +2731,8 @@ sql_drop_index(struct Parse *parse_context) >> goto exit_drop_index; >> } >>=20 >> - /* >> - * Generate code to remove entry from _index space >> - * But firstly, delete statistics since schema >> - * changes after DDL. >> - */ >> - int record_reg =3D ++parse_context->nMem; >> - int space_id_reg =3D ++parse_context->nMem; >> - int index_id_reg =3D ++parse_context->nMem; >> - sqlVdbeAddOp2(v, OP_Integer, space->def->id, space_id_reg); >> - sqlVdbeAddOp2(v, OP_Integer, index_id, index_id_reg); >> - sqlVdbeAddOp3(v, OP_MakeRecord, space_id_reg, 2, record_reg); >> - sqlVdbeAddOp2(v, OP_SDelete, BOX_INDEX_ID, record_reg); >> + vdbe_emit_index_drop(parse_context, index_id, space->def->id); >> + /* Delete statistics since schema changes after DDL. */ >=20 > Looks like obsolete comment. Removed. >> diff --git a/test/sql/constraint.test.lua = b/test/sql/constraint.test.lua >> index 163f4309c..0cada5f09 100755 >> --- a/test/sql/constraint.test.lua >> +++ b/test/sql/constraint.test.lua >> @@ -131,6 +131,43 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);') >> -- uniqueness, index names should be unique in a space. >> box.execute('CREATE UNIQUE INDEX d ON t2(i);') >>=20 >> +-- >> +-- gh-4120: Ensure that works >> +-- correctly for each type of constraint. >> +-- >> +-- Drop PRIMARY KEY constraint. >> +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;') >> +box.space.T2.index.C ~=3D nil >=20 > But PK is not dropped. Please, add tests where PK will be successfully > dropped. +-- +-- gh-4120: Ensure that works +-- correctly for each type of constraint. +-- +-- Drop UNIQUE constraint. +box.space.T2.index.E ~=3D nil +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') +box.space.T2.index.E =3D=3D nil +-- Drop PRIMARY KEY constraint named "C". +box.execute('DROP INDEX d ON t2;') +box.space.T2.index.C ~=3D nil +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;') +box.space.T2.index.C =3D=3D nil +-- Drop CHECK constraint. +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') +box.space.T2.ck_constraint.E ~=3D nil +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') +box.space.T2.ck_constraint.E =3D=3D nil +-- Drop FOREIGN KEY constraint. +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES = t1(i);') +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') +-- Check the error message. +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') + >> +-- Drop UNIQUE constraint. >> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') >> +box.space.T2.index.E =3D=3D nil >> +-- Drop CHECK constraint. >> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') >> +box.space.T2.ck_constraint.E ~=3D nil >> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') >> +box.space.T2.ck_constraint.E =3D=3D nil >> +-- Drop FOREIGN KEY constraint. >> +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) = REFERENCES t1(i);') >> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') >> +-- Check the error message. >> +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') >> + >> +-- >> +-- Ensure that constraint name dropped from/added into the >> +-- internal space hash table during a transaction. >> +-- >=20 > This patch is about extending SQL syntax, don't get how this check > is related to subj.. I mean to check that this operation is transactional. Removed. commit 2220d8f4e09421e88d05a9d5265f6aca0fd30916 Author: Roman Khabibov Date: Thu Jan 2 20:13:36 2020 +0300 sql: support constraint drop =20 Extend statement to drop table constraints by their names. =20 Closes #4120 =20 @TarantoolBot document Title: Drop table constraints in SQL Now, it is possible to drop table constraints (PRIMARY KEY, UNIQUE, FOREIGN KEY, CHECK) using statement by their names. =20 For example: =20 tarantool> box.execute([[CREATE TABLE test ( a INTEGER PRIMARY KEY, b INTEGER, CONSTRAINT cnstr CHECK (a >=3D 0) );]]) --- - row_count: 1 ... =20 tarantool> box.execute('ALTER TABLE test DROP CONSTRAINT cnstr;') --- - row_count: 1 ... =20 The same for all the other constraints. diff --git a/src/box/constraint_id.h b/src/box/constraint_id.h index 21f067cdc..ed4f37426 100755 --- a/src/box/constraint_id.h +++ b/src/box/constraint_id.h @@ -40,6 +40,7 @@ enum constraint_type { CONSTRAINT_TYPE_UNIQUE, CONSTRAINT_TYPE_FK, CONSTRAINT_TYPE_CK, + constraint_type_MAX, }; =20 extern const char *constraint_type_strs[]; diff --git a/src/box/errcode.h b/src/box/errcode.h index 6f6e28c6c..b7787d378 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -221,7 +221,7 @@ struct errcode_record { /*166 */_(ER_NO_SUCH_COLLATION, "Collation '%s' does not = exist") \ /*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") \ + /*169 */_(ER_NO_SUCH_CONSTRAINT, "Constraint '%s' does = not exist in space '%s'") \ /*170 */_(ER_CONSTRAINT_EXISTS, "%s constraint '%s' = already exists in 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") \ diff --git a/src/box/sql/alter.c b/src/box/sql/alter.c index 973b420cf..14f6c1a97 100644 --- a/src/box/sql/alter.c +++ b/src/box/sql/alter.c @@ -121,7 +121,7 @@ sql_alter_ck_constraint_enable(struct Parse *parse) int addr =3D sqlVdbeAddOp4Int(v, OP_Found, cursor, 0, key_reg, = 2); sqlVdbeAddOp4(v, OP_SetDiag, ER_NO_SUCH_CONSTRAINT, 0, 0, sqlMPrintf(db, = tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), - constraint_name), P4_DYNAMIC); + constraint_name, tbl_name), = P4_DYNAMIC); sqlVdbeAddOp2(v, OP_Halt, -1, ON_CONFLICT_ACTION_ABORT); sqlVdbeJumpHere(v, addr); =20 diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 58a6a8a6b..1a695efa4 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -1508,17 +1508,6 @@ vdbe_emit_fk_constraint_drop(struct Parse = *parse_context, char *constraint_name, sqlVdbeAddOp4(vdbe, OP_String8, 0, key_reg, 0, constraint_name, P4_DYNAMIC); sqlVdbeAddOp2(vdbe, OP_Integer, child_id, key_reg + 1); - const char *error_msg =3D - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), - constraint_name); - if (vdbe_emit_halt_with_presence_test(parse_context, - BOX_FK_CONSTRAINT_ID, 0, - key_reg, 2, = ER_NO_SUCH_CONSTRAINT, - error_msg, false, - OP_Found) !=3D 0) { - sqlDbFree(parse_context->db, constraint_name); - return; - } sqlVdbeAddOp3(vdbe, OP_MakeRecord, key_reg, 2, key_reg + 2); sqlVdbeAddOp2(vdbe, OP_SDelete, BOX_FK_CONSTRAINT_ID, key_reg + = 2); VdbeComment((vdbe, "Delete FK constraint %s", constraint_name)); @@ -1543,12 +1532,6 @@ vdbe_emit_ck_constraint_drop(struct Parse = *parser, const char *ck_name, sqlVdbeAddOp2(v, OP_Integer, space_id, key_reg); sqlVdbeAddOp4(v, OP_String8, 0, key_reg + 1, 0, sqlDbStrDup(db, ck_name), P4_DYNAMIC); - const char *error_msg =3D - tt_sprintf(tnt_errcode_desc(ER_NO_SUCH_CONSTRAINT), = ck_name); - if (vdbe_emit_halt_with_presence_test(parser, = BOX_CK_CONSTRAINT_ID, 0, - key_reg, 2, = ER_NO_SUCH_CONSTRAINT, - error_msg, false, = OP_Found) !=3D 0) - return; sqlVdbeAddOp3(v, OP_MakeRecord, key_reg, 2, key_reg + 2); sqlVdbeAddOp2(v, OP_SDelete, BOX_CK_CONSTRAINT_ID, key_reg + 2); VdbeComment((v, "Delete CK constraint %s", ck_name)); @@ -2061,35 +2044,72 @@ fk_constraint_change_defer_mode(struct Parse = *parse_context, bool is_deferred) is_deferred; } =20 +/** + * Emit code to drop the entry from _index or _ck_contstraint or + * _fk_constraint space corresponding with the constraint type. + */ void -sql_drop_foreign_key(struct Parse *parse_context) +sql_drop_constraint(struct Parse *parse) { - struct drop_entity_def *drop_def =3D = &parse_context->drop_fk_def.base; - assert(drop_def->base.entity_type =3D=3D ENTITY_TYPE_FK); + struct drop_entity_def *drop_def =3D = &parse->drop_constraint_def.base; + assert(drop_def->base.entity_type =3D=3D = ENTITY_TYPE_CONSTRAINT); assert(drop_def->base.alter_action =3D=3D ALTER_ACTION_DROP); const char *table_name =3D = drop_def->base.entity_name->a[0].zName; assert(table_name !=3D NULL); - struct space *child =3D space_by_name(table_name); - if (child =3D=3D NULL) { + struct space *space =3D space_by_name(table_name); + if (space =3D=3D NULL) { diag_set(ClientError, ER_NO_SUCH_SPACE, table_name); - parse_context->is_aborted =3D true; + parse->is_aborted =3D true; return; } - char *constraint_name =3D - sql_name_from_token(parse_context->db, &drop_def->name); - if (constraint_name =3D=3D NULL) { - parse_context->is_aborted =3D true; + char *name =3D sql_name_from_token(parse->db, &drop_def->name); + if (name =3D=3D NULL) { + parse->is_aborted =3D true; return; } - vdbe_emit_fk_constraint_drop(parse_context, constraint_name, - child->def->id); + struct constraint_id *id =3D space_find_constraint_id(space, = name); + if (id =3D=3D NULL) { + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, = table_name); + parse->is_aborted =3D true; + return; + } + uint32_t space_id =3D space->def->id; + assert(id->type < constraint_type_MAX); + switch (id->type) { + case CONSTRAINT_TYPE_PK: + case CONSTRAINT_TYPE_UNIQUE: { + uint32_t index_id =3D box_index_id_by_name(space_id, = name, + strlen(name)); + /* + * We have already sure, that this index exists, + * so we don't check index_id for BOX_ID_NIL. + */ + assert(index_id !=3D BOX_ID_NIL); + int space_id_reg =3D ++parse->nMem; + int index_id_reg =3D ++parse->nMem; + struct Vdbe *v =3D sqlGetVdbe(parse); + assert(v !=3D NULL); + sqlVdbeAddOp2(v, OP_Integer, space_id, space_id_reg); + vdbe_emit_index_drop(parse, index_id, index_id_reg, + space_id_reg); + break; + } + case CONSTRAINT_TYPE_FK: + vdbe_emit_fk_constraint_drop(parse, name, space_id); + break; + case CONSTRAINT_TYPE_CK: + vdbe_emit_ck_constraint_drop(parse, name, space_id); + break; + default: + unreachable(); + } /* * We account changes to row count only if drop of - * foreign keys take place in a separate - * ALTER TABLE DROP CONSTRAINT statement, since whole - * DROP TABLE always returns 1 (one) as a row count. + * constraints take place in a separate ALTER TABLE DROP + * CONSTRAINT statement, since whole DROP TABLE always + * returns 1 (one) as a row count. */ - struct Vdbe *v =3D sqlGetVdbe(parse_context); + struct Vdbe *v =3D sqlGetVdbe(parse); sqlVdbeCountChanges(v); sqlVdbeChangeP5(v, OPFLAG_NCHANGE); } diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index cfe1c0012..1a0e89703 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -1763,9 +1763,9 @@ cmd ::=3D alter_table_start(A) RENAME TO nm(N). { } =20 cmd ::=3D ALTER TABLE fullname(X) DROP CONSTRAINT nm(Z). { - drop_fk_def_init(&pParse->drop_fk_def, X, &Z, false); + drop_constraint_def_init(&pParse->drop_constraint_def, X, &Z, false); pParse->initiateTTrans =3D true; - sql_drop_foreign_key(pParse); + sql_drop_constraint(pParse); } =20 cmd ::=3D alter_table_start(A) enable(E) CHECK CONSTRAINT nm(Z). { diff --git a/src/box/sql/parse_def.h b/src/box/sql/parse_def.h index 2f433e4c0..e6d45b256 100644 --- a/src/box/sql/parse_def.h +++ b/src/box/sql/parse_def.h @@ -159,10 +159,6 @@ enum entity_type { ENTITY_TYPE_TRIGGER, ENTITY_TYPE_CK, ENTITY_TYPE_FK, - /** - * For assertion checks that constraint definition is - * created before initialization of a term constraint. - */ ENTITY_TYPE_CONSTRAINT, }; =20 @@ -265,7 +261,7 @@ struct drop_trigger_def { struct drop_entity_def base; }; =20 -struct drop_fk_def { +struct drop_constraint_def { struct drop_entity_def base; }; =20 @@ -408,11 +404,12 @@ drop_trigger_def_init(struct drop_trigger_def = *drop_trigger_def, } =20 static inline void -drop_fk_def_init(struct drop_fk_def *drop_fk_def, struct SrcList = *parent_name, - struct Token *name, bool if_exist) +drop_constraint_def_init(struct drop_constraint_def = *drop_constraint_def, + struct SrcList *parent_name, struct Token = *name, + bool if_exist) { - drop_entity_def_init(&drop_fk_def->base, parent_name, name, = if_exist, - ENTITY_TYPE_FK); + drop_entity_def_init(&drop_constraint_def->base, parent_name, = name, + if_exist, ENTITY_TYPE_CONSTRAINT); } =20 static inline void diff --git a/src/box/sql/sqlInt.h b/src/box/sql/sqlInt.h index d1fcf4761..b1d4b913a 100644 --- a/src/box/sql/sqlInt.h +++ b/src/box/sql/sqlInt.h @@ -2237,7 +2237,7 @@ struct Parse { struct create_trigger_def create_trigger_def; struct create_view_def create_view_def; struct rename_entity_def rename_entity_def; - struct drop_fk_def drop_fk_def; + struct drop_constraint_def drop_constraint_def; struct drop_index_def drop_index_def; struct drop_table_def drop_table_def; struct drop_trigger_def drop_trigger_def; @@ -3743,11 +3743,9 @@ sql_create_foreign_key(struct Parse = *parse_context); /** * Function called from parser to handle * SQL statement. - * - * @param parse_context Parsing context. */ void -sql_drop_foreign_key(struct Parse *parse_context); +sql_drop_constraint(struct Parse *parse); =20 /** * Now our SQL implementation can't operate on spaces which diff --git a/test/sql-tap/alter2.test.lua b/test/sql-tap/alter2.test.lua index 3e21a5f40..759acc9c3 100755 --- a/test/sql-tap/alter2.test.lua +++ b/test/sql-tap/alter2.test.lua @@ -256,7 +256,7 @@ test:do_catchsql_test( ALTER TABLE child DROP CONSTRAINT fake; ]], { -- - 1, "Constraint FAKE does not exist" + 1, "Constraint 'FAKE' does not exist in space 'CHILD'" -- }) =20 diff --git a/test/sql/checks.result b/test/sql/checks.result index 4ae0b4c10..7b18e5d6b 100644 --- a/test/sql/checks.result +++ b/test/sql/checks.result @@ -801,7 +801,7 @@ box.execute('ALTER TABLE test ADD CONSTRAINT = \"some_ck\" CHECK(a < 10);') box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"falsch\"") --- - null -- Constraint falsch does not exist +- Constraint 'falsch' does not exist in space 'TEST' ... box.execute("ALTER TABLE test DISABLE CHECK CONSTRAINT \"some_ck\"") --- diff --git a/test/sql/constraint.result b/test/sql/constraint.result index 1585c2327..e8b8f382c 100644 --- a/test/sql/constraint.result +++ b/test/sql/constraint.result @@ -291,6 +291,73 @@ box.execute('CREATE UNIQUE INDEX d ON t2(i);') | - Index 'D' already exists in space 'T2' | ... =20 +-- +-- gh-4120: Ensure that works +-- correctly for each type of constraint. +-- +-- Drop UNIQUE constraint. +box.space.T2.index.E ~=3D nil + | --- + | - true + | ... +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') + | --- + | - row_count: 1 + | ... +box.space.T2.index.E =3D=3D nil + | --- + | - true + | ... +-- Drop PRIMARY KEY constraint named "C". +box.execute('DROP INDEX d ON t2;') + | --- + | - row_count: 1 + | ... +box.space.T2.index.C ~=3D nil + | --- + | - true + | ... +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;') + | --- + | - row_count: 1 + | ... +box.space.T2.index.C =3D=3D nil + | --- + | - true + | ... +-- Drop CHECK constraint. +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') + | --- + | - row_count: 1 + | ... +box.space.T2.ck_constraint.E ~=3D nil + | --- + | - true + | ... +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') + | --- + | - row_count: 1 + | ... +box.space.T2.ck_constraint.E =3D=3D nil + | --- + | - true + | ... +-- Drop FOREIGN KEY constraint. +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES = t1(i);') + | --- + | - row_count: 1 + | ... +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') + | --- + | - row_count: 1 + | ... +-- Check the error message. +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') + | --- + | - null + | - Constraint 'E' does not exist in space 'T2' + | ... + -- -- Cleanup. -- diff --git a/test/sql/constraint.test.lua b/test/sql/constraint.test.lua index 163f4309c..ef2cdb8cc 100755 --- a/test/sql/constraint.test.lua +++ b/test/sql/constraint.test.lua @@ -131,6 +131,30 @@ box.execute('CREATE UNIQUE INDEX e ON t2(i);') -- uniqueness, index names should be unique in a space. box.execute('CREATE UNIQUE INDEX d ON t2(i);') =20 +-- +-- gh-4120: Ensure that works +-- correctly for each type of constraint. +-- +-- Drop UNIQUE constraint. +box.space.T2.index.E ~=3D nil +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') +box.space.T2.index.E =3D=3D nil +-- Drop PRIMARY KEY constraint named "C". +box.execute('DROP INDEX d ON t2;') +box.space.T2.index.C ~=3D nil +box.execute('ALTER TABLE t2 DROP CONSTRAINT c;') +box.space.T2.index.C =3D=3D nil +-- Drop CHECK constraint. +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') +box.space.T2.ck_constraint.E ~=3D nil +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') +box.space.T2.ck_constraint.E =3D=3D nil +-- Drop FOREIGN KEY constraint. +box.execute('ALTER TABLE t2 ADD CONSTRAINT e FOREIGN KEY(i) REFERENCES = t1(i);') +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') +-- Check the error message. +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') + -- -- Cleanup. --