From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org> To: Roman Khabibov <roman.habibov@tarantool.org>, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH v2 1/2] box: introduce constraint names hash table Date: Sat, 30 Nov 2019 02:03:22 +0100 [thread overview] Message-ID: <11d5344a-0830-9c5c-9e75-30564aa257d0@tarantool.org> (raw) In-Reply-To: <8187f6bb57792948fb122d3c2cb3cfc72975fa77.1574965971.git.roman.habibov@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 <string.h> > +#include <stdlib.h> > + > +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 <COPYRIGHT HOLDER> ``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 > + * <COPYRIGHT HOLDER> 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 <stdint.h> > + > +#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.
next prev parent reply other threads:[~2019-11-30 1:03 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-11-28 18:34 [Tarantool-patches] [PATCH v2 0/2] Add constraint names hash table to space Roman Khabibov 2019-11-28 18:34 ` [Tarantool-patches] [PATCH v2 1/2] box: introduce constraint names hash table Roman Khabibov 2019-11-30 1:03 ` Vladislav Shpilevoy [this message] 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 1/3] " Roman Khabibov 2019-12-07 16:34 ` Vladislav Shpilevoy 2019-12-10 12:48 ` Roman Khabibov 2019-11-28 18:34 ` [Tarantool-patches] [PATCH v2 2/2] sql: make constraint operations transactional Roman Khabibov 2019-11-29 7:38 ` Roman Khabibov 2019-11-30 1:03 ` Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 2/3] " Roman Khabibov 2019-12-05 18:43 ` Roman Khabibov 2019-12-07 16:35 ` Vladislav Shpilevoy 2019-12-10 12:49 ` [Tarantool-patches] [PATCH v2 2/3] box: " Roman Khabibov 2019-12-15 22:26 ` Vladislav Shpilevoy 2019-12-17 15:03 ` Roman Khabibov 2019-12-28 0:18 ` Nikita Pettik 2019-12-28 11:07 ` Vladislav Shpilevoy 2019-12-29 0:07 ` Nikita Pettik 2019-12-29 15:51 ` Vladislav Shpilevoy 2019-12-29 22:28 ` Nikita Pettik 2019-12-29 22:35 ` Vladislav Shpilevoy 2019-12-30 11:12 ` Sergey Ostanevich 2019-12-30 12:05 ` Nikita Pettik 2019-12-21 20:54 ` Sergey Ostanevich 2019-12-22 14:59 ` Vladislav Shpilevoy 2019-12-24 12:06 ` Roman Khabibov 2019-11-30 1:03 ` [Tarantool-patches] [PATCH v2 0/2] Add constraint names hash table to space Vladislav Shpilevoy 2019-12-04 16:23 ` [Tarantool-patches] [PATCH v2 0/3] " Roman Khabibov 2019-12-07 16:34 ` Vladislav Shpilevoy
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=11d5344a-0830-9c5c-9e75-30564aa257d0@tarantool.org \ --to=v.shpilevoy@tarantool.org \ --cc=roman.habibov@tarantool.org \ --cc=tarantool-patches@dev.tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH v2 1/2] box: introduce constraint names hash table' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox