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 8620627963 for ; Sun, 5 Aug 2018 20:28:07 -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 iEyp1UalONf1 for ; Sun, 5 Aug 2018 20:28:07 -0400 (EDT) Received: from smtp60.i.mail.ru (smtp60.i.mail.ru [217.69.128.40]) (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 031B427913 for ; Sun, 5 Aug 2018 20:28:07 -0400 (EDT) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 10.3 \(3273\)) Subject: [tarantool-patches] Re: [PATCH 3/5] sql: introduce ADD CONSTRAINT statement From: "n.pettik" In-Reply-To: Date: Mon, 6 Aug 2018 03:28:04 +0300 Content-Transfer-Encoding: quoted-printable Message-Id: References: <092e07d5399d662cd38b755a0403b11cc4dde2a1.1531443603.git.korablev@tarantool.org> <647dcc4b-a2c4-5cc8-c81c-2dbb66ce18ed@tarantool.org> <9B5CA4F9-A6DB-46FF-BE86-CC3900D22D97@tarantool.org> 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: tarantool-patches@freelists.org Cc: Vladislav Shpilevoy I squashed your fixes. However, personally I don=E2=80=99t understand = why you decided to refactor endTable() routine, for instance. Ofc refactoring is always appreciated but I would prefer separating functional part of = patch and cosmetic/codestyle changes. Initial version of this patch AFAIR included = 1k +- lines. Now (due to codestyle fixes) it is about 1.8k + and 1.6k -.. It would be = quite complicated to find smth in such giant diff. >> 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 >=20 > 1. No comment. Comment for static inline two-lines function? Seriously? Ok, I will add it, but I think we should consider making an exception in our developing guide for instance for static functions less than 10 lines or sort of. +++ b/src/box/alter.cc @@ -3665,6 +3665,15 @@ fkey_grab_by_name(struct rlist *list, const char = *fkey_name) */ #define FKEY_MASK(x) (((x)>31) ? 0xffffffff : ((uint64_t)1<<(x))) =20 +/** + * Set bits of @mask which correspond to fields involved in + * given foreign key constraint. + * + * @param fk Links of this FK constraint are used to update mask. + * @param[out] mask Mask to be updated. + * @param type Type of links to be used to update mask: + * parent or child. + */ static inline void fkey_set_mask(const struct fkey *fk, uint64_t *mask, int type) >=20 >> +fkey_set_mask(const struct fkey *fk, uint64_t *parent_mask, >> + uint64_t *child_mask) >> +{ >> + for (uint32_t i =3D 0; i < fk->def->field_count; ++i) { >> + *parent_mask |=3D = FKEY_MASK(fk->def->links[i].parent_field); >> + *child_mask |=3D = 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); >=20 > 2. Use after free. Skipped since you already fixed on branch. >> } >> /** 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 =3D (struct fkey *)trigger->data; >> + fkey_update_mask(fk); >=20 > 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. Yep, it is definitely mistake. Also skipped. >> 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 =3D get_enc(buf); >> + char *p =3D enc->encode_array(buf, link_count); >> + for (uint32_t i =3D 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 =3D sizeof(struct field_link) * i; >> + p =3D enc->encode_uint(p, *((char *) links + offset)); >=20 > 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. The same (I realised my mistake and skipped). >=20 >> + } >> + 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 =3D sqlite3DbStrDup(v->db, >> + = child_fk->def->name); >> + if (fk_name_dup =3D=3D NULL) >> + return; >> + vdbe_emit_fkey_drop(parse_context, fk_name_dup, = space_id); >=20 > 5. Can we use P4_STATIC and do not duplicate? No, unfortunately we can=E2=80=99t. P4_STATIC attribute is usable only = when the same memory is used twice during VDBE execution. STATIC attribute prevents = from memory releasing (even at the end of execution AFAIK). If we released = memory right after generating opcode (or at the end of parsing, it doesn=E2=80=99= t matter), we will probably get corrupted memory. > 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. Can be used, but wouldn=E2=80=99t be freed (or I misunderstand what you = initially meant). > 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). >=20 >> + } >> /* >> * 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] =3D '\0'; >> + sqlite3NormalizeName(fk->name); >=20 > 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. Ok, I forgot about normalization in sqlite3NameFromToken(). Thx for fix.