From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 40096469719 for ; Tue, 11 Feb 2020 19:56:25 +0300 (MSK) Date: Tue, 11 Feb 2020 19:56:24 +0300 From: Nikita Pettik Message-ID: <20200211165624.GB8095@tarantool.org> References: <20200109101502.4158-1-roman.habibov@tarantool.org> <20200113170045.GD7851@tarantool.org> <17741BF6-ADCA-4B87-BD6C-955DAF009EC7@tarantool.org> <20200128173939.GA16149@tarantool.org> <5204001A-28FC-407A-918E-88DE7168C95C@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5204001A-28FC-407A-918E-88DE7168C95C@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: Roman Khabibov Cc: tarantool-patches@dev.tarantool.org On 01 Feb 20:36, Roman Khabibov wrote: > Hi! Thanks for the review. > > >>>> 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); > >>>> @@ -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. > >> 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. > > > > Data can be consistent at the parsing stage, but be inconsistent during > > execution. Especially taking into account 'prepare' mechanism having > > been recently introduced. So please put these checks back. > Done. In sql_drop_constraint() in index dropping branch there's still no vdbe_emit_halt_with_presence_test() call. Please add it there as well. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index d9bf8de91..27963c455 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -2052,35 +2052,74 @@ 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. > + */ As a rule we place comments near function declaration. > void > -sql_drop_foreign_key(struct Parse *parse_context) > +sql_drop_constraint(struct Parse *parse_context) > { > } > - vdbe_emit_fk_constraint_drop(parse_context, constraint_name, > - child->def); > + struct Vdbe *v = sqlGetVdbe(parse_context); > + assert(v != NULL); > + assert(id->type < constraint_type_MAX); > + switch (id->type) { > + case CONSTRAINT_TYPE_PK: > + case CONSTRAINT_TYPE_UNIQUE: { > + uint32_t index_id = box_index_id_by_name(space->def->id, name, > + strlen(name)); > + /* > + * We have already verified, that this index > + * exists, so we don't check index_id for > + * BOX_ID_NIL. > + */ > + assert(index_id != BOX_ID_NIL); > + 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); > + break; > + } > + case CONSTRAINT_TYPE_FK: > + vdbe_emit_fk_constraint_drop(parse_context, name, space->def); > + break; > + case CONSTRAINT_TYPE_CK: > + vdbe_emit_ck_constraint_drop(parse_context, name, space->def); > + 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. > */ This comment doesn't look like related to this function: sql_drop_constraint() is never called within drop table statement. > - struct Vdbe *v = sqlGetVdbe(parse_context); > 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 ::= alter_table_start(A) RENAME TO nm(N). { > } > > cmd ::= 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 = true; > - sql_drop_foreign_key(pParse); > + sql_drop_constraint(pParse); > } > > cmd ::= 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. > - */ Why did you drop this comment? > ENTITY_TYPE_CONSTRAINT,