From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 9FF6327C63 for ; Thu, 2 Aug 2018 18:15:14 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id JwrxkEQL93z5 for ; Thu, 2 Aug 2018 18:15:14 -0400 (EDT) Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 5C89C27C45 for ; Thu, 2 Aug 2018 18:15:14 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH 3/5] sql: introduce ADD CONSTRAINT statement References: <092e07d5399d662cd38b755a0403b11cc4dde2a1.1531443603.git.korablev@tarantool.org> <647dcc4b-a2c4-5cc8-c81c-2dbb66ce18ed@tarantool.org> <9B5CA4F9-A6DB-46FF-BE86-CC3900D22D97@tarantool.org> From: Vladislav Shpilevoy Message-ID: Date: Fri, 3 Aug 2018 01:15:12 +0300 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: "n.pettik" , tarantool-patches@freelists.org Thanks for the patch! See 6 comments below and a separate commit on the branch. > diff --git a/src/box/alter.cc b/src/box/alter.cc > index 5b55bfd7a..6b9e29470 100644 > --- a/src/box/alter.cc > +++ b/src/box/alter.cc > @@ -3660,6 +3660,50 @@ fkey_grab_by_name(struct rlist *list, const char *fkey_name) > return NULL; > } > > +static void 1. No comment. > +fkey_set_mask(const struct fkey *fk, uint64_t *parent_mask, > + uint64_t *child_mask) > +{ > + for (uint32_t i = 0; i < fk->def->field_count; ++i) { > + *parent_mask |= FKEY_MASK(fk->def->links[i].parent_field); > + *child_mask |= FKEY_MASK(fk->def->links[i].child_field); > + } > +} > + > @@ -3673,6 +3717,7 @@ on_create_fkey_rollback(struct trigger *trigger, void *event) > rlist_del_entry(fk, parent_link); > rlist_del_entry(fk, child_link); > fkey_delete(fk); > + fkey_update_mask(fk); 2. Use after free. > } > > /** Return old FK and release memory for the new one. */ > @@ -3712,6 +3759,7 @@ on_drop_or_replace_fkey_commit(struct trigger *trigger, void *event) > { > (void) event; > struct fkey *fk = (struct fkey *)trigger->data; > + fkey_update_mask(fk); 3. You should not update mask on commit. The mask should be updated before the commit yielded. Else, during yield, some new requests can arrive which will not match the mask despite of presence of the fkey in the space. > fkey_delete(fk); > } > > diff --git a/src/box/sql.c b/src/box/sql.c > index 9795ad2ac..46a0c3472 100644 > --- a/src/box/sql.c > +++ b/src/box/sql.c > @@ -1489,6 +1393,23 @@ tarantoolSqlite3MakeTableOpts(Table *pTable, const char *zSql, char *buf) > return p - buf; > } > > +int > +fkey_encode_links(uint32_t *links, uint32_t link_count, char *buf) > +{ > + const struct Enc *enc = get_enc(buf); > + char *p = enc->encode_array(buf, link_count); > + for (uint32_t i = 0; i < link_count; ++i) { > + /* > + * field_link consists of two uin32_t members, > + * so if we calculate proper offset, we will > + * get next parent/child member. > + */ > + size_t offset = sizeof(struct field_link) * i; > + p = enc->encode_uint(p, *((char *) links + offset)); 4. Encode_uint takes a second argument of type uint64_t. But you cast here links to char * and then unreference it so it becomes just char - 8 bits. > + } > + return p - buf; > +} > + > /* > * Format "parts" array for _index entry. > * Returns result size. > diff --git a/src/box/sql/build.c b/src/box/sql/build.c > index 819c2626a..13013ee5a 100644 > --- a/src/box/sql/build.c > +++ b/src/box/sql/build.c > @@ -1982,6 +2165,15 @@ sql_code_drop_table(struct Parse *parse_context, struct space *space, > sqlite3VdbeAddOp2(v, OP_SDelete, BOX_SEQUENCE_ID, idx_rec_reg); > VdbeComment((v, "Delete entry from _sequence")); > } > + /* Delete all child FK constraints. */ > + struct fkey *child_fk; > + rlist_foreach_entry (child_fk, &space->child_fkey, child_link) { > + const char *fk_name_dup = sqlite3DbStrDup(v->db, > + child_fk->def->name); > + if (fk_name_dup == NULL) > + return; > + vdbe_emit_fkey_drop(parse_context, fk_name_dup, space_id); 5. Can we use P4_STATIC and do not duplicate? I want to say, that if we do not duplicate the memory here, then only 2 situations are possible: 1) Until the VDBE is executed nothing is happened, the memory is ok and can be used. 2) Such alter occurred before VDBE execution so the memory is dirty and we can not use it. But in such a case schema_version is updated and the VDBE is expired entirely that leads to its regeneration (will lead). > + } > /* > * Drop all _space and _index entries that refer to the > * table. > @@ -2106,177 +2299,281 @@ sql_drop_table(struct Parse *parse_context, struct SrcList *table_name_list, > +void > +sql_create_foreign_key(struct Parse *parse_context, struct SrcList *child, > + struct Token *constraint, struct ExprList *child_cols, > + struct Token *parent, struct ExprList *parent_cols, > + bool is_deferred, int actions) > +{ > - > - /* Link the foreign key to the table as the last step. > + memcpy(fk->name, constraint_name, name_len); > + fk->name[name_len] = '\0'; > + sqlite3NormalizeName(fk->name); 6. You do not need to normalize it here since the name was either auto-generated or got from sqlite3NameFromToken(). The latter already calls normalize(). The former should generate a well-formed name. What is more, if you call normalize only here, then all the errors above with auto-generated names appears in lowercase, but below - in uppercase, that leads to curious test results: somewhere errors show fk_constraint_1_C, in another places: FK_CONSTRAINT_1_C. > + /* > + * In case of CREATE TABLE processing, all foreign keys > + * constraints must be created after space itself, so > + * lets delay it until sqlite3EndTable() call and simply > + * maintain list of all FK constraints inside parser. > */ > - p->pFKey = pFKey; > - pFKey = 0; > + if (!is_alter) { > + struct fkey_parse *parse_fk = > + rlist_first_entry(&parse_context->new_fkey, > + struct fkey_parse, link); > + parse_fk->fkey = fk; > + } else { > + vdbe_emit_fkey_create(parse_context, fk); > + } >