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 667EE46970E for ; Mon, 13 Jan 2020 20:00:47 +0300 (MSK) Date: Mon, 13 Jan 2020 20:00:45 +0300 From: Nikita Pettik Message-ID: <20200113170045.GD7851@tarantool.org> References: <20200109101502.4158-1-roman.habibov@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20200109101502.4158-1-roman.habibov@tarantool.org> Subject: Re: [Tarantool-patches] [PATCH] sql: support constraint drop List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org 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'") \ Why did you change error (in scope of this 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); > } > > +/** > + * 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) > +{ 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). > + struct Vdbe *v = sqlGetVdbe(parse_context); > + assert(v != NULL); > + int record_reg = ++parse_context->nMem; > + int space_id_reg = ++parse_context->nMem; > + int index_id_reg = ++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 == 0) > + VdbeComment((v, "Remove primary key", index_id)); > + else > + VdbeComment((v, "Remove secondary index iid = %u", index_id)); Simply without branching: "Remove index with iid = %u" > +} > + > /** > * 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 = > - 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) != 0) { > - sqlDbFree(parse_context->db, constraint_name); > - return; > - } Why did you drop this check? > 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 = > - 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) != 0) > - return; Same question. > 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 = ++parse_context->nMem; > int space_id_reg = ++parse_context->nMem; > - int index_id_reg = ++parse_context->nMem; > int space_id = 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 = 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 = %u", > - space->index[i]->def->iid)); > + vdbe_emit_index_drop(parse_context, > + space->index[i]->def->iid, > + space_id); I'd better move this refactoring to a separate patch. > } > } > - 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; > } > > +/** > + * 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 = &parse_context->drop_fk_def.base; > - assert(drop_def->base.entity_type == ENTITY_TYPE_FK); > + struct drop_entity_def *drop_def = > + &parse_context->drop_constraint_def.base; > + assert(drop_def->base.entity_type == ENTITY_TYPE_CONSTRAINT); > assert(drop_def->base.alter_action == ALTER_ACTION_DROP); > const char *table_name = drop_def->base.entity_name->a[0].zName; > assert(table_name != NULL); > - struct space *child = space_by_name(table_name); > - if (child == NULL) { > + struct space *space = space_by_name(table_name); > + if (space == NULL) { > diag_set(ClientError, ER_NO_SUCH_SPACE, table_name); > parse_context->is_aborted = true; > return; > } > - char *constraint_name = > - sql_name_from_token(parse_context->db, &drop_def->name); > - if (constraint_name == NULL) { > + char *name = sql_name_from_token(parse_context->db, &drop_def->name); > + if (name == NULL) { > + parse_context->is_aborted = true; > + return; > + } > + struct constraint_id *id = space_find_constraint_id(space, name); > + if (id == NULL) { > + diag_set(ClientError, ER_NO_SUCH_CONSTRAINT, name, table_name); > parse_context->is_aborted = true; > return; > } > - vdbe_emit_fk_constraint_drop(parse_context, constraint_name, > - child->def->id); > + uint32_t space_id = space->def->id; > + assert(id->type == CONSTRAINT_TYPE_PK || id->type == > + CONSTRAINT_TYPE_UNIQUE || id->type == CONSTRAINT_TYPE_FK || > + id->type == CONSTRAINT_TYPE_CK); 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); > + switch (id->type) { > + case CONSTRAINT_TYPE_PK: > + case CONSTRAINT_TYPE_UNIQUE: { > + uint32_t index_id = 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. > + */ Then add assert(index_id != 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 > @@ -2704,18 +2731,8 @@ sql_drop_index(struct Parse *parse_context) > goto exit_drop_index; > } > > - /* > - * Generate code to remove entry from _index space > - * But firstly, delete statistics since schema > - * changes after DDL. > - */ > - int record_reg = ++parse_context->nMem; > - int space_id_reg = ++parse_context->nMem; > - int index_id_reg = ++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. */ Looks like obsolete comment. > 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);') > > +-- > +-- 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 ~= nil But PK is not dropped. Please, add tests where PK will be successfully dropped. > +-- Drop UNIQUE constraint. > +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') > +box.space.T2.index.E == nil > +-- Drop CHECK constraint. > +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') > +box.space.T2.ck_constraint.E ~= nil > +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') > +box.space.T2.ck_constraint.E == 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. > +-- This patch is about extending SQL syntax, don't get how this check is related to subj.. > +box.begin() \ > +box.execute('CREATE UNIQUE INDEX e ON t2(i);') \ > +-- Drop UNIQUE constraint. \ > +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \ > +-- Drop CHECK constraint. \ > +box.execute('ALTER TABLE t2 ADD CONSTRAINT e CHECK(i > 0);') \ > +box.execute('ALTER TABLE t2 DROP CONSTRAINT e;') \ > +-- 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;') \ > +box.commit() > + > -- > -- Cleanup. > -- > -- > 2.21.0 (Apple Git-122) >