From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp38.i.mail.ru (smtp38.i.mail.ru [94.100.177.98]) (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 54E2F440F3D for ; Wed, 13 Nov 2019 01:58:23 +0300 (MSK) References: <49805fa2211dfa3e5d92a34a740a3f9cf488c327.1573566885.git.roman.habibov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <272b5cb3-cc80-8f8b-f13c-b5b6516c9b30@tarantool.org> Date: Wed, 13 Nov 2019 00:04:31 +0100 MIME-Version: 1.0 In-Reply-To: <49805fa2211dfa3e5d92a34a740a3f9cf488c327.1573566885.git.roman.habibov@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH 1/2] box: introduce constraint names hash table List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Roman Khabibov , tarantool-patches@dev.tarantool.org Thanks for the patch! See 9 comments below. On 12/11/2019 15:02, Roman Khabibov wrote: > Add hash table and API for interaction with it to struct space. This > hash table is needed to keep constraint names of a table together and check > their duplicates. > > Needed for #3503 1. This is rather a part of 3503. Not something self sufficient. > --- > src/box/space.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > src/box/space.h | 40 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 86 insertions(+) > > diff --git a/src/box/space.c b/src/box/space.c > index 94716a414..c15b9f5b4 100644 > --- a/src/box/space.c > +++ b/src/box/space.c > @@ -202,6 +203,13 @@ space_create(struct space *space, struct engine *engine, > } > } > } > + > + space->constraint_names = mh_strnptr_new(); 2. I think you need to store more information than just a name. You need to store constraint type and its ID to be able to drop a constraint knowing only its name. > + if (space->constraint_names == NULL) { > + diag_set(OutOfMemory, sizeof(*space->constraint_names), "malloc", > + "constraint_names"); > + goto fail; > + } > return 0; > > fail_free_indexes: > @@ -257,6 +265,7 @@ space_delete(struct space *space) > trigger_destroy(&space->before_replace); > trigger_destroy(&space->on_replace); > space_def_delete(space->def); > + mh_strnptr_delete(space->constraint_names); 3. Please, add an assertion, that the hash is empty. > /* > * SQL Triggers should be deleted with > * on_replace_dd_trigger on deletion from _trigger. > @@ -617,6 +626,43 @@ space_remove_ck_constraint(struct space *space, struct ck_constraint *ck) > } > } > > +bool > +space_has_constraint(struct space *space, const char *name) > +{ > + uint32_t len = strlen(name); > + mh_int_t pos = mh_strnptr_find_inp(space->constraint_names, name, len); > + if (pos != mh_end(space->constraint_names)) > + return true; > + return false; 4. --- a/src/box/space.c +++ b/src/box/space.c @@ -631,9 +631,7 @@ space_has_constraint(struct space *space, const char *name) { uint32_t len = strlen(name); mh_int_t pos = mh_strnptr_find_inp(space->constraint_names, name, len); - if (pos != mh_end(space->constraint_names)) - return true; - return false; + return pos != mh_end(space->constraint_names); } > +} > + > +int > +space_put_constraint_name(struct space *space, const char *name) > +{ > + uint32_t len = strlen(name); > + uint32_t hash = mh_strn_hash(name, len); > + char *name_copy = malloc(len); 5. You need to check for malloc result. > + memcpy(name_copy, name, len); > + const struct mh_strnptr_node_t name_node = { name_copy, len, hash, > + NULL }; > + if (mh_strnptr_put(space->constraint_names, &name_node, NULL, > + NULL) == mh_end(space->constraint_names)) { > + diag_set(OutOfMemory, sizeof(name_node), "malloc", > + "constraint_names"); > + return -1; 6. name_copy leaks here. > + } > + > + return 0; > +} > + > +void > +space_drop_constraint_name(struct space *space, const char *name) > +{ > + mh_int_t pos = mh_strnptr_find_inp(space->constraint_names, name, > + strlen(name)); > + mh_strnptr_del(space->constraint_names, pos, NULL); 7. name_copy leaks here as well. > +} > + > /* {{{ Virtual method stubs */ > > size_t > diff --git a/src/box/space.h b/src/box/space.h > index 7926aa65e..36a767421 100644 > --- a/src/box/space.h > +++ b/src/box/space.h > @@ -234,6 +234,11 @@ struct space { > * of parent constraints as well as child ones. > */ > uint64_t fk_constraint_mask; > + /** > + * Hash table with constraint names. Used for non-system > + * spaces only. 8. Why for non-system spaces only? > + */ > + struct mh_strnptr_t *constraint_names; > }; > > /** Initialize a base space instance. */ > @@ -516,6 +521,41 @@ space_add_ck_constraint(struct space *space, struct ck_constraint *ck); > void > space_remove_ck_constraint(struct space *space, struct ck_constraint *ck); > > +/** > + * Check if a constraint with @a name exists within @a space. > + * > + * @param space Space. > + * @param name Constraint name. > + * > + * @retval true The constraint with @a name exists. > + * @retval false Doesn't exist respectively. > + */ > +bool > +space_has_constraint(struct space *space, const char *name); > + > +/** > + * Put node with @a name of a constraint the the hash table of @a 9. 'the the' ? > + * space. > + * > + * @param space Space. > + * @param name Constraint name. > + * > + * @retval 0 Success. > + * @retval -1 Memory allocation error. > + */ > +int > +space_put_constraint_name(struct space *space, const char *name);