From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp49.i.mail.ru (smtp49.i.mail.ru [94.100.177.109]) (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 8EAFA4696C0 for ; Sat, 30 Nov 2019 04:03:24 +0300 (MSK) References: <8187f6bb57792948fb122d3c2cb3cfc72975fa77.1574965971.git.roman.habibov@tarantool.org> From: Vladislav Shpilevoy Message-ID: <11d5344a-0830-9c5c-9e75-30564aa257d0@tarantool.org> Date: Sat, 30 Nov 2019 02:03:22 +0100 MIME-Version: 1.0 In-Reply-To: <8187f6bb57792948fb122d3c2cb3cfc72975fa77.1574965971.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 v2 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 6 comments below! On 28/11/2019 19:34, Roman Khabibov wrote: > Add hash table and API for interaction with it to struct space. > This hash table is needed to keep constraint of a table together > and check their duplicates by name. > > Part of #3503 > --- > src/box/CMakeLists.txt | 1 + > src/box/constraint_def.c | 59 ++++++++++++++++++++++++++++ > src/box/constraint_def.h | 83 ++++++++++++++++++++++++++++++++++++++++ > src/box/space.c | 56 +++++++++++++++++++++++++++ > src/box/space.h | 39 +++++++++++++++++++ > 5 files changed, 238 insertions(+) > create mode 100755 src/box/constraint_def.c > create mode 100755 src/box/constraint_def.h > > diff --git a/src/box/constraint_def.c b/src/box/constraint_def.c > new file mode 100755 > index 000000000..914b36da3 > --- /dev/null > +++ b/src/box/constraint_def.c > +#include "constraint_def.h" > +#include "assoc.h" > +#include "errcode.h" > +#include "diag.h" > +#include > +#include > + > +struct constraint_def * > +constraint_def_new(uint32_t space_id, enum constraint_type type, > + const char *name) { 1. Please, put { on a separate line. In all the other places too. > + uint32_t len = strlen(name); > + uint32_t size = sizeof(struct constraint_def) + len + 1; > + struct constraint_def *ret = malloc(size); > + if (ret == NULL) { > + diag_set(OutOfMemory, size, "malloc", "constraint_def"); > + return NULL; > + } > + ret->space_id = space_id; > + ret->type = type; > + memcpy(ret->name, name, len + 1); > + return ret; > +} > + > +void > +constraint_def_free(struct constraint_def *def) { 2. Please, rename it to _delete(). We always use new/delete() for constructor/destructor managing memory. Free() is used only by a few obsolete places, and by subsystem destructors. > + free(def); > +} > diff --git a/src/box/constraint_def.h b/src/box/constraint_def.h > new file mode 100755 > index 000000000..ef238eb46 > --- /dev/null > +++ b/src/box/constraint_def.h > @@ -0,0 +1,83 @@ > +#ifndef INCLUDES_BOX_CONSTRAINT_DEF_H > +#define INCLUDES_BOX_CONSTRAINT_DEF_H 3. Please, take a look at other header guards. They have a certain naming policy. For example, tuple.h in box/ has a guard TARANTOOL_BOX_TUPLE_H_INCLUDED. I guess you could use TARANTOOL_BOX_CONSTRAINT_DEF_H_INCLUDED. But even better would be to use #pragma once. I don't know whether they are allowed in our code though. > +/* > + * Copyright 2010-2019, Tarantool AUTHORS, please see AUTHORS > + * file. > + * > + * Redistribution and use in source and binary forms, with or > + * without modification, are permitted provided that the following > + * conditions are met: > + * > + * 1. Redistributions of source code must retain the above > + * copyright notice, this list of conditions and the > + * following disclaimer. > + * > + * 2. Redistributions in binary form must reproduce the above > + * copyright notice, this list of conditions and the following > + * disclaimer in the documentation and/or other materials > + * provided with the distribution. > + * > + * THIS SOFTWARE IS PROVIDED BY ``AS IS'' AND > + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED > + * TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL > + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, > + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL > + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF > + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR > + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF > + * THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF > + * SUCH DAMAGE. > + */ > + > +#include > + > +#if defined(__cplusplus) > +extern "C" { > +#endif > + > +enum constraint_type { > + CONSTRAINT_TYPE_PK = 0, 4. You can omit the assignments, if they does not matter anyway. > + CONSTRAINT_TYPE_UNIQUE = 1, > + CONSTRAINT_TYPE_FK = 2, > + CONSTRAINT_TYPE_CK = 3 > +}; > diff --git a/src/box/space.c b/src/box/space.c > index 94716a414..91a7d575b 100644 > --- a/src/box/space.c > +++ b/src/box/space.c > @@ -257,6 +266,20 @@ space_delete(struct space *space) > trigger_destroy(&space->before_replace); > trigger_destroy(&space->on_replace); > space_def_delete(space->def); > + > + /** > + * Free memory of the constraint hash table. Destroy every > + * constraint def object. 5. There should not be any objects by that time. Why is not the hash table empty? > + */ > + struct mh_strnptr_t *mh = space->constraint_names; > + while (mh_size(mh) > 0) { > + mh_int_t i = mh_first(mh); > + struct constraint_def *def = > + (struct constraint_def *) mh_strnptr_node(mh, i)->val; > + constraint_def_free(def); > + } > + mh_strnptr_delete(mh); > + > /* > * SQL Triggers should be deleted with > * on_replace_dd_trigger on deletion from _trigger. > @@ -617,6 +640,39 @@ space_remove_ck_constraint(struct space *space, struct ck_constraint *ck)> diff --git a/src/box/space.h b/src/box/space.h > index 7926aa65e..96396311e 100644 > --- a/src/box/space.h > +++ b/src/box/space.h > @@ -516,6 +519,42 @@ space_add_ck_constraint(struct space *space, struct ck_constraint *ck); > +struct constraint_def * > +space_constraint_def_by_name(struct space *space, const char *name); > + > +/** > + * Put node with @a def to the constraint hash table of @a space. > + * > + * @param space Space. > + * @param def Constraint def. > + * > + * @retval 0 Success. > + * @retval -1 Memory allocation error. > + */ > +int > +space_put_constraint(struct space *space, struct constraint_def *def); > + > +/** > + * Remove node with @a name from the constraint hash table of @a > + * space. But don't destroy the constraint def object binded to > + * this @a name. > + * > + * @param space Space. > + * @param name Constraint name. > + */ > +void > +space_drop_constraint(struct space *space, const char *name); 6. That function is a bit confusing. I understand, why do you need to not destroy a constraint right here. But drop() name really assumes that. Moreover, in most of the places you do space_constraint_def_by_name() before space_drop_constraint(). So this is a double search. I propose you to rename drop to pop, and make it return the old constraint. It will make clear, that the constraint is not deleted here, and will eliminate the double search.